[PATCH net-next 0/2] Ease nsid allocation

[PATCH net-next 0/2] Ease nsid allocation

[Nicolas Dichtel]

The goal of the series is to ease nsid allocation from userland.
The first patch is a preparation work and the second enables to receive the
new nsid in the answer to RTM_NEWNSID.

 net/core/net_namespace.c | 118 ++++++++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 47 deletions(-)

Comments are welcomed,
Nicolas

[PATCH net-next 1/2] netns: move rtnl_net_get_size() and rtnl_net_fill()

[Nicolas Dichtel]

There is no functional change in this patch, it only prepares the next one
where rtnl_net_newid() will use rtnl_net_get_size() and rtnl_net_fill().

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/net_namespace.c | 92 ++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a0e0d298c991..8f5fa5d5becd 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -716,6 +716,52 @@ static const struct nla_policy rtnl_net_policy[NETNSA_MAX + 1] = {
 	[NETNSA_TARGET_NSID]	= { .type = NLA_S32 },
 };
 
+struct net_fill_args {
+	u32 portid;
+	u32 seq;
+	int flags;
+	int cmd;
+	int nsid;
+	bool add_ref;
+	int ref_nsid;
+};
+
+static int rtnl_net_get_size(void)
+{
+	return NLMSG_ALIGN(sizeof(struct rtgenmsg))
+	       + nla_total_size(sizeof(s32)) /* NETNSA_NSID */
+	       + nla_total_size(sizeof(s32)) /* NETNSA_CURRENT_NSID */
+	       ;
+}
+
+static int rtnl_net_fill(struct sk_buff *skb, struct net_fill_args *args)
+{
+	struct nlmsghdr *nlh;
+	struct rtgenmsg *rth;
+
+	nlh = nlmsg_put(skb, args->portid, args->seq, args->cmd, sizeof(*rth),
+			args->flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	rth = nlmsg_data(nlh);
+	rth->rtgen_family = AF_UNSPEC;
+
+	if (nla_put_s32(skb, NETNSA_NSID, args->nsid))
+		goto nla_put_failure;
+
+	if (args->add_ref &&
+	    nla_put_s32(skb, NETNSA_CURRENT_NSID, args->ref_nsid))
+		goto nla_put_failure;
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
 static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  struct netlink_ext_ack *extack)
 {
@@ -776,52 +822,6 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-static int rtnl_net_get_size(void)
-{
-	return NLMSG_ALIGN(sizeof(struct rtgenmsg))
-	       + nla_total_size(sizeof(s32)) /* NETNSA_NSID */
-	       + nla_total_size(sizeof(s32)) /* NETNSA_CURRENT_NSID */
-	       ;
-}
-
-struct net_fill_args {
-	u32 portid;
-	u32 seq;
-	int flags;
-	int cmd;
-	int nsid;
-	bool add_ref;
-	int ref_nsid;
-};
-
-static int rtnl_net_fill(struct sk_buff *skb, struct net_fill_args *args)
-{
-	struct nlmsghdr *nlh;
-	struct rtgenmsg *rth;
-
-	nlh = nlmsg_put(skb, args->portid, args->seq, args->cmd, sizeof(*rth),
-			args->flags);
-	if (!nlh)
-		return -EMSGSIZE;
-
-	rth = nlmsg_data(nlh);
-	rth->rtgen_family = AF_UNSPEC;
-
-	if (nla_put_s32(skb, NETNSA_NSID, args->nsid))
-		goto nla_put_failure;
-
-	if (args->add_ref &&
-	    nla_put_s32(skb, NETNSA_CURRENT_NSID, args->ref_nsid))
-		goto nla_put_failure;
-
-	nlmsg_end(skb, nlh);
-	return 0;
-
-nla_put_failure:
-	nlmsg_cancel(skb, nlh);
-	return -EMSGSIZE;
-}
-
 static int rtnl_net_valid_getid_req(struct sk_buff *skb,
 				    const struct nlmsghdr *nlh,
 				    struct nlattr **tb,
-- 
2.23.0

[PATCH net-next 2/2] netns/rtnl: return the new nsid to the user

[Nicolas Dichtel]

When the user asks for a new nsid, he can let the kernel choose it (by
providing -1 in NETNSA_NSID). In this case, it's useful to answer to the
netlink message with the chosen nsid.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/net_namespace.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 8f5fa5d5becd..266d095296f3 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -810,8 +810,32 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = alloc_netid(net, peer, nsid);
 	spin_unlock_bh(&net->nsid_lock);
 	if (err >= 0) {
+		struct net_fill_args fillargs = {
+			.portid = NETLINK_CB(skb).portid,
+			.seq = nlh->nlmsg_seq,
+			.cmd = RTM_NEWNSID,
+			.nsid = err,
+		};
+		struct sk_buff *msg;
+
+		/* The id has been allocated, thus first notify listeners */
 		rtnl_net_notifyid(net, RTM_NEWNSID, err);
-		err = 0;
+
+		/* Then, try to send the new nsid to the sender */
+		msg = nlmsg_new(rtnl_net_get_size(), GFP_KERNEL);
+		if (!msg) {
+			err = -ENOMEM;
+			NL_SET_ERR_MSG(extack, "Unable to alloc reply msg");
+			goto out;
+		}
+
+		err = rtnl_net_fill(msg, &fillargs);
+		if (err < 0) {
+			kfree_skb(msg);
+			goto out;
+		}
+
+		err = rtnl_unicast(msg, net, NETLINK_CB(skb).portid);
 	} else if (err == -ENOSPC && nsid >= 0) {
 		err = -EEXIST;
 		NL_SET_BAD_ATTR(extack, tb[NETNSA_NSID]);
-- 
2.23.0

Re: [PATCH net-next 0/2] Ease nsid allocation

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 30 Sep 2019 18:02:12 +0200

> The goal of the series is to ease nsid allocation from userland.
> The first patch is a preparation work and the second enables to receive the
> new nsid in the answer to RTM_NEWNSID.

The new reply message could break existing apps.

If an app only performs netnsid operations, and fills up the receive
queue because it isn't reading these new replies (it had no reason to,
they didn't exist previously), operations will start failing that
would not fail previously because the receive queue is full.

Given this, I don't see how we can make the change.

Re: [PATCH net-next 0/2] Ease nsid allocation

[Nicolas Dichtel]

Le 02/10/2019 à 03:20, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Mon, 30 Sep 2019 18:02:12 +0200
> 
>> The goal of the series is to ease nsid allocation from userland.
>> The first patch is a preparation work and the second enables to receive the
>> new nsid in the answer to RTM_NEWNSID.
> 
> The new reply message could break existing apps.
> 
> If an app only performs netnsid operations, and fills up the receive
> queue because it isn't reading these new replies (it had no reason to,
> they didn't exist previously), operations will start failing that
> would not fail previously because the receive queue is full.
Yes I see the problem. I was wondering if this was acceptable because the nl ack
is sent at the end. But nl ack are optional :/

> 
> Given this, I don't see how we can make the change.
> 
Is a new flag attribute ok to turn on this reply?


Thank you,
Nicolas

Re: [PATCH net-next 0/2] Ease nsid allocation

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 2 Oct 2019 10:46:03 +0200

> Is a new flag attribute ok to turn on this reply?

That might work, yes.

Re: [PATCH net-next 0/2] Ease nsid allocation

[Guillaume Nault]

On Wed, Oct 02, 2019 at 10:46:03AM +0200, Nicolas Dichtel wrote:
> Le 02/10/2019 à 03:20, David Miller a écrit :
> > From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> > Date: Mon, 30 Sep 2019 18:02:12 +0200
> > 
> >> The goal of the series is to ease nsid allocation from userland.
> >> The first patch is a preparation work and the second enables to receive the
> >> new nsid in the answer to RTM_NEWNSID.
> > 
> > The new reply message could break existing apps.
> > 
> > If an app only performs netnsid operations, and fills up the receive
> > queue because it isn't reading these new replies (it had no reason to,
> > they didn't exist previously), operations will start failing that
> > would not fail previously because the receive queue is full.
> Yes I see the problem. I was wondering if this was acceptable because the nl ack
> is sent at the end. But nl ack are optional :/
> 
> > 
> > Given this, I don't see how we can make the change.
> > 
> Is a new flag attribute ok to turn on this reply?
> 
Why not using the existing NLM_F_ECHO mechanism?

IIUC, if rtnl_net_notifyid() did pass the proper nlmsghdr and portid to
rtnl_notify(), the later would automatically notify the caller with
updated information if the original request had the NLM_F_ECHO flag.

Re: [PATCH net-next 0/2] Ease nsid allocation

[Nicolas Dichtel]

Le 03/10/2019 à 18:19, Guillaume Nault a écrit :
[snip]
> Why not using the existing NLM_F_ECHO mechanism?
> 
> IIUC, if rtnl_net_notifyid() did pass the proper nlmsghdr and portid to
> rtnl_notify(), the later would automatically notify the caller with
> updated information if the original request had the NLM_F_ECHO flag.
> 
Good point. Note that with library like libnl, the auto sequence number check
will fail (seq number is 0 instead of the previous + 1) and thus must be bypassed.

[PATCH net] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Nicolas Dichtel]

The flag NLM_F_ECHO aims to reply to the user the message notified to all
listeners.
It was not the case with the command RTM_NEWNSID, let's fix this.

Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
Reported-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/net_namespace.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a0e0d298c991..f496ce0e8da8 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -245,7 +245,8 @@ static int __peernet2id(struct net *net, struct net *peer)
 	return __peernet2id_alloc(net, peer, &no);
 }
 
-static void rtnl_net_notifyid(struct net *net, int cmd, int id);
+static void rtnl_net_notifyid(struct net *net, int cmd, int id, u32 portid,
+			      struct nlmsghdr *nlh);
 /* This function returns the id of a peer netns. If no id is assigned, one will
  * be allocated and returned.
  */
@@ -268,7 +269,7 @@ int peernet2id_alloc(struct net *net, struct net *peer)
 	id = __peernet2id_alloc(net, peer, &alloc);
 	spin_unlock_bh(&net->nsid_lock);
 	if (alloc && id >= 0)
-		rtnl_net_notifyid(net, RTM_NEWNSID, id);
+		rtnl_net_notifyid(net, RTM_NEWNSID, id, 0, NULL);
 	if (alive)
 		put_net(peer);
 	return id;
@@ -532,7 +533,7 @@ static void unhash_nsid(struct net *net, struct net *last)
 			idr_remove(&tmp->netns_ids, id);
 		spin_unlock_bh(&tmp->nsid_lock);
 		if (id >= 0)
-			rtnl_net_notifyid(tmp, RTM_DELNSID, id);
+			rtnl_net_notifyid(tmp, RTM_DELNSID, id, 0, NULL);
 		if (tmp == last)
 			break;
 	}
@@ -764,7 +765,8 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = alloc_netid(net, peer, nsid);
 	spin_unlock_bh(&net->nsid_lock);
 	if (err >= 0) {
-		rtnl_net_notifyid(net, RTM_NEWNSID, err);
+		rtnl_net_notifyid(net, RTM_NEWNSID, err, NETLINK_CB(skb).portid,
+				  nlh);
 		err = 0;
 	} else if (err == -ENOSPC && nsid >= 0) {
 		err = -EEXIST;
@@ -1051,7 +1053,8 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
 	return err < 0 ? err : skb->len;
 }
 
-static void rtnl_net_notifyid(struct net *net, int cmd, int id)
+static void rtnl_net_notifyid(struct net *net, int cmd, int id, u32 portid,
+			      struct nlmsghdr *nlh)
 {
 	struct net_fill_args fillargs = {
 		.cmd = cmd,
@@ -1068,7 +1071,7 @@ static void rtnl_net_notifyid(struct net *net, int cmd, int id)
 	if (err < 0)
 		goto err_out;
 
-	rtnl_notify(msg, net, 0, RTNLGRP_NSID, NULL, 0);
+	rtnl_notify(msg, net, portid, RTNLGRP_NSID, nlh, 0);
 	return;
 
 err_out:
-- 
2.23.0

Re: [PATCH net-next 0/2] Ease nsid allocation

[Guillaume Nault]

On Fri, Oct 04, 2019 at 05:45:22PM +0200, Nicolas Dichtel wrote:
> Le 03/10/2019 à 18:19, Guillaume Nault a écrit :
> [snip]
> > Why not using the existing NLM_F_ECHO mechanism?
> > 
> > IIUC, if rtnl_net_notifyid() did pass the proper nlmsghdr and portid to
> > rtnl_notify(), the later would automatically notify the caller with
> > updated information if the original request had the NLM_F_ECHO flag.
> > 
> Good point. Note that with library like libnl, the auto sequence number check
> will fail (seq number is 0 instead of the previous + 1) and thus must be bypassed.
>
Well, it's up to the caller of rtnl_notify() to build a netlink message
with proper sequence number. Currently, rtnl_net_notifyid() always sets
it to 0. That's probably why the libnl test fails.

Re: [PATCH net] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Guillaume Nault]

On Mon, Oct 07, 2019 at 01:58:35PM +0200, Nicolas Dichtel wrote:
> The flag NLM_F_ECHO aims to reply to the user the message notified to all
> listeners.
> It was not the case with the command RTM_NEWNSID, let's fix this.
> 
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/core/net_namespace.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index a0e0d298c991..f496ce0e8da8 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> -static void rtnl_net_notifyid(struct net *net, int cmd, int id)
> +static void rtnl_net_notifyid(struct net *net, int cmd, int id, u32 portid,
> +			      struct nlmsghdr *nlh)
>  {
>  	struct net_fill_args fillargs = {
>  		.cmd = cmd,

We also need to set .portid and .seq otherwise rtnl_net_fill() builds
a netlink message with invalid port id and sequence number (as you
noted in your previous message).

Re: [PATCH net] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Nicolas Dichtel]

Le 09/10/2019 à 01:10, Guillaume Nault a écrit :
[snip]
> We also need to set .portid and .seq otherwise rtnl_net_fill() builds
> a netlink message with invalid port id and sequence number (as you
> noted in your previous message).
> 
Yes you're right. I don't know why, I had in mind that nl msg sent by the kernel
should have the portid and seq number set to 0.
Will send a v2.


Thank you,
Nicolas

[PATCH net v2] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Nicolas Dichtel]

The flag NLM_F_ECHO aims to reply to the user the message notified to all
listeners.
It was not the case with the command RTM_NEWNSID, let's fix this.

Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
Reported-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2:
  fix portid and seq number of the nl msg sent by the kernel

 net/core/net_namespace.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a0e0d298c991..6d3e4821b02d 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -245,7 +245,8 @@ static int __peernet2id(struct net *net, struct net *peer)
 	return __peernet2id_alloc(net, peer, &no);
 }
 
-static void rtnl_net_notifyid(struct net *net, int cmd, int id);
+static void rtnl_net_notifyid(struct net *net, int cmd, int id, u32 portid,
+			      struct nlmsghdr *nlh);
 /* This function returns the id of a peer netns. If no id is assigned, one will
  * be allocated and returned.
  */
@@ -268,7 +269,7 @@ int peernet2id_alloc(struct net *net, struct net *peer)
 	id = __peernet2id_alloc(net, peer, &alloc);
 	spin_unlock_bh(&net->nsid_lock);
 	if (alloc && id >= 0)
-		rtnl_net_notifyid(net, RTM_NEWNSID, id);
+		rtnl_net_notifyid(net, RTM_NEWNSID, id, 0, NULL);
 	if (alive)
 		put_net(peer);
 	return id;
@@ -532,7 +533,7 @@ static void unhash_nsid(struct net *net, struct net *last)
 			idr_remove(&tmp->netns_ids, id);
 		spin_unlock_bh(&tmp->nsid_lock);
 		if (id >= 0)
-			rtnl_net_notifyid(tmp, RTM_DELNSID, id);
+			rtnl_net_notifyid(tmp, RTM_DELNSID, id, 0, NULL);
 		if (tmp == last)
 			break;
 	}
@@ -764,7 +765,8 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = alloc_netid(net, peer, nsid);
 	spin_unlock_bh(&net->nsid_lock);
 	if (err >= 0) {
-		rtnl_net_notifyid(net, RTM_NEWNSID, err);
+		rtnl_net_notifyid(net, RTM_NEWNSID, err, NETLINK_CB(skb).portid,
+				  nlh);
 		err = 0;
 	} else if (err == -ENOSPC && nsid >= 0) {
 		err = -EEXIST;
@@ -1051,9 +1053,12 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
 	return err < 0 ? err : skb->len;
 }
 
-static void rtnl_net_notifyid(struct net *net, int cmd, int id)
+static void rtnl_net_notifyid(struct net *net, int cmd, int id, u32 portid,
+			      struct nlmsghdr *nlh)
 {
 	struct net_fill_args fillargs = {
+		.portid = portid,
+		.seq = nlh ? nlh->nlmsg_seq : 0,
 		.cmd = cmd,
 		.nsid = id,
 	};
@@ -1068,7 +1073,7 @@ static void rtnl_net_notifyid(struct net *net, int cmd, int id)
 	if (err < 0)
 		goto err_out;
 
-	rtnl_notify(msg, net, 0, RTNLGRP_NSID, NULL, 0);
+	rtnl_notify(msg, net, portid, RTNLGRP_NSID, nlh, 0);
 	return;
 
 err_out:
-- 
2.23.0

Re: [PATCH net] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Guillaume Nault]

On Wed, Oct 09, 2019 at 10:07:56AM +0200, Nicolas Dichtel wrote:
> Le 09/10/2019 à 01:10, Guillaume Nault a écrit :
> [snip]
> > We also need to set .portid and .seq otherwise rtnl_net_fill() builds
> > a netlink message with invalid port id and sequence number (as you
> > noted in your previous message).
> > 
> Yes you're right. I don't know why, I had in mind that nl msg sent by the kernel
> should have the portid and seq number set to 0.
> Will send a v2.
> 
I guess this idea comes from the fact that portid and seq don't carry
any meaningful information when the message is sent to a multicast
group.

Re: [PATCH net v2] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Guillaume Nault]

On Wed, Oct 09, 2019 at 11:19:10AM +0200, Nicolas Dichtel wrote:
> The flag NLM_F_ECHO aims to reply to the user the message notified to all
> listeners.
> It was not the case with the command RTM_NEWNSID, let's fix this.
> 
Acked-by: Guillaume Nault <gnault@redhat.com>
Tested-by: Guillaume Nault <gnault@redhat.com>

Re: [PATCH net v2] netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID

[Jakub Kicinski]

On Wed,  9 Oct 2019 11:19:10 +0200, Nicolas Dichtel wrote:
> The flag NLM_F_ECHO aims to reply to the user the message notified to all
> listeners.
> It was not the case with the command RTM_NEWNSID, let's fix this.
> 
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied, thanks!