Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Christian Brauner]

On Mon, Jan 29, 2018 at 11:31:57AM -0500, David Miller wrote:
> From: Christian Brauner 
> Date: Wed, 24 Jan 2018 15:26:31 +0100
> 
> > Based on the previous discussion this enables passing a IFLA_IF_NETNSID
> > property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
> > RTM_NEWLINK will be sent out in a separate patch since there are more
> > corner-cases to think about.
>  ...
> > Changelog 2018-01-24:
> > * Preserve old behavior and report -ENODEV when either ifindex or ifname is
> >   provided and IFLA_GROUP is set. Spotted by Wolfgang Bumiller.
> 
> Series applied, and this seems to be consistent with what Jiri envisioned
> in commit 79e1ad148c84 ("rtnetlink: use netnsid to query interface")
> 
> Thank you.

Thanks for applying!
Christian

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[David Miller]

From: Christian Brauner 
Date: Wed, 24 Jan 2018 15:26:31 +0100

> Based on the previous discussion this enables passing a IFLA_IF_NETNSID
> property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
> RTM_NEWLINK will be sent out in a separate patch since there are more
> corner-cases to think about.
 ...
> Changelog 2018-01-24:
> * Preserve old behavior and report -ENODEV when either ifindex or ifname is
>   provided and IFLA_GROUP is set. Spotted by Wolfgang Bumiller.

Series applied, and this seems to be consistent with what Jiri envisioned
in commit 79e1ad148c84 ("rtnetlink: use netnsid to query interface")

Thank you.

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Christian Brauner]

On Thu, Jan 25, 2018 at 01:59:06PM +0100, Christian Brauner wrote:
> On Wed, Jan 24, 2018 at 03:26:31PM +0100, Christian Brauner wrote:
> > Hi,
> > 
> > Based on the previous discussion this enables passing a IFLA_IF_NETNSID
> > property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
> > RTM_NEWLINK will be sent out in a separate patch since there are more
> > corner-cases to think about.
> > 
> > Best,
> > Christian
> > 
> > Changelog 2018-01-24:
> > * Preserve old behavior and report -ENODEV when either ifindex or ifname is
> >   provided and IFLA_GROUP is set. Spotted by Wolfgang Bumiller.
> > 
> > Christian Brauner (3):
> >   rtnetlink: enable IFLA_IF_NETNSID in do_setlink()
> >   rtnetlink: enable IFLA_IF_NETNSID for RTM_SETLINK
> >   rtnetlink: enable IFLA_IF_NETNSID for RTM_DELLINK
> > 
> >  net/core/rtnetlink.c | 96 
> > 
> >  1 file changed, 75 insertions(+), 21 deletions(-)
> > 
> > -- 
> > 2.14.1
> > 
> 
> I've done more testing around enabling IFLA_IF_NETNSID for RTM_NEWLINK
> as well and I think that the change is actually trivial. However, I
> would like to wait before sending this patch out until we have agreed on
> the patches for RTM_SETLINK and RTM_DELLINK in this series. Once we have
> merged those I can just send another small commit. Or - if changes to
> this patch series are required - I can just add it in a v2.

Jiri, do you want that patch resent with the small commit for
RTM_NEWLINK included or are you fine with sending it after this series?

Thanks!
Christian

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Nicolas Dichtel]

Le 26/01/2018 à 09:36, Jiri Benc a écrit :
> On Fri, 26 Jan 2018 00:34:51 +0100, Nicolas Dichtel wrote:
>> Why meaningful? The user knows that the answer is like if if was done in 
>> another
>> netns. It enables to have only one netlink socket instead of one per netns. 
>> But
>> the code using it will be the same.  
> 
> Because you can't use it to query the linked interface. You can't even
> use it as an opaque value to track interfaces (netnsid+ifindex) because
> netnsids are not unique across net name spaces. You can easily have two
> interfaces that have all the ifindex, ifname, netnsid (and basically
> everything else) identical but being completely different interfaces.
Yes, the user have to map those info correctly. And this complexifies the (user)
code a lot.

> That's really not helpful.
> 
>> I fear that with your approach, it will results to a lot of complexity in the
>> kernel.  
> 
> The complexity is (at least partly) already there. It's an inevitable
> result of the design decision to have relative identifiers.
Yes, you're right. My approach moves the complexity to the user, which make this
feature hard to use.

> 
> I agree that we should think about how to make this easy to implement.
> I like your idea of doing this somehow generically. Perhaps it's
> possible to do while keeping the netnsids valid in the caller's netns?
Yes. I agree that it will be a lot easier to use if the conversion is done in
the kernel. And having a generic mechanism will also help a lot to use it.

> 
>> What is really missing for me, is a way to get a fd from an nsid. The user
>> should be able to call RTM_GETNSID with an fd and a nsid and the kernel 
>> performs
>> the needed operations so that the fd points to the corresponding netns.  
> 
> That's what I was missing, too. I even looked into implementing it. But
> opening a fd on behalf of the process and returning it over netlink is a
> wrong thing to do. Netlink messages can get lost. Then you have a fd
> leak you can do nothing about.
Yes, I also looked at this ;-)

> 
> Given that we have netnsids used for so much stuff already (like
> NETLINK_LISTEN_ALL_NSID) you need to track them anyway. And if you need
> to track them, why bother with another identifier? It would be better
> if netnsid can be used universally for anything. Then there will be no
> need for the conversion.
I like this idea a lot. So the missing part is a setns() using the nsid ;-)


Regards,
Nicolas

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Jiri Benc]

On Fri, 26 Jan 2018 00:34:51 +0100, Nicolas Dichtel wrote:
> Why meaningful? The user knows that the answer is like if if was done in 
> another
> netns. It enables to have only one netlink socket instead of one per netns. 
> But
> the code using it will be the same.  

Because you can't use it to query the linked interface. You can't even
use it as an opaque value to track interfaces (netnsid+ifindex) because
netnsids are not unique across net name spaces. You can easily have two
interfaces that have all the ifindex, ifname, netnsid (and basically
everything else) identical but being completely different interfaces.
That's really not helpful.

> I fear that with your approach, it will results to a lot of complexity in the
> kernel.  

The complexity is (at least partly) already there. It's an inevitable
result of the design decision to have relative identifiers.

I agree that we should think about how to make this easy to implement.
I like your idea of doing this somehow generically. Perhaps it's
possible to do while keeping the netnsids valid in the caller's netns?

> What is really missing for me, is a way to get a fd from an nsid. The user
> should be able to call RTM_GETNSID with an fd and a nsid and the kernel 
> performs
> the needed operations so that the fd points to the corresponding netns.  

That's what I was missing, too. I even looked into implementing it. But
opening a fd on behalf of the process and returning it over netlink is a
wrong thing to do. Netlink messages can get lost. Then you have a fd
leak you can do nothing about.

Given that we have netnsids used for so much stuff already (like
NETLINK_LISTEN_ALL_NSID) you need to track them anyway. And if you need
to track them, why bother with another identifier? It would be better
if netnsid can be used universally for anything. Then there will be no
need for the conversion.

 Jiri

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Nicolas Dichtel]

Le 25/01/2018 à 23:30, Jiri Benc a écrit :
> On Thu, 25 Jan 2018 15:20:59 +0100, Nicolas Dichtel wrote:
>> Hmm, I don't agree. For me, it would be the correct answer. If user has a 
>> socket
>> in ns_a and targets a RTM_GETLINK in ns_b, the answer he gets should be like 
>> if
>> it was done in ns_b.
> 
> But that information would be useless for the caller. Why return a
> value that has no meaning for the caller and can not be used? More so
> when the kernel is aware of what the correct meaningful value is?
Why meaningful? The user knows that the answer is like if if was done in another
netns. It enables to have only one netlink socket instead of one per netns. But
the code using it will be the same.
I fear that with your approach, it will results to a lot of complexity in the
kernel.

> 
>> This is already the case with messages received with NETLINK_LISTEN_ALL_NSID,
>> there is no reason to do something different.
> 
> NETLINK_LISTEN_ALL_NSID is tough due to way it is implemented. But yes,
> it should translate the netnsids to be valid in the socket's netns.
> That's the only sane way for the listener.
A listener that uses this option should know the details about each netns it
listens. Thus, he has no problem to interpret the answer.

What is really missing for me, is a way to get a fd from an nsid. The user
should be able to call RTM_GETNSID with an fd and a nsid and the kernel performs
the needed operations so that the fd points to the corresponding netns.

Nicolas

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Jiri Benc]

On Thu, 25 Jan 2018 15:20:59 +0100, Nicolas Dichtel wrote:
> Hmm, I don't agree. For me, it would be the correct answer. If user has a 
> socket
> in ns_a and targets a RTM_GETLINK in ns_b, the answer he gets should be like 
> if
> it was done in ns_b.

But that information would be useless for the caller. Why return a
value that has no meaning for the caller and can not be used? More so
when the kernel is aware of what the correct meaningful value is?

> This is already the case with messages received with NETLINK_LISTEN_ALL_NSID,
> there is no reason to do something different.

NETLINK_LISTEN_ALL_NSID is tough due to way it is implemented. But yes,
it should translate the netnsids to be valid in the socket's netns.
That's the only sane way for the listener.

 Jiri

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Nicolas Dichtel]

Le 24/01/2018 à 17:35, Jiri Benc a écrit :
> On Wed, 24 Jan 2018 16:24:34 +0100, Nicolas Dichtel wrote:
>> I wonder if it would be possible to do something in the netlink framework, 
>> like
>> NETLINK_LISTEN_ALL_NSID.
>> Having some ancillary data at the netlink socket level and a function like
>> nlsock_net() (instead of sock_net()) to get the corresponding netns.
>> With that, it would be possible, in a generci way, to support this feature 
>> for
>> all netlink family.
> 
> I'm not sure it's worth the effort to do that in the framework. You'll
> need modifications all the way down to the code that generates the
> attributes anyway.
> 
> It's not enough to just specify that the operation should be done on a
> different netns and hide that from the handlers. Take for example the
> existing RTM_GETLINK. Let's say it's executed from within ns_a and
> targeted to ns_b (thus IFLA_IF_NETNSID = netnsid of ns_b). Now, if
> there's a veth interface in ns_b whose other end is in ns_c, there will
> be IFLA_LINK_NETNSID attribute in the response. But the value has to be
> netnsid of ns_c as seen from *ns_a*. If you just pretended to switch to
> ns_b before invoking rtnl_getlink, it would be netnsid of ns_c as seen
> from ns_b which would be wrong.
Hmm, I don't agree. For me, it would be the correct answer. If user has a socket
in ns_a and targets a RTM_GETLINK in ns_b, the answer he gets should be like if
it was done in ns_b.
This is already the case with messages received with NETLINK_LISTEN_ALL_NSID,
there is no reason to do something different.


Regards,
Nicolas

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Christian Brauner]

On Wed, Jan 24, 2018 at 03:26:31PM +0100, Christian Brauner wrote:
> Hi,
> 
> Based on the previous discussion this enables passing a IFLA_IF_NETNSID
> property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
> RTM_NEWLINK will be sent out in a separate patch since there are more
> corner-cases to think about.
> 
> Best,
> Christian
> 
> Changelog 2018-01-24:
> * Preserve old behavior and report -ENODEV when either ifindex or ifname is
>   provided and IFLA_GROUP is set. Spotted by Wolfgang Bumiller.
> 
> Christian Brauner (3):
>   rtnetlink: enable IFLA_IF_NETNSID in do_setlink()
>   rtnetlink: enable IFLA_IF_NETNSID for RTM_SETLINK
>   rtnetlink: enable IFLA_IF_NETNSID for RTM_DELLINK
> 
>  net/core/rtnetlink.c | 96 
> 
>  1 file changed, 75 insertions(+), 21 deletions(-)
> 
> -- 
> 2.14.1
> 

I've done more testing around enabling IFLA_IF_NETNSID for RTM_NEWLINK
as well and I think that the change is actually trivial. However, I
would like to wait before sending this patch out until we have agreed on
the patches for RTM_SETLINK and RTM_DELLINK in this series. Once we have
merged those I can just send another small commit. Or - if changes to
this patch series are required - I can just add it in a v2.

Thanks
Christian

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Jiri Benc]

On Wed, 24 Jan 2018 16:24:34 +0100, Nicolas Dichtel wrote:
> I wonder if it would be possible to do something in the netlink framework, 
> like
> NETLINK_LISTEN_ALL_NSID.
> Having some ancillary data at the netlink socket level and a function like
> nlsock_net() (instead of sock_net()) to get the corresponding netns.
> With that, it would be possible, in a generci way, to support this feature for
> all netlink family.

I'm not sure it's worth the effort to do that in the framework. You'll
need modifications all the way down to the code that generates the
attributes anyway.

It's not enough to just specify that the operation should be done on a
different netns and hide that from the handlers. Take for example the
existing RTM_GETLINK. Let's say it's executed from within ns_a and
targeted to ns_b (thus IFLA_IF_NETNSID = netnsid of ns_b). Now, if
there's a veth interface in ns_b whose other end is in ns_c, there will
be IFLA_LINK_NETNSID attribute in the response. But the value has to be
netnsid of ns_c as seen from *ns_a*. If you just pretended to switch to
ns_b before invoking rtnl_getlink, it would be netnsid of ns_c as seen
from ns_b which would be wrong.

That's why 79e1ad148c844 added the tgt_net stuff.

 Jiri

Re: [PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Nicolas Dichtel]

Hi,

Le 24/01/2018 à 15:26, Christian Brauner a écrit :
> Hi,
> 
> Based on the previous discussion this enables passing a IFLA_IF_NETNSID
> property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
> RTM_NEWLINK will be sent out in a separate patch since there are more
> corner-cases to think about.
I wonder if it would be possible to do something in the netlink framework, like
NETLINK_LISTEN_ALL_NSID.
Having some ancillary data at the netlink socket level and a function like
nlsock_net() (instead of sock_net()) to get the corresponding netns.
With that, it would be possible, in a generci way, to support this feature for
all netlink family.


Regards,
Nicolas

[PATCH net-next 0/3 V1] rtnetlink: enable IFLA_IF_NETNSID for RTM_{DEL,SET}LINK

[Christian Brauner]

Hi,

Based on the previous discussion this enables passing a IFLA_IF_NETNSID
property along with RTM_SETLINK and RTM_DELLINK requests. The patch for
RTM_NEWLINK will be sent out in a separate patch since there are more
corner-cases to think about.

Best,
Christian

Changelog 2018-01-24:
* Preserve old behavior and report -ENODEV when either ifindex or ifname is
  provided and IFLA_GROUP is set. Spotted by Wolfgang Bumiller.

Christian Brauner (3):
  rtnetlink: enable IFLA_IF_NETNSID in do_setlink()
  rtnetlink: enable IFLA_IF_NETNSID for RTM_SETLINK
  rtnetlink: enable IFLA_IF_NETNSID for RTM_DELLINK

 net/core/rtnetlink.c | 96 
 1 file changed, 75 insertions(+), 21 deletions(-)

-- 
2.14.1