All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] netlink: include netnsid only when netns differs.
@ 2017-05-30 21:33 Flavio Leitner
  2017-05-31  8:38 ` Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-05-30 21:33 UTC (permalink / raw)
  To: netdev; +Cc: Flavio Leitner

Don't include netns id for notifications broadcasts when the
socket and the skb are in the same netns because it will be
an error which can't be distinguished from a peer netns failing
to allocate an id.

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netlink/af_netlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index ee841f0..b9f1392 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1414,8 +1414,10 @@ static void do_one_broadcast(struct sock *sk,
 		p->skb2 = NULL;
 		goto out;
 	}
-	NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net);
-	NETLINK_CB(p->skb2).nsid_is_set = true;
+	if (!net_eq(sock_net(sk), p->net)) {
+		NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net);
+		NETLINK_CB(p->skb2).nsid_is_set = true;
+	}
 	val = netlink_broadcast_deliver(sk, p->skb2);
 	if (val < 0) {
 		netlink_overrun(sk);
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next] netlink: include netnsid only when netns differs.
  2017-05-30 21:33 [PATCH net-next] netlink: include netnsid only when netns differs Flavio Leitner
@ 2017-05-31  8:38 ` Nicolas Dichtel
  2017-05-31 12:28   ` Flavio Leitner
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-05-31  8:38 UTC (permalink / raw)
  To: Flavio Leitner, netdev

Le 30/05/2017 à 23:33, Flavio Leitner a écrit :
> Don't include netns id for notifications broadcasts when the
> socket and the skb are in the same netns because it will be
> an error which can't be distinguished from a peer netns failing
> to allocate an id.
I don't understand the problem. peernet2id() doesn't allocate ids, it only do a
lookup. If you need an id for the current netns, you have to allocate one.

This patch changes the metadata exported to the userland and will break existing
tools.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next] netlink: include netnsid only when netns differs.
  2017-05-31  8:38 ` Nicolas Dichtel
@ 2017-05-31 12:28   ` Flavio Leitner
  2017-05-31 13:48     ` Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-05-31 12:28 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: netdev

On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote:
> Le 30/05/2017 à 23:33, Flavio Leitner a écrit :
> > Don't include netns id for notifications broadcasts when the
> > socket and the skb are in the same netns because it will be
> > an error which can't be distinguished from a peer netns failing
> > to allocate an id.
> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a
> lookup. If you need an id for the current netns, you have to allocate one.

The issue is that if you query an interface on the same netns, the
error is returned, then we cannot tell if the iface is on the same
netns or if there was an error while allocating the ID and the
iface is on another netns.

> This patch changes the metadata exported to the userland and will break existing
> tools.

It should not break because it changes only for interfaces on
the same netns where there is no ID and that value wasn't
exported until recently.

-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next] netlink: include netnsid only when netns differs.
  2017-05-31 12:28   ` Flavio Leitner
@ 2017-05-31 13:48     ` Nicolas Dichtel
  2017-05-31 18:34       ` Flavio Leitner
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-05-31 13:48 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: netdev

Le 31/05/2017 à 14:28, Flavio Leitner a écrit :
> On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote:
>> Le 30/05/2017 à 23:33, Flavio Leitner a écrit :
>>> Don't include netns id for notifications broadcasts when the
>>> socket and the skb are in the same netns because it will be
>>> an error which can't be distinguished from a peer netns failing
>>> to allocate an id.
>> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a
>> lookup. If you need an id for the current netns, you have to allocate one.
> 
> The issue is that if you query an interface on the same netns, the
> error is returned, then we cannot tell if the iface is on the same
> netns or if there was an error while allocating the ID and the
> iface is on another netns.
If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same.

Some lines before your patch, we call peernet_has_id() when the netns differ,
thus we ensure that the id is available.

The principle was that netlink messages of other netns can be sent only if an id
is assigned.

> 
>> This patch changes the metadata exported to the userland and will break existing
>> tools.
> 
> It should not break because it changes only for interfaces on
> the same netns where there is no ID and that value wasn't
> exported until recently.
> 
It was exported since the initial patch (59324cf35aba ("netlink: allow to listen
"all" netns"). Am I wrong?


Regards,
Nicolas

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next] netlink: include netnsid only when netns differs.
  2017-05-31 13:48     ` Nicolas Dichtel
@ 2017-05-31 18:34       ` Flavio Leitner
  2017-06-01  7:57         ` Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-05-31 18:34 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: netdev

On Wed, May 31, 2017 at 03:48:06PM +0200, Nicolas Dichtel wrote:
> Le 31/05/2017 à 14:28, Flavio Leitner a écrit :
> > On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote:
> >> Le 30/05/2017 à 23:33, Flavio Leitner a écrit :
> >>> Don't include netns id for notifications broadcasts when the
> >>> socket and the skb are in the same netns because it will be
> >>> an error which can't be distinguished from a peer netns failing
> >>> to allocate an id.
> >> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a
> >> lookup. If you need an id for the current netns, you have to allocate one.
> > 
> > The issue is that if you query an interface on the same netns, the
> > error is returned, then we cannot tell if the iface is on the same
> > netns or if there was an error while allocating the ID and the
> > iface is on another netns.
> If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same.
> 
> Some lines before your patch, we call peernet_has_id() when the netns differ,
> thus we ensure that the id is available.

Right, but that's internal to the kernel.

> The principle was that netlink messages of other netns can be sent only if an id
> is assigned.

OK, could you please update include/uapi/linux/net_namespace.h to reflect that?
It says NETNSA_NSID_NOT_ASSIGNED are attributes for RTM_NEWNSID or RTM_GETNSID
which makes sense, but NOT_ASSIGNED sounds little like SAME_NSID for other
message types.

-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next] netlink: include netnsid only when netns differs.
  2017-05-31 18:34       ` Flavio Leitner
@ 2017-06-01  7:57         ` Nicolas Dichtel
  2017-06-01  8:00           ` [PATCH net] netlink: don't send unknown nsid Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-06-01  7:57 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: netdev

Le 31/05/2017 à 20:34, Flavio Leitner a écrit :
> On Wed, May 31, 2017 at 03:48:06PM +0200, Nicolas Dichtel wrote:
>> Le 31/05/2017 à 14:28, Flavio Leitner a écrit :
>>> On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote:
>>>> Le 30/05/2017 à 23:33, Flavio Leitner a écrit :
>>>>> Don't include netns id for notifications broadcasts when the
>>>>> socket and the skb are in the same netns because it will be
>>>>> an error which can't be distinguished from a peer netns failing
>>>>> to allocate an id.
>>>> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a
>>>> lookup. If you need an id for the current netns, you have to allocate one.
>>>
>>> The issue is that if you query an interface on the same netns, the
>>> error is returned, then we cannot tell if the iface is on the same
>>> netns or if there was an error while allocating the ID and the
>>> iface is on another netns.
>> If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same.
>>
>> Some lines before your patch, we call peernet_has_id() when the netns differ,
>> thus we ensure that the id is available.
> 
> Right, but that's internal to the kernel.
Sure, but a good example exists in iproute2:
https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/tree/ip/ipmonitor.c#n45

> 
>> The principle was that netlink messages of other netns can be sent only if an id
>> is assigned.
> 
> OK, could you please update include/uapi/linux/net_namespace.h to reflect that?
> It says NETNSA_NSID_NOT_ASSIGNED are attributes for RTM_NEWNSID or RTM_GETNSID
> which makes sense, but NOT_ASSIGNED sounds little like SAME_NSID for other
> message types.
I agree, it's confusing. I will send a patch.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH net] netlink: don't send unknown nsid
  2017-06-01  7:57         ` Nicolas Dichtel
@ 2017-06-01  8:00           ` Nicolas Dichtel
  2017-06-01 15:50             ` David Miller
  2017-06-01 17:02             ` Flavio Leitner
  0 siblings, 2 replies; 15+ messages in thread
From: Nicolas Dichtel @ 2017-06-01  8:00 UTC (permalink / raw)
  To: davem; +Cc: netdev, fbl, Nicolas Dichtel

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);
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-01  8:00           ` [PATCH net] netlink: don't send unknown nsid Nicolas Dichtel
@ 2017-06-01 15:50             ` David Miller
  2017-06-01 17:02             ` Flavio Leitner
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2017-06-01 15:50 UTC (permalink / raw)
  To: nicolas.dichtel; +Cc: netdev, fbl

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-01  8:00           ` [PATCH net] netlink: don't send unknown nsid Nicolas Dichtel
  2017-06-01 15:50             ` David Miller
@ 2017-06-01 17:02             ` Flavio Leitner
  2017-06-01 20:42               ` Nicolas Dichtel
  1 sibling, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-06-01 17:02 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: davem, netdev

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.

-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-01 17:02             ` Flavio Leitner
@ 2017-06-01 20:42               ` Nicolas Dichtel
  2017-06-01 22:44                 ` Flavio Leitner
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-06-01 20:42 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: davem, netdev

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 ;-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-01 20:42               ` Nicolas Dichtel
@ 2017-06-01 22:44                 ` Flavio Leitner
  2017-06-05  8:40                   ` Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-06-01 22:44 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: davem, netdev

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?

-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-01 22:44                 ` Flavio Leitner
@ 2017-06-05  8:40                   ` Nicolas Dichtel
  2017-06-07 18:40                     ` Flavio Leitner
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-06-05  8:40 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: davem, netdev

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-05  8:40                   ` Nicolas Dichtel
@ 2017-06-07 18:40                     ` Flavio Leitner
  2017-06-08  8:31                       ` Nicolas Dichtel
  0 siblings, 1 reply; 15+ messages in thread
From: Flavio Leitner @ 2017-06-07 18:40 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: davem, netdev

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?

-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-07 18:40                     ` Flavio Leitner
@ 2017-06-08  8:31                       ` Nicolas Dichtel
  2017-06-09 17:33                         ` Flavio Leitner
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Dichtel @ 2017-06-08  8:31 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: davem, netdev

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net] netlink: don't send unknown nsid
  2017-06-08  8:31                       ` Nicolas Dichtel
@ 2017-06-09 17:33                         ` Flavio Leitner
  0 siblings, 0 replies; 15+ messages in thread
From: Flavio Leitner @ 2017-06-09 17:33 UTC (permalink / raw)
  To: Nicolas Dichtel; +Cc: davem, netdev

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.
-- 
Flavio

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-06-09 17:33 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 21:33 [PATCH net-next] netlink: include netnsid only when netns differs Flavio Leitner
2017-05-31  8:38 ` Nicolas Dichtel
2017-05-31 12:28   ` Flavio Leitner
2017-05-31 13:48     ` Nicolas Dichtel
2017-05-31 18:34       ` Flavio Leitner
2017-06-01  7:57         ` Nicolas Dichtel
2017-06-01  8:00           ` [PATCH net] netlink: don't send unknown nsid Nicolas Dichtel
2017-06-01 15:50             ` David Miller
2017-06-01 17:02             ` Flavio Leitner
2017-06-01 20:42               ` Nicolas Dichtel
2017-06-01 22:44                 ` Flavio Leitner
2017-06-05  8:40                   ` Nicolas Dichtel
2017-06-07 18:40                     ` Flavio Leitner
2017-06-08  8:31                       ` Nicolas Dichtel
2017-06-09 17:33                         ` Flavio Leitner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.