[patch net-next v2 0/3] net: devlink: Finish network namespace support

[patch net-next v2 0/3] net: devlink: Finish network namespace support

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Devlink from the beginning counts with network namespaces, but the
instances has been fixed to init_net. The first patch allows user
to move existing devlink instances into namespaces:

$ devlink dev
netdevsim/netdevsim1
$ ip netns add ns1
$ devlink dev set netdevsim/netdevsim1 netns ns1
$ devlink -N ns1 dev
netdevsim/netdevsim1

The last patch allows user to create new netdevsim instance directly
inside network namespace of a caller.

Jiri Pirko (3):
  net: devlink: allow to change namespaces
  net: devlink: export devlink net set/get helpers
  netdevsim: create devlink and netdev instances in namespace

 drivers/net/netdevsim/bus.c       |   1 +
 drivers/net/netdevsim/dev.c       |  17 ++--
 drivers/net/netdevsim/netdev.c    |   4 +-
 drivers/net/netdevsim/netdevsim.h |   8 +-
 include/net/devlink.h             |   3 +
 include/uapi/linux/devlink.h      |   4 +
 net/core/devlink.c                | 129 ++++++++++++++++++++++++++++--
 7 files changed, 152 insertions(+), 14 deletions(-)

-- 
2.21.0

[patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

All devlink instances are created in init_net and stay there for a
lifetime. Allow user to be able to move devlink instances into
namespaces.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- change the check for multiple attributes
- add warnon in case there is no attribute passed
---
 include/uapi/linux/devlink.h |   4 ++
 net/core/devlink.c           | 113 ++++++++++++++++++++++++++++++++++-
 2 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index ffc993256527..95f0a1edab99 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -348,6 +348,10 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
 	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
 
+	DEVLINK_ATTR_NETNS_FD,			/* u32 */
+	DEVLINK_ATTR_NETNS_PID,			/* u32 */
+	DEVLINK_ATTR_NETNS_ID,			/* u32 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4f40aeace902..e1cbfd90f788 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -439,8 +439,16 @@ static void devlink_nl_post_doit(const struct genl_ops *ops,
 {
 	struct devlink *devlink;
 
-	devlink = devlink_get_from_info(info);
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
+	/* When devlink changes netns, it would not be found
+	 * by devlink_get_from_info(). So try if it is stored first.
+	 */
+	if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_DEVLINK) {
+		devlink = info->user_ptr[0];
+	} else {
+		devlink = devlink_get_from_info(info);
+		WARN_ON(IS_ERR(devlink));
+	}
+	if (!IS_ERR(devlink) && ~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
 		mutex_unlock(&devlink->lock);
 	mutex_unlock(&devlink_mutex);
 }
@@ -645,6 +653,71 @@ static int devlink_nl_cmd_get_doit(struct sk_buff *skb, struct genl_info *info)
 	return genlmsg_reply(msg, info);
 }
 
+static struct net *devlink_netns_get(struct sk_buff *skb,
+				     struct devlink *devlink,
+				     struct genl_info *info)
+{
+	struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
+	struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
+	struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
+	struct net *net;
+
+	if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
+		NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (netns_pid_attr) {
+		net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
+	} else if (netns_fd_attr) {
+		net = get_net_ns_by_fd(nla_get_u32(netns_fd_attr));
+	} else if (netns_id_attr) {
+		net = get_net_ns_by_id(sock_net(skb->sk),
+				       nla_get_u32(netns_id_attr));
+		if (!net)
+			net = ERR_PTR(-EINVAL);
+	} else {
+		WARN_ON(1);
+		net = ERR_PTR(-EINVAL);
+	}
+	if (IS_ERR(net)) {
+		NL_SET_ERR_MSG(info->extack, "Unknown network namespace");
+		return ERR_PTR(-EINVAL);
+	}
+	if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
+		put_net(net);
+		return ERR_PTR(-EPERM);
+	}
+	return net;
+}
+
+static void devlink_netns_change(struct devlink *devlink, struct net *net)
+{
+	if (net_eq(devlink_net(devlink), net))
+		return;
+	devlink_notify(devlink, DEVLINK_CMD_DEL);
+	devlink_net_set(devlink, net);
+	devlink_notify(devlink, DEVLINK_CMD_NEW);
+}
+
+static int devlink_nl_cmd_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+
+	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
+	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
+	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
+		struct net *net;
+
+		net = devlink_netns_get(skb, devlink, info);
+		if (IS_ERR(net))
+			return PTR_ERR(net);
+		devlink_netns_change(devlink, net);
+		put_net(net);
+	}
+	return 0;
+}
+
 static int devlink_nl_cmd_get_dumpit(struct sk_buff *msg,
 				     struct netlink_callback *cb)
 {
@@ -5184,6 +5257,9 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_NETNS_PID] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NETNS_FD] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NETNS_ID] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -5195,6 +5271,13 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 		/* can be retrieved by unprivileged users */
 	},
+	{
+		.cmd = DEVLINK_CMD_SET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = devlink_nl_cmd_set_doit,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
 	{
 		.cmd = DEVLINK_CMD_PORT_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6955,9 +7038,33 @@ int devlink_compat_switch_id_get(struct net_device *dev,
 	return 0;
 }
 
+static void __net_exit devlink_pernet_exit(struct net *net)
+{
+	struct devlink *devlink;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list)
+		if (net_eq(devlink_net(devlink), net))
+			devlink_netns_change(devlink, &init_net);
+	mutex_unlock(&devlink_mutex);
+}
+
+static struct pernet_operations __net_initdata devlink_pernet_ops = {
+	.exit = devlink_pernet_exit,
+};
+
 static int __init devlink_init(void)
 {
-	return genl_register_family(&devlink_nl_family);
+	int err;
+
+	err = genl_register_family(&devlink_nl_family);
+	if (err)
+		goto out;
+	err = register_pernet_device(&devlink_pernet_ops);
+
+out:
+	WARN_ON(err);
+	return err;
 }
 
 subsys_initcall(devlink_init);
-- 
2.21.0

[patch net-next v2 2/3] net: devlink: export devlink net set/get helpers

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Allow drivers to set/get net struct for devlink instance. Set is only
allowed for newly allocated devlink instance.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/devlink.h |  3 +++
 net/core/devlink.c    | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index bc36f942a7d5..98b89eabd73a 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -35,6 +35,7 @@ struct devlink {
 	struct device *dev;
 	possible_net_t _net;
 	struct mutex lock;
+	bool registered;
 	char priv[0] __aligned(NETDEV_ALIGN);
 };
 
@@ -591,6 +592,8 @@ static inline struct devlink *netdev_to_devlink(struct net_device *dev)
 
 struct ib_device;
 
+struct net *devlink_net(const struct devlink *devlink);
+void devlink_net_set(struct devlink *devlink, struct net *net);
 struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size);
 int devlink_register(struct devlink *devlink, struct device *dev);
 void devlink_unregister(struct devlink *devlink);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e1cbfd90f788..fc364bdb9cf2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -92,16 +92,25 @@ static LIST_HEAD(devlink_list);
  */
 static DEFINE_MUTEX(devlink_mutex);
 
-static struct net *devlink_net(const struct devlink *devlink)
+struct net *devlink_net(const struct devlink *devlink)
 {
 	return read_pnet(&devlink->_net);
 }
+EXPORT_SYMBOL_GPL(devlink_net);
 
-static void devlink_net_set(struct devlink *devlink, struct net *net)
+static void __devlink_net_set(struct devlink *devlink, struct net *net)
 {
 	write_pnet(&devlink->_net, net);
 }
 
+void devlink_net_set(struct devlink *devlink, struct net *net)
+{
+	if (WARN_ON(devlink->registered))
+		return;
+	__devlink_net_set(devlink, net);
+}
+EXPORT_SYMBOL_GPL(devlink_net_set);
+
 static struct devlink *devlink_get_from_attrs(struct net *net,
 					      struct nlattr **attrs)
 {
@@ -696,7 +705,7 @@ static void devlink_netns_change(struct devlink *devlink, struct net *net)
 	if (net_eq(devlink_net(devlink), net))
 		return;
 	devlink_notify(devlink, DEVLINK_CMD_DEL);
-	devlink_net_set(devlink, net);
+	__devlink_net_set(devlink, net);
 	devlink_notify(devlink, DEVLINK_CMD_NEW);
 }
 
@@ -5603,7 +5612,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
 	if (!devlink)
 		return NULL;
 	devlink->ops = ops;
-	devlink_net_set(devlink, &init_net);
+	__devlink_net_set(devlink, &init_net);
 	INIT_LIST_HEAD(&devlink->port_list);
 	INIT_LIST_HEAD(&devlink->sb_list);
 	INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list);
@@ -5627,6 +5636,7 @@ int devlink_register(struct devlink *devlink, struct device *dev)
 {
 	mutex_lock(&devlink_mutex);
 	devlink->dev = dev;
+	devlink->registered = true;
 	list_add_tail(&devlink->list, &devlink_list);
 	devlink_notify(devlink, DEVLINK_CMD_NEW);
 	mutex_unlock(&devlink_mutex);
-- 
2.21.0

[patch net-next v2 3/3] netdevsim: create devlink and netdev instances in namespace

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

When user does create new netdevsim instance using sysfs bus file,
create the devlink instance and related netdev instance in the namespace
of the caller.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- remove net_namespace.h include and forward decralared net struct
- add comment to initial_net pointer
---
 drivers/net/netdevsim/bus.c       |  1 +
 drivers/net/netdevsim/dev.c       | 17 +++++++++++------
 drivers/net/netdevsim/netdev.c    |  4 +++-
 drivers/net/netdevsim/netdevsim.h |  8 +++++++-
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 1a0ff3d7747b..6aeed0c600f8 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -283,6 +283,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
 	nsim_bus_dev->dev.bus = &nsim_bus;
 	nsim_bus_dev->dev.type = &nsim_bus_dev_type;
 	nsim_bus_dev->port_count = port_count;
+	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
 
 	err = device_register(&nsim_bus_dev->dev);
 	if (err)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index c5c417a3c0ce..685dd21f5500 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -268,7 +268,8 @@ static const struct devlink_ops nsim_dev_devlink_ops = {
 };
 
 static struct nsim_dev *
-nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev, unsigned int port_count)
+nsim_dev_create(struct net *net, struct nsim_bus_dev *nsim_bus_dev,
+		unsigned int port_count)
 {
 	struct nsim_dev *nsim_dev;
 	struct devlink *devlink;
@@ -277,6 +278,7 @@ nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev, unsigned int port_count)
 	devlink = devlink_alloc(&nsim_dev_devlink_ops, sizeof(*nsim_dev));
 	if (!devlink)
 		return ERR_PTR(-ENOMEM);
+	devlink_net_set(devlink, net);
 	nsim_dev = devlink_priv(devlink);
 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
 	nsim_dev->switch_id.id_len = sizeof(nsim_dev->switch_id.id);
@@ -335,7 +337,7 @@ static void nsim_dev_destroy(struct nsim_dev *nsim_dev)
 	devlink_free(devlink);
 }
 
-static int __nsim_dev_port_add(struct nsim_dev *nsim_dev,
+static int __nsim_dev_port_add(struct net *net, struct nsim_dev *nsim_dev,
 			       unsigned int port_index)
 {
 	struct nsim_dev_port *nsim_dev_port;
@@ -361,7 +363,7 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev,
 	if (err)
 		goto err_dl_port_unregister;
 
-	nsim_dev_port->ns = nsim_create(nsim_dev, nsim_dev_port);
+	nsim_dev_port->ns = nsim_create(net, nsim_dev, nsim_dev_port);
 	if (IS_ERR(nsim_dev_port->ns)) {
 		err = PTR_ERR(nsim_dev_port->ns);
 		goto err_port_debugfs_exit;
@@ -404,17 +406,19 @@ static void nsim_dev_port_del_all(struct nsim_dev *nsim_dev)
 
 int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
 {
+	struct net *initial_net = nsim_bus_dev->initial_net;
 	struct nsim_dev *nsim_dev;
 	int i;
 	int err;
 
-	nsim_dev = nsim_dev_create(nsim_bus_dev, nsim_bus_dev->port_count);
+	nsim_dev = nsim_dev_create(initial_net, nsim_bus_dev,
+				   nsim_bus_dev->port_count);
 	if (IS_ERR(nsim_dev))
 		return PTR_ERR(nsim_dev);
 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
 
 	for (i = 0; i < nsim_bus_dev->port_count; i++) {
-		err = __nsim_dev_port_add(nsim_dev, i);
+		err = __nsim_dev_port_add(initial_net, nsim_dev, i);
 		if (err)
 			goto err_port_del_all;
 	}
@@ -449,13 +453,14 @@ int nsim_dev_port_add(struct nsim_bus_dev *nsim_bus_dev,
 		      unsigned int port_index)
 {
 	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
+	struct net *net = devlink_net(priv_to_devlink(nsim_dev));
 	int err;
 
 	mutex_lock(&nsim_dev->port_list_lock);
 	if (__nsim_dev_port_lookup(nsim_dev, port_index))
 		err = -EEXIST;
 	else
-		err = __nsim_dev_port_add(nsim_dev, port_index);
+		err = __nsim_dev_port_add(net, nsim_dev, port_index);
 	mutex_unlock(&nsim_dev->port_list_lock);
 	return err;
 }
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 0740940f41b1..25c7de7a4a31 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -280,7 +280,8 @@ static void nsim_setup(struct net_device *dev)
 }
 
 struct netdevsim *
-nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
+nsim_create(struct net *net, struct nsim_dev *nsim_dev,
+	    struct nsim_dev_port *nsim_dev_port)
 {
 	struct net_device *dev;
 	struct netdevsim *ns;
@@ -290,6 +291,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 
+	dev_net_set(dev, net);
 	ns = netdev_priv(dev);
 	ns->netdev = dev;
 	ns->nsim_dev = nsim_dev;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 79c05af2a7c0..9563acb61b03 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -74,8 +74,11 @@ struct netdevsim {
 	struct nsim_ipsec ipsec;
 };
 
+struct net;
+
 struct netdevsim *
-nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port);
+nsim_create(struct net *net, struct nsim_dev *nsim_dev,
+	    struct nsim_dev_port *nsim_dev_port);
 void nsim_destroy(struct netdevsim *ns);
 
 #ifdef CONFIG_BPF_SYSCALL
@@ -213,6 +216,9 @@ struct nsim_bus_dev {
 	struct device dev;
 	struct list_head list;
 	unsigned int port_count;
+	struct net *initial_net; /* Purpose of this is to carry net pointer
+				  * during the probe time only.
+				  */
 	unsigned int num_vfs;
 	struct nsim_vf_config *vfconfigs;
 };
-- 
2.21.0

[patch iproute2-next v2 1/2] devlink: introduce cmdline option to switch to a different namespace

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 devlink/devlink.c  | 12 ++++++++++--
 man/man8/devlink.8 |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index d8197ea3a478..9242cc05ad0c 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -32,6 +32,7 @@
 #include "mnlg.h"
 #include "json_writer.h"
 #include "utils.h"
+#include "namespace.h"
 
 #define ESWITCH_MODE_LEGACY "legacy"
 #define ESWITCH_MODE_SWITCHDEV "switchdev"
@@ -6332,7 +6333,7 @@ static int cmd_health(struct dl *dl)
 static void help(void)
 {
 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
-	       "       devlink [ -f[orce] ] -b[atch] filename\n"
+	       "       devlink [ -f[orce] ] -b[atch] filename -N[etns] netnsname\n"
 	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | health }\n"
 	       "       OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
 }
@@ -6478,6 +6479,7 @@ int main(int argc, char **argv)
 		{ "json",		no_argument,		NULL, 'j' },
 		{ "pretty",		no_argument,		NULL, 'p' },
 		{ "verbose",		no_argument,		NULL, 'v' },
+		{ "Netns",		required_argument,	NULL, 'N' },
 		{ NULL, 0, NULL, 0 }
 	};
 	const char *batch_file = NULL;
@@ -6493,7 +6495,7 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	while ((opt = getopt_long(argc, argv, "Vfb:njpv",
+	while ((opt = getopt_long(argc, argv, "Vfb:njpvN:",
 				  long_options, NULL)) >= 0) {
 
 		switch (opt) {
@@ -6519,6 +6521,12 @@ int main(int argc, char **argv)
 		case 'v':
 			dl->verbose = true;
 			break;
+		case 'N':
+			if (netns_switch(optarg)) {
+				ret = EXIT_FAILURE;
+				goto dl_free;
+			}
+			break;
 		default:
 			pr_err("Unknown option.\n");
 			help();
diff --git a/man/man8/devlink.8 b/man/man8/devlink.8
index 13d4dcd908b3..9fc9b034eefe 100644
--- a/man/man8/devlink.8
+++ b/man/man8/devlink.8
@@ -51,6 +51,10 @@ When combined with -j generate a pretty JSON output.
 .BR "\-v" , " --verbose"
 Turn on verbose output.
 
+.TP
+.BR "\-N", " \-Netns " <NETNSNAME>
+Switches to the specified network namespace.
+
 .SS
 .I OBJECT
 
-- 
2.21.0

[patch iproute2-next v2 2/2] devlink: add support for network namespace change

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 devlink/devlink.c            | 54 +++++++++++++++++++++++++++++++++++-
 include/uapi/linux/devlink.h |  4 +++
 man/man8/devlink-dev.8       | 12 ++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 9242cc05ad0c..a39bd8771d8b 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -235,6 +235,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_HEALTH_REPORTER_NAME	BIT(27)
 #define DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD	BIT(27)
 #define DL_OPT_HEALTH_REPORTER_AUTO_RECOVER	BIT(28)
+#define DL_OPT_NETNS	BIT(29)
 
 struct dl_opts {
 	uint32_t present; /* flags of present items */
@@ -271,6 +272,8 @@ struct dl_opts {
 	const char *reporter_name;
 	uint64_t reporter_graceful_period;
 	bool reporter_auto_recover;
+	bool netns_is_pid;
+	uint32_t netns;
 };
 
 struct dl {
@@ -1331,6 +1334,22 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
 			if (err)
 				return err;
 			o_found |= DL_OPT_HEALTH_REPORTER_AUTO_RECOVER;
+		} else if (dl_argv_match(dl, "netns") &&
+			(o_all & DL_OPT_NETNS)) {
+			const char *netns_str;
+
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &netns_str);
+			if (err)
+				return err;
+			opts->netns = netns_get_fd(netns_str);
+			if (opts->netns < 0) {
+				err = dl_argv_uint32_t(dl, &opts->netns);
+				if (err)
+					return err;
+				opts->netns_is_pid = true;
+			}
+			o_found |= DL_OPT_NETNS;
 		} else {
 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
 			return -EINVAL;
@@ -1444,7 +1463,11 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
 	if (opts->present & DL_OPT_HEALTH_REPORTER_AUTO_RECOVER)
 		mnl_attr_put_u8(nlh, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,
 				opts->reporter_auto_recover);
-
+	if (opts->present & DL_OPT_NETNS)
+		mnl_attr_put_u32(nlh,
+				 opts->netns_is_pid ? DEVLINK_ATTR_NETNS_PID :
+						      DEVLINK_ATTR_NETNS_FD,
+				 opts->netns);
 }
 
 static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
@@ -1499,6 +1522,7 @@ static bool dl_dump_filter(struct dl *dl, struct nlattr **tb)
 static void cmd_dev_help(void)
 {
 	pr_err("Usage: devlink dev show [ DEV ]\n");
+	pr_err("       devlink dev set DEV netns { PID | NAME | ID }\n");
 	pr_err("       devlink dev eswitch set DEV [ mode { legacy | switchdev } ]\n");
 	pr_err("                               [ inline-mode { none | link | network | transport } ]\n");
 	pr_err("                               [ encap { disable | enable } ]\n");
@@ -2551,6 +2575,31 @@ static int cmd_dev_show(struct dl *dl)
 	return err;
 }
 
+static void cmd_dev_set_help(void)
+{
+	pr_err("Usage: devlink dev set DEV netns { PID | NAME | ID }\n");
+}
+
+static int cmd_dev_set(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	if (dl_argv_match(dl, "help") || dl_no_arg(dl)) {
+		cmd_dev_set_help();
+		return 0;
+	}
+
+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_SET,
+			       NLM_F_REQUEST | NLM_F_ACK);
+
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE, DL_OPT_NETNS);
+	if (err)
+		return err;
+
+	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
 static void cmd_dev_reload_help(void)
 {
 	pr_err("Usage: devlink dev reload [ DEV ]\n");
@@ -2747,6 +2796,9 @@ static int cmd_dev(struct dl *dl)
 		   dl_argv_match(dl, "list") || dl_no_arg(dl)) {
 		dl_arg_inc(dl);
 		return cmd_dev_show(dl);
+	} else if (dl_argv_match(dl, "set")) {
+		dl_arg_inc(dl);
+		return cmd_dev_set(dl);
 	} else if (dl_argv_match(dl, "eswitch")) {
 		dl_arg_inc(dl);
 		return cmd_dev_eswitch(dl);
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index fc195cbd66f4..bc1869993e20 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -348,6 +348,10 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
 	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
 
+	DEVLINK_ATTR_NETNS_FD,			/* u32 */
+	DEVLINK_ATTR_NETNS_PID,			/* u32 */
+	DEVLINK_ATTR_NETNS_ID,			/* u32 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
index 1804463b2321..0e1a5523fa7b 100644
--- a/man/man8/devlink-dev.8
+++ b/man/man8/devlink-dev.8
@@ -25,6 +25,13 @@ devlink-dev \- devlink device configuration
 .ti -8
 .B devlink dev help
 
+.ti -8
+.BR "devlink dev set"
+.IR DEV
+.RI "[ "
+.BI "netns { " PID " | " NAME " | " ID " }
+.RI "]"
+
 .ti -8
 .BR "devlink dev eswitch set"
 .IR DEV
@@ -92,6 +99,11 @@ Format is:
 .in +2
 BUS_NAME/BUS_ADDRESS
 
+.SS devlink dev set  - sets devlink device attributes
+
+.TP
+.BI "netns { " PID " | " NAME " | " ID " }
+
 .SS devlink dev eswitch show - display devlink device eswitch attributes
 .SS devlink dev eswitch set  - sets devlink device eswitch attributes
 
-- 
2.21.0

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jakub Kicinski]

On Tue, 30 Jul 2019 10:57:32 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> All devlink instances are created in init_net and stay there for a
> lifetime. Allow user to be able to move devlink instances into
> namespaces.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

I'm no namespace expert, but seems reasonable, so FWIW:

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

If I read things right we will only send the devlink instance
notification to other namespaces when it moves, but not
notifications for sub-objects like ports. Is the expectation 
that the user space dumps the objects it cares about or will
the other notifications be added as needed (or option 3 - I 
misread the code).

I was also wondering it moving the devlink instance during a 
multi-part transaction could be an issue. But AFAIU it should 
be fine - the flashing doesn't release the instance lock, and 
health stuff should complete correctly even if devlink is moved
half way through?

Re: [patch net-next v2 2/3] net: devlink: export devlink net set/get helpers

[Jakub Kicinski]

On Tue, 30 Jul 2019 10:57:33 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Allow drivers to set/get net struct for devlink instance. Set is only
> allowed for newly allocated devlink instance.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Re: [patch net-next v2 3/3] netdevsim: create devlink and netdev instances in namespace

[Jakub Kicinski]

On Tue, 30 Jul 2019 10:57:34 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> When user does create new netdevsim instance using sysfs bus file,
> create the devlink instance and related netdev instance in the namespace
> of the caller.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

> v1->v2:
> - remove net_namespace.h include and forward decralared net struct
> - add comment to initial_net pointer

Thanks!

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Wed, Jul 31, 2019 at 12:39:52AM CEST, jakub.kicinski@netronome.com wrote:
>On Tue, 30 Jul 2019 10:57:32 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> All devlink instances are created in init_net and stay there for a
>> lifetime. Allow user to be able to move devlink instances into
>> namespaces.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>I'm no namespace expert, but seems reasonable, so FWIW:
>
>Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
>If I read things right we will only send the devlink instance
>notification to other namespaces when it moves, but not
>notifications for sub-objects like ports. Is the expectation 
>that the user space dumps the objects it cares about or will
>the other notifications be added as needed (or option 3 - I 
>misread the code).

You are correct. I plan to take care of the notifications of all objects
in the follow-up patchset. But I can do it in this one if needed. Up to
you.


>
>I was also wondering it moving the devlink instance during a 
>multi-part transaction could be an issue. But AFAIU it should 
>be fine - the flashing doesn't release the instance lock, and 
>health stuff should complete correctly even if devlink is moved
>half way through?

Yes, I don't see any issue there as well.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 7/31/19 1:26 PM, Jiri Pirko wrote:
> Wed, Jul 31, 2019 at 12:39:52AM CEST, jakub.kicinski@netronome.com wrote:
>> On Tue, 30 Jul 2019 10:57:32 +0200, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> All devlink instances are created in init_net and stay there for a
>>> lifetime. Allow user to be able to move devlink instances into
>>> namespaces.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>> I'm no namespace expert, but seems reasonable, so FWIW:
>>
>> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>
>> If I read things right we will only send the devlink instance
>> notification to other namespaces when it moves, but not
>> notifications for sub-objects like ports. Is the expectation 
>> that the user space dumps the objects it cares about or will
>> the other notifications be added as needed (or option 3 - I 
>> misread the code).
> 
> You are correct. I plan to take care of the notifications of all objects
> in the follow-up patchset. But I can do it in this one if needed. Up to
> you.
> 

seems like it should be a part of this one. If a devlink instance
changes namespaces, all of its associated resources should as well.

Also, seems like you are missing a 'can this devlink instance be moved'
check. e.g., what happens if a resource controller has been configured
for the devlink instance and it is moved to a namespace whose existing
config exceeds those limits?

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Wed, Jul 31, 2019 at 09:41:10PM CEST, dsahern@gmail.com wrote:
>On 7/31/19 1:26 PM, Jiri Pirko wrote:
>> Wed, Jul 31, 2019 at 12:39:52AM CEST, jakub.kicinski@netronome.com wrote:
>>> On Tue, 30 Jul 2019 10:57:32 +0200, Jiri Pirko wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> All devlink instances are created in init_net and stay there for a
>>>> lifetime. Allow user to be able to move devlink instances into
>>>> namespaces.
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>
>>> I'm no namespace expert, but seems reasonable, so FWIW:
>>>
>>> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>>
>>> If I read things right we will only send the devlink instance
>>> notification to other namespaces when it moves, but not
>>> notifications for sub-objects like ports. Is the expectation 
>>> that the user space dumps the objects it cares about or will
>>> the other notifications be added as needed (or option 3 - I 
>>> misread the code).
>> 
>> You are correct. I plan to take care of the notifications of all objects
>> in the follow-up patchset. But I can do it in this one if needed. Up to
>> you.
>> 
>
>seems like it should be a part of this one. If a devlink instance
>changes namespaces, all of its associated resources should as well.

Okay. Will do.


>
>Also, seems like you are missing a 'can this devlink instance be moved'

I don't see why not.


>check. e.g., what happens if a resource controller has been configured
>for the devlink instance and it is moved to a namespace whose existing
>config exceeds those limits?

It's moved with all the values. The whole instance is moved.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 7/31/19 1:45 PM, Jiri Pirko wrote:
>> check. e.g., what happens if a resource controller has been configured
>> for the devlink instance and it is moved to a namespace whose existing
>> config exceeds those limits?
> 
> It's moved with all the values. The whole instance is moved.
> 

The values are moved, but the FIB in a namespace could already contain
more routes than the devlink instance allows.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 7/31/19 1:46 PM, David Ahern wrote:
> On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>> check. e.g., what happens if a resource controller has been configured
>>> for the devlink instance and it is moved to a namespace whose existing
>>> config exceeds those limits?
>>
>> It's moved with all the values. The whole instance is moved.
>>
> 
> The values are moved, but the FIB in a namespace could already contain
> more routes than the devlink instance allows.
> 

From a quick test your recent refactoring to netdevsim broke the
resource controller. It was, and is intended to be, per network namespace.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 7/31/19 1:58 PM, David Ahern wrote:
> On 7/31/19 1:46 PM, David Ahern wrote:
>> On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>>> check. e.g., what happens if a resource controller has been configured
>>>> for the devlink instance and it is moved to a namespace whose existing
>>>> config exceeds those limits?
>>>
>>> It's moved with all the values. The whole instance is moved.
>>>
>>
>> The values are moved, but the FIB in a namespace could already contain
>> more routes than the devlink instance allows.
>>
> 
> From a quick test your recent refactoring to netdevsim broke the
> resource controller. It was, and is intended to be, per network namespace.
> 

Specifically this commit:

commit 5fc494225c1eb81309cc4c91f183cd30e4edb674
Author: Jiri Pirko <jiri@mellanox.com>
Date:   Thu Apr 25 15:59:42 2019 +0200

    netdevsim: create devlink instance per netdevsim instance

    Currently there is one devlink instance created per network namespace.
    That is quite odd considering the fact that devlink instance should
    represent an ASIC. The following patches are going to move the devlink
    instance even more down to a bus device, but until then, have one
    devlink instance per netdevsim instance. Struct nsim_devlink is
    introduced to hold fib setting.

    The changes in the fib code are only related to holding the
    configuration per devlink instance instead of network namespace.

broke the intent of the resource controller clearly stated in the
original commit

commit 37923ed6b8cea94d7d76038e2f72c57a0b45daab
Author: David Ahern <dsa@cumulusnetworks.com>
Date:   Tue Mar 27 18:22:00 2018 -0700

    netdevsim: Add simple FIB resource controller via devlink

...
    Currently, devlink only supports initial namespace. Code is in place to
    adapt netdevsim to a per namespace controller once the network namespace
    issues are resolved.

And discussed here based on the RFC and v1 of the original patchset:

https://lore.kernel.org/netdev/03eade79-1727-3a31-8e31-a0a7f51b72cf@cumulusnetworks.com/

and

https://lore.kernel.org/netdev/a916f016-5d8b-3f56-0a84-95d1712bec4c@cumulusnetworks.com/

This is a regression that needs to be fixed.

Re: [patch net-next v2 0/3] net: devlink: Finish network namespace support

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 30 Jul 2019 10:57:31 +0200

> Devlink from the beginning counts with network namespaces, but the
> instances has been fixed to init_net. The first patch allows user
> to move existing devlink instances into namespaces:
 ...

I read this thread and see there will be a v3.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Wed, Jul 31, 2019 at 09:46:13PM CEST, dsahern@gmail.com wrote:
>On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>> check. e.g., what happens if a resource controller has been configured
>>> for the devlink instance and it is moved to a namespace whose existing
>>> config exceeds those limits?
>> 
>> It's moved with all the values. The whole instance is moved.
>> 
>
>The values are moved, but the FIB in a namespace could already contain
>more routes than the devlink instance allows.


There is no relation between fib and devlink.
>

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Wed, Jul 31, 2019 at 09:58:10PM CEST, dsahern@gmail.com wrote:
>On 7/31/19 1:46 PM, David Ahern wrote:
>> On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>>> check. e.g., what happens if a resource controller has been configured
>>>> for the devlink instance and it is moved to a namespace whose existing
>>>> config exceeds those limits?
>>>
>>> It's moved with all the values. The whole instance is moved.
>>>
>> 
>> The values are moved, but the FIB in a namespace could already contain
>> more routes than the devlink instance allows.
>> 
>
>From a quick test your recent refactoring to netdevsim broke the
>resource controller. It was, and is intended to be, per network namespace.

unifying devlink instances with network namespace in netdevsim was
really odd. Netdevsim is also a device, like any other. With other
devices, you do not do this so I don't see why to do this with netdevsim.

Now you create netdevsim instance in sysfs, there is proper bus probe
mechanism done, there is a devlink instance created for this device,
there are netdevices and devlink ports created. Same as for the real
hardware.

Honestly, creating a devlink instance per-network namespace
automagically, no relation to netdevsim devices, that is simply wrong.
There should be always 1:1 relationshin between a device and devlink
instance.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 8/2/19 1:48 AM, Jiri Pirko wrote:
> Wed, Jul 31, 2019 at 09:58:10PM CEST, dsahern@gmail.com wrote:
>> On 7/31/19 1:46 PM, David Ahern wrote:
>>> On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>>>> check. e.g., what happens if a resource controller has been configured
>>>>> for the devlink instance and it is moved to a namespace whose existing
>>>>> config exceeds those limits?
>>>>
>>>> It's moved with all the values. The whole instance is moved.
>>>>
>>>
>>> The values are moved, but the FIB in a namespace could already contain
>>> more routes than the devlink instance allows.
>>>
>>
>>From a quick test your recent refactoring to netdevsim broke the
>> resource controller. It was, and is intended to be, per network namespace.
> 
> unifying devlink instances with network namespace in netdevsim was
> really odd. Netdevsim is also a device, like any other. With other
> devices, you do not do this so I don't see why to do this with netdevsim.
> 
> Now you create netdevsim instance in sysfs, there is proper bus probe
> mechanism done, there is a devlink instance created for this device,
> there are netdevices and devlink ports created. Same as for the real
> hardware.
> 
> Honestly, creating a devlink instance per-network namespace
> automagically, no relation to netdevsim devices, that is simply wrong.
> There should be always 1:1 relationshin between a device and devlink
> instance.
> 

Jiri: prior to your recent change netdevsim had a fib resource
controller per network namespace. Please return that behavior or revert
the change.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Fri, Aug 02, 2019 at 05:45:36PM CEST, dsahern@gmail.com wrote:
>On 8/2/19 1:48 AM, Jiri Pirko wrote:
>> Wed, Jul 31, 2019 at 09:58:10PM CEST, dsahern@gmail.com wrote:
>>> On 7/31/19 1:46 PM, David Ahern wrote:
>>>> On 7/31/19 1:45 PM, Jiri Pirko wrote:
>>>>>> check. e.g., what happens if a resource controller has been configured
>>>>>> for the devlink instance and it is moved to a namespace whose existing
>>>>>> config exceeds those limits?
>>>>>
>>>>> It's moved with all the values. The whole instance is moved.
>>>>>
>>>>
>>>> The values are moved, but the FIB in a namespace could already contain
>>>> more routes than the devlink instance allows.
>>>>
>>>
>>>From a quick test your recent refactoring to netdevsim broke the
>>> resource controller. It was, and is intended to be, per network namespace.
>> 
>> unifying devlink instances with network namespace in netdevsim was
>> really odd. Netdevsim is also a device, like any other. With other
>> devices, you do not do this so I don't see why to do this with netdevsim.
>> 
>> Now you create netdevsim instance in sysfs, there is proper bus probe
>> mechanism done, there is a devlink instance created for this device,
>> there are netdevices and devlink ports created. Same as for the real
>> hardware.
>> 
>> Honestly, creating a devlink instance per-network namespace
>> automagically, no relation to netdevsim devices, that is simply wrong.
>> There should be always 1:1 relationshin between a device and devlink
>> instance.
>> 
>
>Jiri: prior to your recent change netdevsim had a fib resource
>controller per network namespace. Please return that behavior or revert
>the change.

There was implicit devlink instance creation per-namespace. No relation
any actual device. It was wrong and misuse of devlink.

Now you have 1 devlink instance per 1 device as it should be. Also, you
have fib resource control for this device, also as it is done for real
devices, like mlxsw.

Could you please describe your usecase? Perhaps we can handle
it differently.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 8/4/19 11:54 PM, Jiri Pirko wrote:
> There was implicit devlink instance creation per-namespace. No relation
> any actual device. It was wrong and misuse of devlink.
> 
> Now you have 1 devlink instance per 1 device as it should be. Also, you
> have fib resource control for this device, also as it is done for real
> devices, like mlxsw.
> 
> Could you please describe your usecase? Perhaps we can handle
> it differently.

I have described this before, multiple times.

It is documented in the commit log for the initial fib.c in netdevsim
(37923ed6b8cea94d7d76038e2f72c57a0b45daab) and
https://lore.kernel.org/netdev/20180328012200.15175-7-dsa@cumulusnetworks.com/

And this comment in the discussion thread:

https://lore.kernel.org/netdev/e9c59b0c-328e-d343-6e8d-d19f643d2e9d@cumulusnetworks.com/:
"The intention is to treat the kernel's tables *per namespace* as a
standalone entity that can be managed very similar to ASIC resources."


So, to state this again, the fib.c in the RFC version
https://lore.kernel.org/netdev/20180322225757.10377-8-dsa@cumulusnetworks.com/

targeted this:

   namespace 1 |  namespace 2  | ... | namespace N
               |               |     |
               |               |     |
   devlink 1   |    devlink 2  | ... |  devlink N

and each devlink instance has resource limits for the number of fib
rules and fib entries *for that namespace* only.

You objected to how the devlink instances per namespace was implemented,
so the non-RFC version limited the devlink instance and resource
controller to init_net only. Fine. I accepted that limitation until
someone had time to work on devlink instances per network namespace
which you are doing now. So, the above goal will be achievable but first
you need to fix the breakage you put into v5.2 and forward.

Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
per-namepace accounting to all namespaces managed by a single devlink
instance in init_net - which is completely wrong.

Move the fib accounting back to per namespace as the original code
intended. If you now want the devlink instance to be namespace based
then it should be trivial for you to fix it and will work going forward.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Mon, Aug 05, 2019 at 04:10:39PM CEST, dsahern@gmail.com wrote:
>On 8/4/19 11:54 PM, Jiri Pirko wrote:
>> There was implicit devlink instance creation per-namespace. No relation
>> any actual device. It was wrong and misuse of devlink.
>> 
>> Now you have 1 devlink instance per 1 device as it should be. Also, you
>> have fib resource control for this device, also as it is done for real
>> devices, like mlxsw.
>> 
>> Could you please describe your usecase? Perhaps we can handle
>> it differently.
>
>I have described this before, multiple times.
>
>It is documented in the commit log for the initial fib.c in netdevsim
>(37923ed6b8cea94d7d76038e2f72c57a0b45daab) and
>https://lore.kernel.org/netdev/20180328012200.15175-7-dsa@cumulusnetworks.com/
>
>And this comment in the discussion thread:
>
>https://lore.kernel.org/netdev/e9c59b0c-328e-d343-6e8d-d19f643d2e9d@cumulusnetworks.com/:
>"The intention is to treat the kernel's tables *per namespace* as a
>standalone entity that can be managed very similar to ASIC resources."
>
>
>So, to state this again, the fib.c in the RFC version
>https://lore.kernel.org/netdev/20180322225757.10377-8-dsa@cumulusnetworks.com/
>
>targeted this:
>
>   namespace 1 |  namespace 2  | ... | namespace N
>               |               |     |
>               |               |     |
>   devlink 1   |    devlink 2  | ... |  devlink N
>
>and each devlink instance has resource limits for the number of fib
>rules and fib entries *for that namespace* only.
>
>You objected to how the devlink instances per namespace was implemented,
>so the non-RFC version limited the devlink instance and resource
>controller to init_net only. Fine. I accepted that limitation until
>someone had time to work on devlink instances per network namespace
>which you are doing now. So, the above goal will be achievable but first
>you need to fix the breakage you put into v5.2 and forward.
>
>Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>per-namepace accounting to all namespaces managed by a single devlink
>instance in init_net - which is completely wrong.

No. Not "all namespaces". Only the one where the devlink is. And that is
always init_net, until this patchset.


>
>Move the fib accounting back to per namespace as the original code
>intended. If you now want the devlink instance to be namespace based
>then it should be trivial for you to fix it and will work going forward.

With this patchset, you can create netdevsim instance in a namespace,
set the resources limits on the devlink instance for this netdevsim
and you have what you need and what you had before. You just need to
create one netdevsim instance per network namespace.
Or multiple netdevsim instances in one namespace with different
limitations. Up to you.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 8/5/19 8:49 AM, Jiri Pirko wrote:
>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>> per-namepace accounting to all namespaces managed by a single devlink
>> instance in init_net - which is completely wrong.
> No. Not "all namespaces". Only the one where the devlink is. And that is
> always init_net, until this patchset.
> 
> 

Jiri: your change to fib.c does not take into account namespace when
doing rules and routes accounting. you broke it. fix it.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Mon, Aug 05, 2019 at 04:51:22PM CEST, dsahern@gmail.com wrote:
>On 8/5/19 8:49 AM, Jiri Pirko wrote:
>>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>>> per-namepace accounting to all namespaces managed by a single devlink
>>> instance in init_net - which is completely wrong.
>> No. Not "all namespaces". Only the one where the devlink is. And that is
>> always init_net, until this patchset.
>> 
>> 
>
>Jiri: your change to fib.c does not take into account namespace when
>doing rules and routes accounting. you broke it. fix it.

What do you mean by "account namespace"? It's a device resource, why to
tight it with namespace? What if you have 2 netdevsim-devlink instances
in one namespace? Why the setting should be per-namespace?

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 8/5/19 9:20 AM, Jiri Pirko wrote:
> Mon, Aug 05, 2019 at 04:51:22PM CEST, dsahern@gmail.com wrote:
>> On 8/5/19 8:49 AM, Jiri Pirko wrote:
>>>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>>>> per-namepace accounting to all namespaces managed by a single devlink
>>>> instance in init_net - which is completely wrong.
>>> No. Not "all namespaces". Only the one where the devlink is. And that is
>>> always init_net, until this patchset.
>>>
>>>
>>
>> Jiri: your change to fib.c does not take into account namespace when
>> doing rules and routes accounting. you broke it. fix it.
> 
> What do you mean by "account namespace"? It's a device resource, why to
> tight it with namespace? What if you have 2 netdevsim-devlink instances
> in one namespace? Why the setting should be per-namespace?
> 

Jiri:

Here's an example of how your 5.2 change to netdevsim broke the resource
controller:

Create a netdevsim device:
$ modprobe netdevsim
$  echo "0 1" > /sys/bus/netdevsim/new_device

Get the current number of IPv4 routes:
$ n=$(ip -4 ro ls table all | wc -l)
$ echo $n
13

Prevent any more from being added. This limit should apply solely to
this namespace, init_net:

$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size $n
$ devlink dev reload netdevsim/netdevsim0
Error: netdevsim: New size is less than current occupancy.
devlink answers: Invalid argument

So that is the first breakage: accounting is off - maybe. Note there are
no other visible namespaces, but who knows what systemd or other
processes are doing with namespaces. Perhaps this accounting is another
example of your changes not properly handling namespaces:

$ devlink resource show netdevsim/netdevsim0
netdevsim/netdevsim0:
  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size 13 occ 17 unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size unlimited occ 10 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none

So the occupancy does not match the tables for init_net.

Reset the max to 17, the current occupancy:
$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size 17
$ devlink dev reload netdevsim/netdevsim0
$ devlink resource show netdevsim/netdevsim0
netdevsim/netdevsim0:
  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size 17 occ 17 unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size unlimited occ 10 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none

Create a new namespace, bring up lo which attempts to add more route
entries:
$ unshare -n
$ ip li set lo up

If you list routes you see the lo routes failed to installed because of
the limits, but it is a silent failure. Try to add a new route and you
see the cross namespace accounting now:
$ ip ro add 192.168.1.0/24 dev lo
Error: netdevsim: Exceeded number of supported fib entries.


Contrast that behavior with 5.1 and you see the new namespaces have no
bearing on accounting in init_net and limits in init_net do not affect
other namespaces.

That behavior needs to be restored in 5.2 and 5.3.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Tue, Aug 06, 2019 at 07:34:59PM CEST, dsahern@gmail.com wrote:
>On 8/5/19 9:20 AM, Jiri Pirko wrote:
>> Mon, Aug 05, 2019 at 04:51:22PM CEST, dsahern@gmail.com wrote:
>>> On 8/5/19 8:49 AM, Jiri Pirko wrote:
>>>>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>>>>> per-namepace accounting to all namespaces managed by a single devlink
>>>>> instance in init_net - which is completely wrong.
>>>> No. Not "all namespaces". Only the one where the devlink is. And that is
>>>> always init_net, until this patchset.
>>>>
>>>>
>>>
>>> Jiri: your change to fib.c does not take into account namespace when
>>> doing rules and routes accounting. you broke it. fix it.
>> 
>> What do you mean by "account namespace"? It's a device resource, why to
>> tight it with namespace? What if you have 2 netdevsim-devlink instances
>> in one namespace? Why the setting should be per-namespace?
>> 
>
>Jiri:
>
>Here's an example of how your 5.2 change to netdevsim broke the resource
>controller:
>
>Create a netdevsim device:
>$ modprobe netdevsim
>$  echo "0 1" > /sys/bus/netdevsim/new_device
>
>Get the current number of IPv4 routes:
>$ n=$(ip -4 ro ls table all | wc -l)
>$ echo $n
>13
>
>Prevent any more from being added. This limit should apply solely to
>this namespace, init_net:
>
>$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size $n
>$ devlink dev reload netdevsim/netdevsim0
>Error: netdevsim: New size is less than current occupancy.
>devlink answers: Invalid argument
>
>So that is the first breakage: accounting is off - maybe. Note there are
>no other visible namespaces, but who knows what systemd or other
>processes are doing with namespaces. Perhaps this accounting is another
>example of your changes not properly handling namespaces:
>
>$ devlink resource show netdevsim/netdevsim0
>netdevsim/netdevsim0:
>  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>    resources:
>      name fib size 13 occ 17 unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>    resources:
>      name fib size unlimited occ 10 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>
>So the occupancy does not match the tables for init_net.
>
>Reset the max to 17, the current occupancy:
>$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size 17
>$ devlink dev reload netdevsim/netdevsim0
>$ devlink resource show netdevsim/netdevsim0
>netdevsim/netdevsim0:
>  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>    resources:
>      name fib size 17 occ 17 unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
>size_gran 1 dpipe_tables none
>    resources:
>      name fib size unlimited occ 10 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
>unlimited size_gran 1 dpipe_tables none
>
>Create a new namespace, bring up lo which attempts to add more route
>entries:
>$ unshare -n
>$ ip li set lo up
>
>If you list routes you see the lo routes failed to installed because of
>the limits, but it is a silent failure. Try to add a new route and you
>see the cross namespace accounting now:
>$ ip ro add 192.168.1.0/24 dev lo
>Error: netdevsim: Exceeded number of supported fib entries.
>
>
>Contrast that behavior with 5.1 and you see the new namespaces have no
>bearing on accounting in init_net and limits in init_net do not affect
>other namespaces.
>
>That behavior needs to be restored in 5.2 and 5.3.

Let's figure out the devlink-controlling-kernel-resources thread first.
What you describe here is exactly that.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern]

On 8/6/19 11:53 AM, Jiri Pirko wrote:
> Let's figure out the devlink-controlling-kernel-resources thread first.
> What you describe here is exactly that.

as I mentioned on the phone, any outcome of that thread will be in 5.4
at best. Meanwhile this breakage in 5.2 and 5.3 needs to be fixed.

Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[Jiri Pirko]

Tue, Aug 06, 2019 at 07:55:30PM CEST, dsahern@gmail.com wrote:
>On 8/6/19 11:53 AM, Jiri Pirko wrote:
>> Let's figure out the devlink-controlling-kernel-resources thread first.
>> What you describe here is exactly that.
>
>as I mentioned on the phone, any outcome of that thread will be in 5.4
>at best. Meanwhile this breakage in 5.2 and 5.3 needs to be fixed.

Why? netdevsim is a dummy device, the purpose of existence is to test
kernel api. No real harm breaking it.