[B.A.T.M.A.N.] [PATCH 1/3] alfred: vis: Use debugfs_make_path helper

[B.A.T.M.A.N.] [PATCH 1/3] alfred: vis: Use debugfs_make_path helper

[Andrew Lunn]

By using the helper, knowledge of network namespaces can be kept to
within the helper.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 vis/vis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index c1f8dad..d1640b6 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -325,7 +325,7 @@ static int parse_orig_list(struct globals *globals)
 	char path[1024];
 	struct vis_list_entry *v_entry;
 
-	snprintf(path, sizeof(path), "/sys/kernel/debug/batman_adv/%s/originators", globals->interface);
+	debugfs_make_path(DEBUG_BATIF_PATH_FMT "/" "originators", globals->interface, path, sizeof(path));
 	fbuf = read_file(path);
 	if (!fbuf)
 		return -1;
-- 
2.7.0.rc3

[B.A.T.M.A.N.] [PATCH 2/3] alfred: Add support for network namespaces

[Andrew Lunn]

When running within a network namespace, access to files within
debugfs have to take into account the network name space. Each
namespace has its own directory under
/sys/kernel/debug/batman_adv/netns.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 batadv_query.c |  2 +-
 debugfs.c      | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 vis/vis.h      |  2 +-
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/batadv_query.c b/batadv_query.c
index c289b80..fe7c007 100644
--- a/batadv_query.c
+++ b/batadv_query.c
@@ -31,7 +31,7 @@
 #include <sys/types.h>
 #include "debugfs.h"
 
-#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s"
+#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s%s"
 #define DEBUG_TRANSTABLE_GLOBAL "transtable_global"
 #define DEBUG_ORIGINATORS "originators"
 
diff --git a/debugfs.c b/debugfs.c
index fc39322..35c1bdf 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -20,11 +20,15 @@
 
 #include "debugfs.h"
 #include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/statfs.h>
+#include <sys/types.h>
+#include <unistd.h>
 
 #ifndef DEBUGFS_MAGIC
 #define DEBUGFS_MAGIC          0x64626720
@@ -42,16 +46,59 @@ static const char *debugfs_known_mountpoints[] = {
 	NULL,
 };
 
+/* Return the current net namespace number. 0 is never a valid
+ * namespace, so use it to return that there is no name space
+ * support.
+ */
+
+static unsigned int debugfs_get_netns_inum(void)
+{
+	char net_path[] = "/proc/self/ns/net";
+	struct stat netst;
+	int netns;
+
+	netns = open(net_path, O_RDONLY);
+	if (netns < 0) {
+		if (errno == ENOENT)
+			/* Probably means no netns support in the kernel */
+			return 0;
+
+		fprintf(stderr,
+			"Error - can't open /proc/self/ns/net for read: %s\n",
+			strerror(errno));
+		return 0;
+	}
+
+	if (fstat(netns, &netst) < 0) {
+		fprintf(stderr, "Stat of netns failed: %s\n",
+			strerror(errno));
+		return 0;
+	}
+	close (netns);
+
+	return netst.st_ino;
+}
+
 /* construct a full path to a debugfs element */
 int debugfs_make_path(const char *fmt, const char *mesh_iface, char *buffer,
 		      int size)
 {
+	unsigned int ns =  debugfs_get_netns_inum();
+	char ns_dir[PATH_MAX];
+
 	if (strlen(debugfs_mountpoint) == 0) {
 		buffer[0] = '\0';
 		return -1;
 	}
 
-	return snprintf(buffer, size, fmt, debugfs_mountpoint, mesh_iface);
+	if (ns) {
+		snprintf(ns_dir, PATH_MAX, "netns/%u/", ns);
+		return snprintf(buffer, size, fmt, debugfs_mountpoint, ns_dir,
+				mesh_iface);
+	} else {
+		return snprintf(buffer, size, fmt, debugfs_mountpoint, "",
+				mesh_iface);
+	}
 }
 
 static int debugfs_found;
diff --git a/vis/vis.h b/vis/vis.h
index 969ca33..9e5dc35 100644
--- a/vis/vis.h
+++ b/vis/vis.h
@@ -37,7 +37,7 @@
 #define UPDATE_INTERVAL				10
 
 #define SYS_IFACE_PATH				"/sys/class/net"
-#define DEBUG_BATIF_PATH_FMT			"%s/batman_adv/%s"
+#define DEBUG_BATIF_PATH_FMT			"%s/batman_adv/%s%s"
 #define SYS_MESH_IFACE_FMT			SYS_IFACE_PATH"/%s/batman_adv/mesh_iface"
 #define SYS_IFACE_STATUS_FMT			SYS_IFACE_PATH"/%s/batman_adv/iface_status"
 
-- 
2.7.0.rc3

Re: [B.A.T.M.A.N.] [PATCH 2/3] alfred: Add support for network namespaces

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On Thursday 28 January 2016 04:53:14 Andrew Lunn wrote:
> When running within a network namespace, access to files within
> debugfs have to take into account the network name space. Each
> namespace has its own directory under
> /sys/kernel/debug/batman_adv/netns.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  batadv_query.c |  2 +-
>  debugfs.c      | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  vis/vis.h      |  2 +-
>  3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/batadv_query.c b/batadv_query.c
> index c289b80..fe7c007 100644
> --- a/batadv_query.c
> +++ b/batadv_query.c
> @@ -31,7 +31,7 @@
>  #include <sys/types.h>
>  #include "debugfs.h"
> 
> -#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s"
> +#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s%s"
>  #define DEBUG_TRANSTABLE_GLOBAL "transtable_global"
>  #define DEBUG_ORIGINATORS "originators"
> 
> diff --git a/debugfs.c b/debugfs.c
> index fc39322..35c1bdf 100644
> --- a/debugfs.c
> +++ b/debugfs.c
> @@ -20,11 +20,15 @@
> 
>  #include "debugfs.h"
>  #include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <sys/mount.h>
>  #include <sys/stat.h>
>  #include <sys/statfs.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> 
>  #ifndef DEBUGFS_MAGIC
>  #define DEBUGFS_MAGIC          0x64626720
> @@ -42,16 +46,59 @@ static const char *debugfs_known_mountpoints[] = {
>  	NULL,
>  };
> 
> +/* Return the current net namespace number. 0 is never a valid
> + * namespace, so use it to return that there is no name space
> + * support.
> + */
> +
> +static unsigned int debugfs_get_netns_inum(void)
> +{
> +	char net_path[] = "/proc/self/ns/net";
> +	struct stat netst;
> +	int netns;
> +
> +	netns = open(net_path, O_RDONLY);
> +	if (netns < 0) {
> +		if (errno == ENOENT)
> +			/* Probably means no netns support in the kernel */
> +			return 0;
> +
> +		fprintf(stderr,
> +			"Error - can't open /proc/self/ns/net for read: %s\n",
> +			strerror(errno));
> +		return 0;
> +	}
> +
> +	if (fstat(netns, &netst) < 0) {
> +		fprintf(stderr, "Stat of netns failed: %s\n",
> +			strerror(errno));
> +		return 0;

Aren't we leaking the netns file descriptor here? You could just set 
netst.st_ino = 0 instead ...

Rest looks good to me :)

Cheers,
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/3] alfred: Add support for network namespaces

[Andrew Lunn]

On Fri, Jan 29, 2016 at 01:18:13PM +0100, Simon Wunderlich wrote:
> On Thursday 28 January 2016 04:53:14 Andrew Lunn wrote:
> > When running within a network namespace, access to files within
> > debugfs have to take into account the network name space. Each
> > namespace has its own directory under
> > /sys/kernel/debug/batman_adv/netns.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  batadv_query.c |  2 +-
> >  debugfs.c      | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  vis/vis.h      |  2 +-
> >  3 files changed, 50 insertions(+), 3 deletions(-)
> > 
> > diff --git a/batadv_query.c b/batadv_query.c
> > index c289b80..fe7c007 100644
> > --- a/batadv_query.c
> > +++ b/batadv_query.c
> > @@ -31,7 +31,7 @@
> >  #include <sys/types.h>
> >  #include "debugfs.h"
> > 
> > -#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s"
> > +#define DEBUG_BATIF_PATH_FMT "%s/batman_adv/%s%s"
> >  #define DEBUG_TRANSTABLE_GLOBAL "transtable_global"
> >  #define DEBUG_ORIGINATORS "originators"
> > 
> > diff --git a/debugfs.c b/debugfs.c
> > index fc39322..35c1bdf 100644
> > --- a/debugfs.c
> > +++ b/debugfs.c
> > @@ -20,11 +20,15 @@
> > 
> >  #include "debugfs.h"
> >  #include <errno.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> >  #include <stdio.h>
> >  #include <string.h>
> >  #include <sys/mount.h>
> >  #include <sys/stat.h>
> >  #include <sys/statfs.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > 
> >  #ifndef DEBUGFS_MAGIC
> >  #define DEBUGFS_MAGIC          0x64626720
> > @@ -42,16 +46,59 @@ static const char *debugfs_known_mountpoints[] = {
> >  	NULL,
> >  };
> > 
> > +/* Return the current net namespace number. 0 is never a valid
> > + * namespace, so use it to return that there is no name space
> > + * support.
> > + */
> > +
> > +static unsigned int debugfs_get_netns_inum(void)
> > +{
> > +	char net_path[] = "/proc/self/ns/net";
> > +	struct stat netst;
> > +	int netns;
> > +
> > +	netns = open(net_path, O_RDONLY);
> > +	if (netns < 0) {
> > +		if (errno == ENOENT)
> > +			/* Probably means no netns support in the kernel */
> > +			return 0;
> > +
> > +		fprintf(stderr,
> > +			"Error - can't open /proc/self/ns/net for read: %s\n",
> > +			strerror(errno));
> > +		return 0;
> > +	}
> > +
> > +	if (fstat(netns, &netst) < 0) {
> > +		fprintf(stderr, "Stat of netns failed: %s\n",
> > +			strerror(errno));
> > +		return 0;
> 
> Aren't we leaking the netns file descriptor here?

Yes, it is leaking. I will fix that in a v2.

Thanks
     Andrew

[B.A.T.M.A.N.] [PATCH 3/3] alfred: Mount debugfs before reducing capabilities

[Andrew Lunn]

The debugfs helper code has the ability to mount the debugfs file
system if it is not already mounted. However, it cannot do this
after the capabilities have been dropped. So perform the mount early.

This is especially important when using network name spaces. Each
namespace has its own /sys, so the mount of debugfs in the global
namespace is not visible in other namespaces.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/main.c b/main.c
index 452d9ae..b1c5ec5 100644
--- a/main.c
+++ b/main.c
@@ -30,6 +30,7 @@
 #include <unistd.h>
 #endif
 #include "alfred.h"
+#include "debugfs.h"
 #include "packet.h"
 #include "list.h"
 
@@ -160,6 +161,9 @@ static struct globals *alfred_init(int argc, char *argv[])
 		{NULL,			0,			NULL,	0},
 	};
 
+	/* We need full capabilities to mount debugfs, so do that now */
+	debugfs_mount(NULL);
+
 	ret = reduce_capabilities();
 	if (ret < 0)
 		return NULL;
-- 
2.7.0.rc3

Re: [B.A.T.M.A.N.] [PATCH 3/3] alfred: Mount debugfs before reducing capabilities

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On Thursday 28 January 2016 04:53:15 Andrew Lunn wrote:
> The debugfs helper code has the ability to mount the debugfs file
> system if it is not already mounted. However, it cannot do this
> after the capabilities have been dropped. So perform the mount early.
> 
> This is especially important when using network name spaces. Each
> namespace has its own /sys, so the mount of debugfs in the global
> namespace is not visible in other namespaces.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/main.c b/main.c
> index 452d9ae..b1c5ec5 100644
> --- a/main.c
> +++ b/main.c
> @@ -30,6 +30,7 @@
>  #include <unistd.h>
>  #endif
>  #include "alfred.h"
> +#include "debugfs.h"
>  #include "packet.h"
>  #include "list.h"
> 
> @@ -160,6 +161,9 @@ static struct globals *alfred_init(int argc, char
> *argv[]) {NULL,			0,			NULL,	0},
>  	};
> 
> +	/* We need full capabilities to mount debugfs, so do that now */
> +	debugfs_mount(NULL);
> +
>  	ret = reduce_capabilities();
>  	if (ret < 0)
>  		return NULL;

Can't we remove the other calls to debugfs_mount() ? I see 3 more calls in 
alfred ...

Cheers,
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 3/3] alfred: Mount debugfs before reducing capabilities

[Andrew Lunn]

On Fri, Jan 29, 2016 at 01:14:53PM +0100, Simon Wunderlich wrote:
> On Thursday 28 January 2016 04:53:15 Andrew Lunn wrote:
> > The debugfs helper code has the ability to mount the debugfs file
> > system if it is not already mounted. However, it cannot do this
> > after the capabilities have been dropped. So perform the mount early.
> > 
> > This is especially important when using network name spaces. Each
> > namespace has its own /sys, so the mount of debugfs in the global
> > namespace is not visible in other namespaces.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/main.c b/main.c
> > index 452d9ae..b1c5ec5 100644
> > --- a/main.c
> > +++ b/main.c
> > @@ -30,6 +30,7 @@
> >  #include <unistd.h>
> >  #endif
> >  #include "alfred.h"
> > +#include "debugfs.h"
> >  #include "packet.h"
> >  #include "list.h"
> > 
> > @@ -160,6 +161,9 @@ static struct globals *alfred_init(int argc, char
> > *argv[]) {NULL,			0,			NULL,	0},
> >  	};
> > 
> > +	/* We need full capabilities to mount debugfs, so do that now */
> > +	debugfs_mount(NULL);
> > +
> >  	ret = reduce_capabilities();
> >  	if (ret < 0)
> >  		return NULL;
> 
> Can't we remove the other calls to debugfs_mount() ? I see 3 more calls in 
> alfred ...

Yes, the other calls within this binary are probably failing, due to
reduced capabilities. I can remove them in a v3 patch.

	Andrew

Re: [B.A.T.M.A.N.] [PATCH 3/3] alfred: Mount debugfs before reducing capabilities

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

Hi Andrew,

On Tuesday 02 February 2016 03:25:12 Andrew Lunn wrote:
> On Fri, Jan 29, 2016 at 01:14:53PM +0100, Simon Wunderlich wrote:
> > On Thursday 28 January 2016 04:53:15 Andrew Lunn wrote:
> [...]
> > > @@ -160,6 +161,9 @@ static struct globals *alfred_init(int argc, char
> > > *argv[]) {NULL,			0,			NULL,	0},
> > > 
> > >  	};
> > > 
> > > +	/* We need full capabilities to mount debugfs, so do that now */
> > > +	debugfs_mount(NULL);
> > > +
> > > 
> > >  	ret = reduce_capabilities();
> > >  	if (ret < 0)
> > >  	
> > >  		return NULL;
> > 
> > Can't we remove the other calls to debugfs_mount() ? I see 3 more calls in
> > alfred ...
> 
> Yes, the other calls within this binary are probably failing, due to
> reduced capabilities. I can remove them in a v3 patch.
> 
> 	Andrew

Are you still planning on sending a fixed patchset? :)

Thanks!
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 3/3] alfred: Mount debugfs before reducing capabilities

[Andrew Lunn]

On Thu, Mar 10, 2016 at 04:09:18PM +0100, Simon Wunderlich wrote:
> Hi Andrew,
> 
> On Tuesday 02 February 2016 03:25:12 Andrew Lunn wrote:
> > On Fri, Jan 29, 2016 at 01:14:53PM +0100, Simon Wunderlich wrote:
> > > On Thursday 28 January 2016 04:53:15 Andrew Lunn wrote:
> > [...]
> > > > @@ -160,6 +161,9 @@ static struct globals *alfred_init(int argc, char
> > > > *argv[]) {NULL,			0,			NULL,	0},
> > > > 
> > > >  	};
> > > > 
> > > > +	/* We need full capabilities to mount debugfs, so do that now */
> > > > +	debugfs_mount(NULL);
> > > > +
> > > > 
> > > >  	ret = reduce_capabilities();
> > > >  	if (ret < 0)
> > > >  	
> > > >  		return NULL;
> > > 
> > > Can't we remove the other calls to debugfs_mount() ? I see 3 more calls in
> > > alfred ...
> > 
> > Yes, the other calls within this binary are probably failing, due to
> > reduced capabilities. I can remove them in a v3 patch.
> > 
> > 	Andrew
> 
> Are you still planning on sending a fixed patchset? :)

Hi Simon

I have a fixed up version. But at the moment, it is not clear what is
happening with the kernel patches. Maybe we need to change the debugfs
API in order to make it easier to build with older kernels?

    Andrew

Re: [B.A.T.M.A.N.] [PATCH 1/3] alfred: vis: Use debugfs_make_path helper

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 281 bytes --]

On Thursday 28 January 2016 04:53:13 Andrew Lunn wrote:
> By using the helper, knowledge of network namespaces can be kept to
> within the helper.

I've applied this one, since its a good change even without the namespace 
changes.

Applied in revision dca3ebf

Thanks!
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]