| Message ID | 1496304007-1962-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Thu, 1 Jun 2017 10:00:07 +0200 > The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a > nsid assigned into the netns where the netlink socket is opened. > The nsid is sent as metadata to userland, but the existence of this nsid is > checked only for netns that are different from the socket netns. Thus, if > no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is > reported to the userland. This value is confusing and useless. > After this patch, only valid nsid are sent to userland. > > Reported-by: Flavio Leitner <fbl@sysclose.org> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Applied, thank you.
On Thu, Jun 01, 2017 at 10:00:07AM +0200, Nicolas Dichtel wrote: > The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a > nsid assigned into the netns where the netlink socket is opened. > The nsid is sent as metadata to userland, but the existence of this nsid is > checked only for netns that are different from the socket netns. Thus, if > no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is > reported to the userland. This value is confusing and useless. > After this patch, only valid nsid are sent to userland. > > Reported-by: Flavio Leitner <fbl@sysclose.org> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > net/netlink/af_netlink.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index ee841f00a6ec..7586d446d7dc 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -62,6 +62,7 @@ > #include <asm/cacheflush.h> > #include <linux/hash.h> > #include <linux/genetlink.h> > +#include <linux/net_namespace.h> > > #include <net/net_namespace.h> > #include <net/sock.h> > @@ -1415,7 +1416,8 @@ static void do_one_broadcast(struct sock *sk, > goto out; > } > NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); > - NETLINK_CB(p->skb2).nsid_is_set = true; > + if (NETLINK_CB(p->skb2).nsid != NETNSA_NSID_NOT_ASSIGNED) > + NETLINK_CB(p->skb2).nsid_is_set = true; > val = netlink_broadcast_deliver(sk, p->skb2); > if (val < 0) { > netlink_overrun(sk); If the assumption is that nsid allocation can never fail or that if it does, we can't report to userspace, then the patch is good, but it doesn't sound like a good long term solution. Let's consider that the allocation of an id fails for whatever reason. I think that should be reported to userspace to allow it to retry, or do something else to handle this situation properly. Not sending anything means that it's in the same netns as the old kernels did, which is incorrect. On the other hand, with the original patch, if the socket and the device are in the same netns, we don't need to report any ID. Previous kernels did that, so we are not breaking anything. When the netns differs, then we either should report the real ID or an error.
Le 01/06/2017 à 19:02, Flavio Leitner a écrit : > On Thu, Jun 01, 2017 at 10:00:07AM +0200, Nicolas Dichtel wrote: >> The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a >> nsid assigned into the netns where the netlink socket is opened. >> The nsid is sent as metadata to userland, but the existence of this nsid is >> checked only for netns that are different from the socket netns. Thus, if >> no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is >> reported to the userland. This value is confusing and useless. >> After this patch, only valid nsid are sent to userland. >> >> Reported-by: Flavio Leitner <fbl@sysclose.org> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> --- >> net/netlink/af_netlink.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c >> index ee841f00a6ec..7586d446d7dc 100644 >> --- a/net/netlink/af_netlink.c >> +++ b/net/netlink/af_netlink.c >> @@ -62,6 +62,7 @@ >> #include <asm/cacheflush.h> >> #include <linux/hash.h> >> #include <linux/genetlink.h> >> +#include <linux/net_namespace.h> >> >> #include <net/net_namespace.h> >> #include <net/sock.h> >> @@ -1415,7 +1416,8 @@ static void do_one_broadcast(struct sock *sk, >> goto out; >> } >> NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); >> - NETLINK_CB(p->skb2).nsid_is_set = true; >> + if (NETLINK_CB(p->skb2).nsid != NETNSA_NSID_NOT_ASSIGNED) >> + NETLINK_CB(p->skb2).nsid_is_set = true; >> val = netlink_broadcast_deliver(sk, p->skb2); >> if (val < 0) { >> netlink_overrun(sk); > > If the assumption is that nsid allocation can never fail or that if it > does, we can't report to userspace, then the patch is good, but it > doesn't sound like a good long term solution. > > Let's consider that the allocation of an id fails for whatever reason. > I think that should be reported to userspace to allow it to retry, or > do something else to handle this situation properly. Not sending > anything means that it's in the same netns as the old kernels did, > which is incorrect. This is correct, because if nsid allocation fails, no netlink messages from this netns are sent to userspace (the check is done at the beginning of do_one_broadcast). The only netns allowed to send netlink messages to userspace without nsid is the netns of the socket. > > On the other hand, with the original patch, if the socket and the > device are in the same netns, we don't need to report any ID. Previous > kernels did that, so we are not breaking anything. When the netns > differs, then we either should report the real ID or an error. > I don't understand. With or without my last patch, the kernel sends netlink messages of other netns than the netns where the socket is opened, only if an nsid is assigned. Nicolas ps: I won't be able to read my emails before monday ;-)
On Thu, Jun 01, 2017 at 10:42:13PM +0200, Nicolas Dichtel wrote: > Le 01/06/2017 à 19:02, Flavio Leitner a écrit : > > On Thu, Jun 01, 2017 at 10:00:07AM +0200, Nicolas Dichtel wrote: > >> The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a > >> nsid assigned into the netns where the netlink socket is opened. > >> The nsid is sent as metadata to userland, but the existence of this nsid is > >> checked only for netns that are different from the socket netns. Thus, if > >> no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is > >> reported to the userland. This value is confusing and useless. > >> After this patch, only valid nsid are sent to userland. > >> > >> Reported-by: Flavio Leitner <fbl@sysclose.org> > >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > >> --- > >> net/netlink/af_netlink.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > >> index ee841f00a6ec..7586d446d7dc 100644 > >> --- a/net/netlink/af_netlink.c > >> +++ b/net/netlink/af_netlink.c > >> @@ -62,6 +62,7 @@ > >> #include <asm/cacheflush.h> > >> #include <linux/hash.h> > >> #include <linux/genetlink.h> > >> +#include <linux/net_namespace.h> > >> > >> #include <net/net_namespace.h> > >> #include <net/sock.h> > >> @@ -1415,7 +1416,8 @@ static void do_one_broadcast(struct sock *sk, > >> goto out; > >> } > >> NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); > >> - NETLINK_CB(p->skb2).nsid_is_set = true; > >> + if (NETLINK_CB(p->skb2).nsid != NETNSA_NSID_NOT_ASSIGNED) > >> + NETLINK_CB(p->skb2).nsid_is_set = true; > >> val = netlink_broadcast_deliver(sk, p->skb2); > >> if (val < 0) { > >> netlink_overrun(sk); > > > > If the assumption is that nsid allocation can never fail or that if it > > does, we can't report to userspace, then the patch is good, but it > > doesn't sound like a good long term solution. > > > > Let's consider that the allocation of an id fails for whatever reason. > > I think that should be reported to userspace to allow it to retry, or > > do something else to handle this situation properly. Not sending > > anything means that it's in the same netns as the old kernels did, > > which is incorrect. > This is correct, because if nsid allocation fails, no netlink messages from this > netns are sent to userspace (the check is done at the beginning of > do_one_broadcast). The only netns allowed to send netlink messages to userspace > without nsid is the netns of the socket. I say it's incorrect because of the explanation below. > > On the other hand, with the original patch, if the socket and the > > device are in the same netns, we don't need to report any ID. Previous > > kernels did that, so we are not breaking anything. When the netns > > differs, then we either should report the real ID or an error. > > > I don't understand. With or without my last patch, the kernel sends netlink > messages of other netns than the netns where the socket is opened, only if an > nsid is assigned. "only if an nsid is assigned" that's the issue. Let me ask this instead: How do you think userspace should behave when netnsid allocation fails?
Le 02/06/2017 à 00:44, Flavio Leitner a écrit : > On Thu, Jun 01, 2017 at 10:42:13PM +0200, Nicolas Dichtel wrote: >> Le 01/06/2017 à 19:02, Flavio Leitner a écrit : [snip] >>> On the other hand, with the original patch, if the socket and the >>> device are in the same netns, we don't need to report any ID. Previous >>> kernels did that, so we are not breaking anything. When the netns >>> differs, then we either should report the real ID or an error. >>> >> I don't understand. With or without my last patch, the kernel sends netlink >> messages of other netns than the netns where the socket is opened, only if an >> nsid is assigned. > > "only if an nsid is assigned" that's the issue. It was design like that because it's not legitimate to unconditionally listen all netns of the system. Isolation between namespaces must be respected (scenarii with containers, etc.). When a nsid is assigned to a peer netns, it's a way to say "ok, I know this netns and I have access to it". > > Let me ask this instead: How do you think userspace should behave when > netnsid allocation fails? > There is two ways to assign a nsid: - manually with netlink ('ip netns set'). In this case, the error is reported to userspace via netlink. - automatically when a x-netns interface is created. The link-nsid is also reported to userspace. If the allocation failed, NETNSA_NSID_NOT_ASSIGNED is reported. And if you were able to create this x-netns interface, it means that you have access to this peer netns, thus you can try to assign the nsid manually. So, in both cases, userland knows that something went wrong. Do you have another scenario in mind? Nicolas
On Mon, Jun 05, 2017 at 10:40:24AM +0200, Nicolas Dichtel wrote: > > Let me ask this instead: How do you think userspace should behave when > > netnsid allocation fails? > > > There is two ways to assign a nsid: > - manually with netlink ('ip netns set'). In this case, the error is reported > to userspace via netlink. OK. > - automatically when a x-netns interface is created. The link-nsid is also > reported to userspace. If the allocation failed, NETNSA_NSID_NOT_ASSIGNED is > reported. And if you were able to create this x-netns interface, it means > that you have access to this peer netns, thus you can try to assign the nsid > manually. Does that prevent the interface to be created? > So, in both cases, userland knows that something went wrong. > Do you have another scenario in mind? Let's say the app is restarted, or another monitoring app is executed with enough perms. How will it identify the error condition?
Le 07/06/2017 à 21:14, Flavio Leitner a écrit : > On Mon, Jun 05, 2017 at 10:40:24AM +0200, Nicolas Dichtel wrote: >>> Let me ask this instead: How do you think userspace should behave when >>> netnsid allocation fails? >>> >> There is two ways to assign a nsid: >> - manually with netlink ('ip netns set'). In this case, the error is reported >> to userspace via netlink. > > OK. > >> - automatically when a x-netns interface is created. The link-nsid is also >> reported to userspace. If the allocation failed, NETNSA_NSID_NOT_ASSIGNED is >> reported. And if you were able to create this x-netns interface, it means >> that you have access to this peer netns, thus you can try to assign the nsid >> manually. > > Does that prevent the interface to be created? No. > >> So, in both cases, userland knows that something went wrong. >> Do you have another scenario in mind? > > Let's say the app is restarted, or another monitoring app is executed > with enough perms. How will it identify the error condition? Your app wants to monitor a subset of netns. It means that you already have a way to identify those netns, something like a file stored somewhere (/var/run/netns/, /proc/<pid>/ns/net, ...). Thus, it's easy to check if those netns have a nsid assigned in the netns where your app will open the socket. This option was called NETLINK_F_LISTEN_ALL_NSID, because it only enables to listen netns *with* a nsid assigned, nothing more. It's up to the user to ensure that nsid are correctly assigned. Regards, Nicolas
On Thu, Jun 08, 2017 at 10:31:53AM +0200, Nicolas Dichtel wrote: > Le 07/06/2017 à 21:14, Flavio Leitner a écrit : > > Let's say the app is restarted, or another monitoring app is executed > > with enough perms. How will it identify the error condition? > Your app wants to monitor a subset of netns. It means that you already have a > way to identify those netns, something like a file stored somewhere > (/var/run/netns/, /proc/<pid>/ns/net, ...). Thus, it's easy to check if those > netns have a nsid assigned in the netns where your app will open the socket. > > This option was called NETLINK_F_LISTEN_ALL_NSID, because it only enables to > listen netns *with* a nsid assigned, nothing more. It's up to the user to ensure > that nsid are correctly assigned. Makes sense, thanks.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index ee841f00a6ec..7586d446d7dc 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -62,6 +62,7 @@ #include <asm/cacheflush.h> #include <linux/hash.h> #include <linux/genetlink.h> +#include <linux/net_namespace.h> #include <net/net_namespace.h> #include <net/sock.h> @@ -1415,7 +1416,8 @@ static void do_one_broadcast(struct sock *sk, goto out; } NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); - NETLINK_CB(p->skb2).nsid_is_set = true; + if (NETLINK_CB(p->skb2).nsid != NETNSA_NSID_NOT_ASSIGNED) + NETLINK_CB(p->skb2).nsid_is_set = true; val = netlink_broadcast_deliver(sk, p->skb2); if (val < 0) { netlink_overrun(sk);
The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a nsid assigned into the netns where the netlink socket is opened. The nsid is sent as metadata to userland, but the existence of this nsid is checked only for netns that are different from the socket netns. Thus, if no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is reported to the userland. This value is confusing and useless. After this patch, only valid nsid are sent to userland. Reported-by: Flavio Leitner <fbl@sysclose.org> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/netlink/af_netlink.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)