[v3,6/6] batman-adv: debugfs: Add netns support

Message ID 1457895034-13823-6-git-send-email-sven@narfation.org (mailing list archive)
State Changes Requested, archived
Delegated to: Marek Lindner
Headers

Commit Message

Sven Eckelmann March 13, 2016, 6:50 p.m. UTC
  From: Andrew Lunn <andrew@lunn.ch>

Unlike sysfs, debugfs is not netns aware. So batman has to take care
to avoid namespace clashes.

Each namespace is given a directory within debugfs/batman-adv/netns,
using the namespaces inum as the directory name.

Files for namespaces other than the global namespace are placed within
the namespace specific directory. Additionally, a symbolic link is
used to link the global namespaces inum back to debugfs/batman-adv/ so
tools do not need to differentiate between the global namespace and
other namespaces.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 compat-patches/0002-ns.inum.patch |  25 ++++++++
 net/batman-adv/debugfs.c          | 130 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 152 insertions(+), 3 deletions(-)
 create mode 100644 compat-patches/0002-ns.inum.patch
  

Comments

Sven Eckelmann March 14, 2016, 10:57 a.m. UTC | #1
On Sunday 13 March 2016 19:50:34 Sven Eckelmann wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> Unlike sysfs, debugfs is not netns aware. So batman has to take care
> to avoid namespace clashes.
> 
> Each namespace is given a directory within debugfs/batman-adv/netns,
> using the namespaces inum as the directory name.
> 
> Files for namespaces other than the global namespace are placed within
> the namespace specific directory. Additionally, a symbolic link is
> used to link the global namespaces inum back to debugfs/batman-adv/ so
> tools do not need to differentiate between the global namespace and
> other namespaces.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  compat-patches/0002-ns.inum.patch |  25 ++++++++
>  net/batman-adv/debugfs.c          | 130 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 152 insertions(+), 3 deletions(-)
>  create mode 100644 compat-patches/0002-ns.inum.patch

Matthias is against applying [1] patch 6 because it would require that
batman-adv supports this interface for a long(tm) time. He proposes to use
netlink instead.

Andrew, do you want to change patch 6 to avoid creating the debugfs stuff in
non-init_net netns?

Kind regards,
	Sven

[1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2016-March/014690.html
  
Andrew Lunn March 14, 2016, 2:06 p.m. UTC | #2
> Matthias is against applying [1] patch 6 because it would require that
> batman-adv supports this interface for a long(tm) time. He proposes to use
> netlink instead.
> 
> Andrew, do you want to change patch 6 to avoid creating the debugfs stuff in
> non-init_net netns?

I'm not sure how well that is going to work, from the user space side
of things.

In the kernel, we know if we are in init_net, or some other netns.

In user space, it is not so simple. Ideally, when in some other netns
than the default, we need all reads/writes to debugfs to fail. What we
don't want is it seeing the default name spaces files, because it is
going to get very confusing. These files refer to something which does
not exist in the current netns.

The problem is, how do we find out if we are in a different netns than
default?

	Andrew
  
Sven Eckelmann March 14, 2016, 3:56 p.m. UTC | #3
On Monday 14 March 2016 15:06:24 Andrew Lunn wrote:
> > Matthias is against applying [1] patch 6 because it would require that
> > batman-adv supports this interface for a long(tm) time. He proposes to use
> > netlink instead.
> > 
> > Andrew, do you want to change patch 6 to avoid creating the debugfs stuff in
> > non-init_net netns?
> 
> I'm not sure how well that is going to work, from the user space side
> of things.
> 
> In the kernel, we know if we are in init_net, or some other netns.
> 
> In user space, it is not so simple. Ideally, when in some other netns
> than the default, we need all reads/writes to debugfs to fail.

Hm, this would require some checks via the the current pid on open:

    net = get_net_ns_by_pid(current->pid);
    ... checky check via neteq(net, &init_net)...
    put_net(pd->net);

> What we
> don't want is it seeing the default name spaces files, because it is
> going to get very confusing. These files refer to something which does
> not exist in the current netns.

Wait, but thats exactly what you are doing already with your default behavior
(which only creates a symlink to netns/${FUNNY_ID}/. Legacy tools will still
read the wrong information because they don't know about the new netns paths.

I thought that the debugfs stuff will be replaced with netlink  and that the
current debugfs files are only there for non-namespace setups with legacy tools.

Kind regards,
	Sven

[1] https://patchwork.open-mesh.org/patch/4965/
  
Andrew Lunn March 14, 2016, 7:20 p.m. UTC | #4
On Mon, Mar 14, 2016 at 04:56:57PM +0100, Sven Eckelmann wrote:
> On Monday 14 March 2016 15:06:24 Andrew Lunn wrote:
> > > Matthias is against applying [1] patch 6 because it would require that
> > > batman-adv supports this interface for a long(tm) time. He proposes to use
> > > netlink instead.
> > > 
> > > Andrew, do you want to change patch 6 to avoid creating the debugfs stuff in
> > > non-init_net netns?
> > 
> > I'm not sure how well that is going to work, from the user space side
> > of things.
> > 
> > In the kernel, we know if we are in init_net, or some other netns.
> > 
> > In user space, it is not so simple. Ideally, when in some other netns
> > than the default, we need all reads/writes to debugfs to fail.
> 
> Hm, this would require some checks via the the current pid on open:
> 
>     net = get_net_ns_by_pid(current->pid);
>     ... checky check via neteq(net, &init_net)...
>     put_net(pd->net);

I suppose this could be made to work. Not sure what error code to
return, maybe ENXIO if batctl is not in the default netns.
 
> > What we
> > don't want is it seeing the default name spaces files, because it is
> > going to get very confusing. These files refer to something which does
> > not exist in the current netns.
> 
> Wait, but thats exactly what you are doing already with your default behavior
> (which only creates a symlink to netns/${FUNNY_ID}/. Legacy tools will still
> read the wrong information because they don't know about the new netns paths.

Correct. And this is unfixable, as far as i can see. You need the
contents of debugfs to be dependent on the observer. The requires core
debugfs support to calls like readdir() and open().

> I thought that the debugfs stuff will be replaced with netlink  and that the
> current debugfs files are only there for non-namespace setups with legacy tools.

Legacy tools are always going to be broken when they are used in the
non-default netns. Probably the best we can do is have the kernel
return ENXIO or whatever when they access files from a different
netns.

We have two options for non-default netns debugfs

1) Extend debugfs and the tools as i suggested patches for.

2) Only support default netns in debugfs, and use netlink for full
   netns aware tools.

   Andrew
  

Patch

diff --git a/compat-patches/0002-ns.inum.patch b/compat-patches/0002-ns.inum.patch
new file mode 100644
index 0000000..9856f6e
--- /dev/null
+++ b/compat-patches/0002-ns.inum.patch
@@ -0,0 +1,25 @@ 
+From: Sven Eckelmann <sven@narfation.org>
+Date: Sat, 12 Mar 2016 00:24:58 +0100
+Subject: [PATCH] ns.inum
+---
+ net/batman-adv/debugfs.c | 6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/net/batman-adv/debugfs.c b/net/batman-adv/debugfs.c
+index cc583e6..59616a6 100644
+--- a/net/batman-adv/debugfs.c
++++ b/net/batman-adv/debugfs.c
+@@ -78,7 +78,13 @@ static DEFINE_MUTEX(batadv_debugfs_ns_mutex);
+  */
+ static unsigned int batadv_debugfs_ns_id(const struct net *net)
+ {
++#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 8, 0)
++	return 0; /* ???? */
++#elif LINUX_VERSION_CODE < KERNEL_VERSION(3, 19, 0)
++	return net->proc_inum;
++#else
+ 	return net->ns.inum;
++#endif
+ }
+ 
+ static struct dentry *batadv_debugfs_ns_get(struct net *net)
diff --git a/net/batman-adv/debugfs.c b/net/batman-adv/debugfs.c
index 3dc5208..cc583e6 100644
--- a/net/batman-adv/debugfs.c
+++ b/net/batman-adv/debugfs.c
@@ -27,8 +27,12 @@ 
 #include <linux/fs.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/netdevice.h>
+#include <linux/ns_common.h>
 #include <linux/poll.h>
 #include <linux/printk.h>
 #include <linux/sched.h> /* for linux/wait.h */
@@ -42,6 +46,7 @@ 
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/wait.h>
+#include <net/net_namespace.h>
 #include <stdarg.h>
 
 #include "bridge_loop_avoidance.h"
@@ -53,6 +58,84 @@ 
 #include "translation-table.h"
 
 static struct dentry *batadv_debugfs;
+static struct dentry *batadv_ns_debugfs;
+
+struct batadv_debugfs_ns_entry {
+	struct net *net;
+	struct dentry *dir;
+	struct kref refcount;
+	struct list_head link;
+};
+
+static LIST_HEAD(batadv_debugfs_ns);
+static DEFINE_MUTEX(batadv_debugfs_ns_mutex);
+
+/**
+ * batadv_debugfs_ns_id - Get namespace specific id
+ * @net: namespace of the soft-interface
+ *
+ * Return: internal id of the namespace
+ */
+static unsigned int batadv_debugfs_ns_id(const struct net *net)
+{
+	return net->ns.inum;
+}
+
+static struct dentry *batadv_debugfs_ns_get(struct net *net)
+{
+	struct batadv_debugfs_ns_entry *ns_entry;
+	char name[32];
+
+	mutex_lock(&batadv_debugfs_ns_mutex);
+	list_for_each_entry(ns_entry, &batadv_debugfs_ns, link) {
+		if (ns_entry->net == net) {
+			kref_get(&ns_entry->refcount);
+			mutex_unlock(&batadv_debugfs_ns_mutex);
+			return ns_entry->dir;
+		}
+	}
+
+	ns_entry = kzalloc(sizeof(*ns_entry), GFP_ATOMIC);
+	if (ns_entry) {
+		INIT_LIST_HEAD(&ns_entry->link);
+		ns_entry->net = net;
+		kref_init(&ns_entry->refcount);
+		sprintf(name, "%u", batadv_debugfs_ns_id(net));
+		ns_entry->dir = debugfs_create_dir(name, batadv_ns_debugfs);
+		if (!ns_entry->dir) {
+			kfree(ns_entry);
+			mutex_unlock(&batadv_debugfs_ns_mutex);
+			return NULL;
+		}
+		list_add(&ns_entry->link, &batadv_debugfs_ns);
+	}
+	mutex_unlock(&batadv_debugfs_ns_mutex);
+	return ns_entry->dir;
+}
+
+static void batadv_ns_entry_release(struct kref *ref)
+{
+	struct batadv_debugfs_ns_entry *ns_entry;
+
+	ns_entry = container_of(ref, struct batadv_debugfs_ns_entry, refcount);
+	debugfs_remove_recursive(ns_entry->dir);
+	list_del(&ns_entry->link);
+	kfree(ns_entry);
+}
+
+static void batadv_debugfs_ns_put(struct net *net)
+{
+	struct batadv_debugfs_ns_entry *ns_entry;
+
+	mutex_lock(&batadv_debugfs_ns_mutex);
+	list_for_each_entry(ns_entry, &batadv_debugfs_ns, link) {
+		if (ns_entry->net == net) {
+			kref_put(&ns_entry->refcount, batadv_ns_entry_release);
+			break;
+		}
+	}
+	mutex_unlock(&batadv_debugfs_ns_mutex);
+}
 
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 #define BATADV_LOG_BUFF_MASK (batadv_log_buff_len - 1)
@@ -451,6 +534,7 @@  void batadv_debugfs_init(void)
 {
 	struct batadv_debuginfo **bat_debug;
 	struct dentry *file;
+	char name[32];
 
 	batadv_debugfs = debugfs_create_dir(BATADV_DEBUGFS_SUBDIR, NULL);
 	if (batadv_debugfs == ERR_PTR(-ENODEV))
@@ -471,6 +555,15 @@  void batadv_debugfs_init(void)
 		}
 	}
 
+	batadv_ns_debugfs = debugfs_create_dir("netns", batadv_debugfs);
+	if (!batadv_ns_debugfs)
+		goto err;
+
+	/* Create a symlink for the default name space */
+	sprintf(name, "%u", batadv_debugfs_ns_id(&init_net));
+	if (!debugfs_create_symlink(name, batadv_ns_debugfs, ".."))
+		goto err;
+
 	return;
 err:
 	debugfs_remove_recursive(batadv_debugfs);
@@ -492,14 +585,24 @@  void batadv_debugfs_destroy(void)
  */
 int batadv_debugfs_add_hardif(struct batadv_hard_iface *hard_iface)
 {
+	struct net *net = dev_net(hard_iface->net_dev);
+	char *name = hard_iface->net_dev->name;
 	struct batadv_debuginfo **bat_debug;
+	struct dentry *debugfs_ns_dir;
 	struct dentry *file;
 
 	if (!batadv_debugfs)
 		goto out;
 
-	hard_iface->debug_dir = debugfs_create_dir(hard_iface->net_dev->name,
-						   batadv_debugfs);
+	debugfs_ns_dir = batadv_debugfs;
+
+	if (net != &init_net) {
+		debugfs_ns_dir = batadv_debugfs_ns_get(net);
+		if (!debugfs_ns_dir)
+			goto out;
+	}
+
+	hard_iface->debug_dir = debugfs_create_dir(name, debugfs_ns_dir);
 	if (!hard_iface->debug_dir)
 		goto out;
 
@@ -517,6 +620,8 @@  int batadv_debugfs_add_hardif(struct batadv_hard_iface *hard_iface)
 rem_attr:
 	debugfs_remove_recursive(hard_iface->debug_dir);
 	hard_iface->debug_dir = NULL;
+	if (net != &init_net)
+		batadv_debugfs_ns_put(net);
 out:
 	return -ENOMEM;
 }
@@ -528,22 +633,36 @@  out:
  */
 void batadv_debugfs_del_hardif(struct batadv_hard_iface *hard_iface)
 {
+	struct net *net = dev_net(hard_iface->net_dev);
+
 	if (batadv_debugfs) {
 		debugfs_remove_recursive(hard_iface->debug_dir);
 		hard_iface->debug_dir = NULL;
 	}
+	if (net != &init_net)
+		batadv_debugfs_ns_put(net);
 }
 
 int batadv_debugfs_add_meshif(struct net_device *dev)
 {
 	struct batadv_priv *bat_priv = netdev_priv(dev);
 	struct batadv_debuginfo **bat_debug;
+	struct net *net = dev_net(dev);
+	struct dentry *debugfs_ns_dir;
 	struct dentry *file;
 
 	if (!batadv_debugfs)
 		goto out;
 
-	bat_priv->debug_dir = debugfs_create_dir(dev->name, batadv_debugfs);
+	debugfs_ns_dir = batadv_debugfs;
+
+	if (net != &init_net) {
+		debugfs_ns_dir = batadv_debugfs_ns_get(net);
+		if (!debugfs_ns_dir)
+			goto out;
+	}
+
+	bat_priv->debug_dir = debugfs_create_dir(dev->name, debugfs_ns_dir);
 	if (!bat_priv->debug_dir)
 		goto out;
 
@@ -572,6 +691,8 @@  int batadv_debugfs_add_meshif(struct net_device *dev)
 rem_attr:
 	debugfs_remove_recursive(bat_priv->debug_dir);
 	bat_priv->debug_dir = NULL;
+	if (net != &init_net)
+		batadv_debugfs_ns_put(net);
 out:
 	return -ENOMEM;
 }
@@ -579,6 +700,7 @@  out:
 void batadv_debugfs_del_meshif(struct net_device *dev)
 {
 	struct batadv_priv *bat_priv = netdev_priv(dev);
+	struct net *net = dev_net(dev);
 
 	batadv_debug_log_cleanup(bat_priv);
 
@@ -586,4 +708,6 @@  void batadv_debugfs_del_meshif(struct net_device *dev)
 		debugfs_remove_recursive(bat_priv->debug_dir);
 		bat_priv->debug_dir = NULL;
 	}
+	if (net != &init_net)
+		batadv_debugfs_ns_put(net);
 }