Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Doug Evans]

Gary Benson writes:
  > This commit allows GDB to access executables and shared libraries
  > on native Linux targets where GDB and the inferior have different
  > mount namespaces.
  >
  > gdb/ChangeLog:
  >
  > 	* linux-nat.c (nat/linux-namespaces.h): New include.
  > 	(fileio.h): Likewise.
  > 	(linux_nat_filesystem_is_local): New function.
  > 	(linux_nat_fileio_pid_of): Likewise.
  > 	(linux_nat_fileio_open): Likewise.
  > 	(linux_nat_fileio_readlink): Likewise.
  > 	(linux_nat_fileio_unlink): Likewise.
  > 	(linux_nat_add_target): Initialize to_filesystem_is_local,
  > 	to_fileio_open, to_fileio_readlink and to_fileio_unlink.
  > 	(_initialize_linux_nat): New "set/show debug lin-ns" commands.
  > 	* NEWS: Mention new "set/show debug lin-ns" commands.

Hi.

Nit: "set debug" options need a doc entry.
[I didn't see it in another patch.
I also didn't see this option used in this patch.]

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This commit allows GDB to access executables and shared libraries
> > on native Linux targets where GDB and the inferior have different
> > mount namespaces.
> >
> > gdb/ChangeLog:
> >
> > 	* linux-nat.c (nat/linux-namespaces.h): New include.
> > 	(fileio.h): Likewise.
> > 	(linux_nat_filesystem_is_local): New function.
> > 	(linux_nat_fileio_pid_of): Likewise.
> > 	(linux_nat_fileio_open): Likewise.
> > 	(linux_nat_fileio_readlink): Likewise.
> > 	(linux_nat_fileio_unlink): Likewise.
> > 	(linux_nat_add_target): Initialize to_filesystem_is_local,
> > 	to_fileio_open, to_fileio_readlink and to_fileio_unlink.
> > 	(_initialize_linux_nat): New "set/show debug lin-ns" commands.
> > 	* NEWS: Mention new "set/show debug lin-ns" commands.
> 
> Nit: "set debug" options need a doc entry.
> [I didn't see it in another patch.
> I also didn't see this option used in this patch.]

It's used in nat/linux-namespaces.c.
I'll add some documentation for it.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This commit allows GDB to access executables and shared libraries
> > on native Linux targets where GDB and the inferior have different
> > mount namespaces.
> >
> > gdb/ChangeLog:
> >
> > 	* linux-nat.c (nat/linux-namespaces.h): New include.
> > 	(fileio.h): Likewise.
> > 	(linux_nat_filesystem_is_local): New function.
> > 	(linux_nat_fileio_pid_of): Likewise.
> > 	(linux_nat_fileio_open): Likewise.
> > 	(linux_nat_fileio_readlink): Likewise.
> > 	(linux_nat_fileio_unlink): Likewise.
> > 	(linux_nat_add_target): Initialize to_filesystem_is_local,
> > 	to_fileio_open, to_fileio_readlink and to_fileio_unlink.
> > 	(_initialize_linux_nat): New "set/show debug lin-ns" commands.
> > 	* NEWS: Mention new "set/show debug lin-ns" commands.
> 
> Nit: "set debug" options need a doc entry.
> [I didn't see it in another patch.

Eli, is this ok?  (Not as a standalone patch, I'll merge it...)

Thanks,
Gary

-- 
gdb/doc/ChangeLog:

	* gdb.texinfo (Debugging Output): Document the "set/show debug
	linux-namespaces" command.
---
 gdb/doc/ChangeLog   |    5 +++++
 gdb/doc/gdb.texinfo |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9ea846a..e1f2e58 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23484,6 +23484,11 @@ Displays the current state of @value{GDBN} JIT debugging.
 Turns on or off debugging messages from the Linux LWP debug support.
 @item show debug lin-lwp
 Show the current state of Linux LWP debugging messages.
+@item set debug linux-namespaces
+@cindex @sc{gnu}/Linux namespaces debug messages
+Turns on or off debugging messages from the Linux namespaces debug support.
+@item show debug linux-namespaces
+Show the current state of Linux namespaces debugging messages.
 @item set debug mach-o
 @cindex Mach-O symbols processing
 Control display of debugging messages related to Mach-O symbols
-- 
1.7.1

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Eli Zaretskii]

> From: Gary Benson <gbenson@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Pedro Alves <palves@redhat.com>,
>         Doug Evans <dje@google.com>
> Date: Tue,  9 Jun 2015 14:20:36 +0100
> 
> Eli, is this ok?

Yes, thanks.

[PATCH 0/7] GNU/Linux mount namespace support

[Gary Benson]

Hi all,

This series allows GDB and gdbserver on GNU/Linux systems to attach
to processes in mount namespaces other than the debugger's own when
invoked with the appropriate permissions.  This allows an application
running in a container to be debugged by running GDB or gdbserver on
the host, or by running GDB or gdbserver from within another container
that shares the host's PID namespace.

This series builds on the 'Do not require "file" commands for remote
targets' series that is currently under review:

  https://sourceware.org/ml/gdb-patches/2015-04/msg00015.html

It also requires the 'Do not manipulate "target:" filenames as local
paths' patch to function correctly:

  https://sourceware.org/ml/gdb-patches/2015-04/msg00547.html

A tree with the latest versions of all these patches applied is
available here:

  https://github.com/gbenson/binutils-gdb/tree/namespaces

The patches in this series are as follows:

 - Patch 1 (Move make_cleanup_close to common code) is a minor
   refactoring required for patch 3.

 - Patch 2 (Introduce target_fileio_set_fs) adds target-layer
   support for systems where different processes can have different
   views of the filesystem.  The new function target_fileio_set_fs
   sets the context in which target_filesystem_is_local and the
   other target_fileio_* functions operate, and can be called to
   select the filesystem of a specific inferior or the filesystem
   of the debugger (GDB or gdbserver).  The default implementation
   of target_fileio_set_fs does nothing, which means that targets
   where inferiors and the debugger share the same filesystem will
   work without modification.

 - Patch 3 (Introduce nat/linux-namespaces.[ch]) introduces some
   shared code that GDB and gdbserver can use to query and enter
   Linux namespaces.

 - GDB contained a small amount of namespace-aware code used to
   display a warning.  Patch 4 (Remove linux_proc_pid_get_ns)
   replaces the querying part of this code with a call to the
   new code introduced by patch 3.

 - Patch 5 (Implement multiple-filesystem support for Linux targets)
   does exactly what it says on the tin.  With this patch you can
   attach to processes in containers using the native Linux target
   using, e.g., "gdb -p PID".

 - Patch 6 (Implement multiple-filesystem support for remote targets)
   adds remote protocol support for remote targets where different
   processes can have different views of the filesystem.  A new
   vFile:setfs packet is introduced that passes the context set using
   target_fileio_set_fs to the remote target.  Caching is used to
   avoid sending unnecessary vFile:setfs packets.

 - Patch 7 (Implement vFile:setfs in gdbserver) implements the
   vFile:setfs packet in gdbserver.  Currently only GNU/Linux systems
   support this; gdbserver on other systems will return empty
   responses in reply to vFile:setfs packets.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Gary Benson]

This commit allows GDB to access executables and shared libraries
on native Linux targets where GDB and the inferior have different
mount namespaces.

gdb/ChangeLog:

	* linux-nat.c (nat/linux-namespaces.h): New include.
	(fileio.h): Likewise.
	(linux_nat_filesystem_is_local): New function.
	(linux_nat_fileio_pid_of): Likewise.
	(linux_nat_fileio_open): Likewise.
	(linux_nat_fileio_readlink): Likewise.
	(linux_nat_fileio_unlink): Likewise.
	(linux_nat_add_target): Initialize to_filesystem_is_local,
	to_fileio_open, to_fileio_readlink and to_fileio_unlink.
	(_initialize_linux_nat): New "set/show debug lin-ns" commands.
	* NEWS: Mention new "set/show debug lin-ns" commands.
---
 gdb/ChangeLog   |   14 +++++++
 gdb/NEWS        |    4 ++
 gdb/linux-nat.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index d463b52..1c0e5ea 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -89,6 +89,10 @@ set|show record btrace bts buffer-size
   The obtained size may differ from the requested size.  Use "info
   record" to see the obtained buffer size.
 
+set debug lin-ns
+show debug lin-ns
+  Control display of debugging info regarding Linux namespaces.
+
 * The command 'thread apply all' can now support new option '-ascending'
   to call its specified command for all threads in ascending order.
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b04aa68..85149a2 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -66,6 +66,8 @@
 #include "target-descriptions.h"
 #include "filestuff.h"
 #include "objfiles.h"
+#include "nat/linux-namespaces.h"
+#include "fileio.h"
 
 #ifndef SPUFS_MAGIC
 #define SPUFS_MAGIC 0x23c9b64e
@@ -4842,6 +4844,104 @@ linux_nat_core_of_thread (struct target_ops *ops, ptid_t ptid)
   return -1;
 }
 
+/* Implementation of to_filesystem_is_local.  */
+
+static int
+linux_nat_filesystem_is_local (struct target_ops *ops)
+{
+  struct inferior *inf = current_inferior ();
+
+  if (inf->fake_pid_p || inf->pid == 0)
+    return 1;
+
+  return linux_ns_same (inf->pid, LINUX_NS_MNT);
+}
+
+/* Convert the INF argument passed to a to_fileio_* method
+   to a process ID suitable for passing to its corresponding
+   linux_mntns_* function.  If INF is non-NULL then the
+   caller is requesting the filesystem seen by INF.  If INF
+   is NULL then the caller is requesting the filesystem seen
+   by the GDB.  We fall back to GDB's filesystem in the case
+   that INF is non-NULL but its PID is unknown.  */
+
+static pid_t
+linux_nat_fileio_pid_of (struct inferior *inf)
+{
+  if (inf == NULL || inf->fake_pid_p || inf->pid == 0)
+    return getpid ();
+  else
+    return inf->pid;
+}
+
+/* Implementation of to_fileio_open.  */
+
+static int
+linux_nat_fileio_open (struct target_ops *self,
+		       struct inferior *inf, const char *filename,
+		       int flags, int mode, int *target_errno)
+{
+  int nat_flags;
+  int fd;
+
+  if (fileio_to_host_openflags (flags, &nat_flags) == -1)
+    {
+      *target_errno = FILEIO_EINVAL;
+      return -1;
+    }
+
+  /* We do not need to convert MODE, since the fileio protocol
+     uses the standard values.  */
+  fd = linux_mntns_open_cloexec (linux_nat_fileio_pid_of (inf),
+				 filename, nat_flags, mode);
+  if (fd == -1)
+    *target_errno = host_to_fileio_error (errno);
+
+  return fd;
+}
+
+/* Implementation of to_fileio_readlink.  */
+
+static char *
+linux_nat_fileio_readlink (struct target_ops *self,
+			   struct inferior *inf, const char *filename,
+			   int *target_errno)
+{
+  char buf[PATH_MAX];
+  int len;
+  char *ret;
+
+  len = linux_mntns_readlink (linux_nat_fileio_pid_of (inf),
+			      filename, buf, sizeof (buf));
+  if (len < 0)
+    {
+      *target_errno = host_to_fileio_error (errno);
+      return NULL;
+    }
+
+  ret = xmalloc (len + 1);
+  memcpy (ret, buf, len);
+  ret[len] = '\0';
+  return ret;
+}
+
+/* Implementation of to_fileio_unlink.  */
+
+static int
+linux_nat_fileio_unlink (struct target_ops *self,
+			 struct inferior *inf, const char *filename,
+			 int *target_errno)
+{
+  int ret;
+
+  ret = linux_mntns_unlink (linux_nat_fileio_pid_of (inf),
+			    filename);
+  if (ret == -1)
+    *target_errno = host_to_fileio_error (errno);
+
+  return ret;
+}
+
 void
 linux_nat_add_target (struct target_ops *t)
 {
@@ -4895,6 +4995,11 @@ linux_nat_add_target (struct target_ops *t)
 
   t->to_core_of_thread = linux_nat_core_of_thread;
 
+  t->to_filesystem_is_local = linux_nat_filesystem_is_local;
+  t->to_fileio_open = linux_nat_fileio_open;
+  t->to_fileio_readlink = linux_nat_fileio_readlink;
+  t->to_fileio_unlink = linux_nat_fileio_unlink;
+
   /* We don't change the stratum; this target will sit at
      process_stratum and thread_db will set at thread_stratum.  This
      is a little strange, since this is a multi-threaded-capable
@@ -5012,6 +5117,15 @@ Enables printf debugging output."),
 			     show_debug_linux_nat,
 			     &setdebuglist, &showdebuglist);
 
+  add_setshow_boolean_cmd ("lin-ns", class_maintenance,
+			   &debug_linux_namespaces, _("\
+Set debugging of GNU/Linux namespaces module."), _("\
+Show debugging of GNU/Linux namespaces module."), _("\
+Enables printf debugging output."),
+			   NULL,
+			   NULL,
+			   &setdebuglist, &showdebuglist);
+
   /* Save this mask as the default.  */
   sigprocmask (SIG_SETMASK, NULL, &normal_mask);
 
-- 
1.7.1

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Eli Zaretskii]

> From: Gary Benson <gbenson@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Pedro Alves <palves@redhat.com>,
>         Doug Evans <dje@google.com>,
>         Iago López Galeiras <iago@endocode.com>
> Date: Thu, 30 Apr 2015 13:05:39 +0100
> 
> +set debug lin-ns
> +show debug lin-ns
> +  Control display of debugging info regarding Linux namespaces.

Would it be better to have a more mnemonic name, like
"linux-namespace", for example?  IMO, "lin-ns" goes too far in making
its name shorter.

Otherwise, the documentation parts are OK.

Thanks.

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Gary Benson]

Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>, Pedro Alves <palves@redhat.com>,
> >         Doug Evans <dje@google.com>,
> >         Iago López Galeiras <iago@endocode.com>
> > Date: Thu, 30 Apr 2015 13:05:39 +0100
> > 
> > +set debug lin-ns
> > +show debug lin-ns
> > +  Control display of debugging info regarding Linux namespaces.
> 
> Would it be better to have a more mnemonic name, like
> "linux-namespace", for example?  IMO, "lin-ns" goes too far in
> making its name shorter.

I can change that.  (I think I modelled it after the lin-lwp option).

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Pedro Alves]

On 04/30/2015 01:05 PM, Gary Benson wrote:
> This commit allows GDB to access executables and shared libraries
> on native Linux targets where GDB and the inferior have different
> mount namespaces.


> +/* Implementation of to_fileio_open.  */
> +
> +static int
> +linux_nat_fileio_open (struct target_ops *self,
> +		       struct inferior *inf, const char *filename,
> +		       int flags, int mode, int *target_errno)
> +{
> +  int nat_flags;
> +  int fd;
> +
> +  if (fileio_to_host_openflags (flags, &nat_flags) == -1)
> +    {
> +      *target_errno = FILEIO_EINVAL;
> +      return -1;
> +    }
> +
> +  /* We do not need to convert MODE, since the fileio protocol
> +     uses the standard values.  */

Hmm, I'm not seeing how the fileio constants have to necessarily
match the host's.  So I don't understand this comment.  What
are these standard values?

> +  fd = linux_mntns_open_cloexec (linux_nat_fileio_pid_of (inf),
> +				 filename, nat_flags, mode);
> +  if (fd == -1)
> +    *target_errno = host_to_fileio_error (errno);
> +
> +  return fd;
> +}
> +

Otherwise this looks good to me.  I'll leave the "set debug" flag's
name up to you.

Thanks,
Pedro Alves

Re: [PATCH 6/9 v2] Implement mount namespace support for native Linux targets

[Gary Benson]

Pedro Alves wrote:
> On 04/30/2015 01:05 PM, Gary Benson wrote:
> > This commit allows GDB to access executables and shared libraries
> > on native Linux targets where GDB and the inferior have different
> > mount namespaces.
> 
> > +/* Implementation of to_fileio_open.  */
> > +
> > +static int
> > +linux_nat_fileio_open (struct target_ops *self,
> > +		       struct inferior *inf, const char *filename,
> > +		       int flags, int mode, int *target_errno)
> > +{
> > +  int nat_flags;
> > +  int fd;
> > +
> > +  if (fileio_to_host_openflags (flags, &nat_flags) == -1)
> > +    {
> > +      *target_errno = FILEIO_EINVAL;
> > +      return -1;
> > +    }
> > +
> > +  /* We do not need to convert MODE, since the fileio protocol
> > +     uses the standard values.  */
> 
> Hmm, I'm not seeing how the fileio constants have to necessarily
> match the host's.  So I don't understand this comment.  What
> are these standard values?

Hmmm, I just copied this from inf_child_fileio_open.  Maybe I should
fix that first, with some convertor function that the compiler can
essentially optimize away *if* the values really do match.

> Otherwise this looks good to me.  I'll leave the "set debug" flag's
> name up to you.

Cool, thanks.

Cheers,
Gary

-- 
http://gbenson.net/