[B.A.T.M.A.N.] [PATCH v2 0/2] netns support for alfred

[B.A.T.M.A.N.] [PATCH v2 0/2] netns support for alfred

[Andrew Lunn]

Dropped patch #1, since it has been merged
Fixed the socket leak
Removed redundant calls to mount debugfs 

Andrew Lunn (2):
  alfred: Add support for network namespaces
  alfred: Mount debugfs before reducing capabilities

 batadv_query.c | 19 +------------------
 debugfs.c      | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 main.c         |  4 ++++
 vis/vis.h      |  2 +-
 4 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.7.0.rc3

[B.A.T.M.A.N.] [PATCH v2 1/2] 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>
---
v2: Fix file descriptor leak
---
 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 2604503..1b9e631 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..3ee2cbd 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));
+		netst.st_ino = 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 37e3164..2ccc6cb 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

[B.A.T.M.A.N.] [PATCH v2 2/2] 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,
and remove all the other attempts to do the mount.

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>
---
v2: Remove the other calls to mount
---
 batadv_query.c | 17 -----------------
 main.c         |  4 ++++
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/batadv_query.c b/batadv_query.c
index 1b9e631..1a385c9 100644
--- a/batadv_query.c
+++ b/batadv_query.c
@@ -90,16 +90,9 @@ int ipv6_to_mac(const struct in6_addr *addr, struct ether_addr *mac)
 
 int batadv_interface_check(const char *mesh_iface)
 {
-	char *debugfs_mnt;
 	char full_path[MAX_PATH + 1];
 	FILE *f;
 
-	debugfs_mnt = debugfs_mount(NULL);
-	if (!debugfs_mnt) {
-		fprintf(stderr, "Could not find debugfs path\n");
-		return -1;
-	}
-
 	debugfs_make_path(DEBUG_BATIF_PATH_FMT "/" DEBUG_TRANSTABLE_GLOBAL,
 			  mesh_iface, full_path, sizeof(full_path));
 	f = fopen(full_path, "r");
@@ -134,7 +127,6 @@ struct ether_addr *translate_mac(const char *mesh_iface, struct ether_addr *mac)
 		tg_originator,
 	} pos;
 	char full_path[MAX_PATH + 1];
-	char *debugfs_mnt;
 	static struct ether_addr in_mac;
 	struct ether_addr *mac_result, *mac_tmp;
 	FILE *f = NULL;
@@ -146,10 +138,6 @@ struct ether_addr *translate_mac(const char *mesh_iface, struct ether_addr *mac)
 	memcpy(&in_mac, mac, sizeof(in_mac));
 	mac_result = &in_mac;
 
-	debugfs_mnt = debugfs_mount(NULL);
-	if (!debugfs_mnt)
-		goto out;
-
 	debugfs_make_path(DEBUG_BATIF_PATH_FMT "/" DEBUG_TRANSTABLE_GLOBAL,
 			  mesh_iface, full_path, sizeof(full_path));
 
@@ -216,7 +204,6 @@ uint8_t get_tq(const char *mesh_iface, struct ether_addr *mac)
 		orig_tqvalue,
 	} pos;
 	char full_path[MAX_PATH + 1];
-	char *debugfs_mnt;
 	static struct ether_addr in_mac;
 	struct ether_addr *mac_tmp;
 	FILE *f = NULL;
@@ -228,10 +215,6 @@ uint8_t get_tq(const char *mesh_iface, struct ether_addr *mac)
 
 	memcpy(&in_mac, mac, sizeof(in_mac));
 
-	debugfs_mnt = debugfs_mount(NULL);
-	if (!debugfs_mnt)
-		goto out;
-
 	debugfs_make_path(DEBUG_BATIF_PATH_FMT "/" DEBUG_ORIGINATORS,
 			  mesh_iface, full_path, sizeof(full_path));
 
diff --git a/main.c b/main.c
index 9610398..52dca97 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 v2 2/2] alfred: Mount debugfs before reducing capabilities

[Sven Eckelmann]

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

On Wednesday 16 March 2016 20:51:12 Andrew Lunn wrote:
> --- a/batadv_query.c
> +++ b/batadv_query.c
> @@ -90,16 +90,9 @@ int ipv6_to_mac(const struct in6_addr *addr, struct ether_addr *mac)
>  
>  int batadv_interface_check(const char *mesh_iface)
>  {
> -	char *debugfs_mnt;
>  	char full_path[MAX_PATH + 1];
>  	FILE *f;
>  
> -	debugfs_mnt = debugfs_mount(NULL);
> -	if (!debugfs_mnt) {
> -		fprintf(stderr, "Could not find debugfs path\n");
> -		return -1;
> -	}
> -
[...]
> diff --git a/main.c b/main.c
> index 9610398..52dca97 100644
> --- a/main.c
> +++ b/main.c
[...]
> @@ -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;
> 

There were some return value checks for the debugfs_mount calls which you've
removed. Why isn't here some kind of check with error handling/warning, ...?

Kind regards,
	Sven

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

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

[Andrew Lunn]

On Fri, Mar 18, 2016 at 12:27:38PM +0100, Sven Eckelmann wrote:
> On Wednesday 16 March 2016 20:51:12 Andrew Lunn wrote:
> > --- a/batadv_query.c
> > +++ b/batadv_query.c
> > @@ -90,16 +90,9 @@ int ipv6_to_mac(const struct in6_addr *addr, struct ether_addr *mac)
> >  
> >  int batadv_interface_check(const char *mesh_iface)
> >  {
> > -	char *debugfs_mnt;
> >  	char full_path[MAX_PATH + 1];
> >  	FILE *f;
> >  
> > -	debugfs_mnt = debugfs_mount(NULL);
> > -	if (!debugfs_mnt) {
> > -		fprintf(stderr, "Could not find debugfs path\n");
> > -		return -1;
> > -	}
> > -
> [...]
> > diff --git a/main.c b/main.c
> > index 9610398..52dca97 100644
> > --- a/main.c
> > +++ b/main.c
> [...]
> > @@ -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;
> > 
> 
> There were some return value checks for the debugfs_mount calls which you've
> removed. Why isn't here some kind of check with error handling/warning, ...?

Hi Sven

I think the other times debugfs mount was called was just before it
was used. So a mount failure means the code that follows is going to
fail.

Putting the mount very early in the program means it could be about to
do something which does not require debugfs. e.g. run in client
mode. So the most we can do a print a warning, something like: "No
access to debugfs. Some functionality may not work".

The other option is a bigger change. Move the calls to
reduce_capabilities() and debugfs_mount() to later, after the option
parsing. We then have a better idea if debugfs is needed or not, and
can then print a fatal error.

	Andrew
 

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

[Simon Wunderlich]

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

On Wednesday 16 March 2016 20:51:12 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,
> and remove all the other attempts to do the mount.
> 
> 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>
> ---
> v2: Remove the other calls to mount

Applied in revision d08b0e0.

Thanks!
     Simon

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