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
- Backwards Compatibility:
If userspace wants to determine whether RTM_DELLINK supports the
IFLA_IF_NETNSID property they should first send an RTM_GETLINK request
with IFLA_IF_NETNSID on lo. If either EACCESS is returned or the reply
does not include IFLA_IF_NETNSID userspace should assume that
IFLA_IF_NETNSID is not supported on this kernel.
If the reply does contain an IFLA_IF_NETNSID property userspace
can send an RTM_DELLINK with a IFLA_IF_NETNSID property. If they receive
EOPNOTSUPP then the kernel does not support the IFLA_IF_NETNSID property
with RTM_DELLINK. Userpace should then fallback to other means.
- Security:
Callers must have CAP_NET_ADMIN in the owning user namespace of the
target network namespace.
Signed-off-by: Christian Brauner <[email protected]>
---
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.
---
net/core/rtnetlink.c | 39 ++++++++++++++++++++++++++++-----------
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a4d4409685e3..fdb9e8777abb 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2630,36 +2630,53 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
struct net *net = sock_net(skb->sk);
- struct net_device *dev;
+ struct net *tgt_net = net;
+ struct net_device *dev = NULL;
struct ifinfomsg *ifm;
char ifname[IFNAMSIZ];
struct nlattr *tb[IFLA_MAX+1];
int err;
+ int netnsid = -1;
err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy, extack);
if (err < 0)
return err;
- if (tb[IFLA_IF_NETNSID])
- return -EOPNOTSUPP;
-
if (tb[IFLA_IFNAME])
nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+ if (tb[IFLA_IF_NETNSID]) {
+ netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
+ tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);
+ if (IS_ERR(tgt_net))
+ return PTR_ERR(tgt_net);
+ }
+
+ err = -EINVAL;
ifm = nlmsg_data(nlh);
if (ifm->ifi_index > 0)
- dev = __dev_get_by_index(net, ifm->ifi_index);
+ dev = __dev_get_by_index(tgt_net, ifm->ifi_index);
else if (tb[IFLA_IFNAME])
- dev = __dev_get_by_name(net, ifname);
+ dev = __dev_get_by_name(tgt_net, ifname);
else if (tb[IFLA_GROUP])
- return rtnl_group_dellink(net, nla_get_u32(tb[IFLA_GROUP]));
+ err = rtnl_group_dellink(tgt_net, nla_get_u32(tb[IFLA_GROUP]));
else
- return -EINVAL;
+ goto out;
- if (!dev)
- return -ENODEV;
+ if (!dev) {
+ if (tb[IFLA_IFNAME] || ifm->ifi_index > 0)
+ err = -ENODEV;
- return rtnl_delete_link(dev);
+ goto out;
+ }
+
+ err = rtnl_delete_link(dev);
+
+out:
+ if (netnsid >= 0)
+ put_net(tgt_net);
+
+ return err;
}
int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
--
2.14.1
- Backwards Compatibility:
If userspace wants to determine whether RTM_SETLINK supports the
IFLA_IF_NETNSID property they should first send an RTM_GETLINK request
with IFLA_IF_NETNSID on lo. If either EACCESS is returned or the reply
does not include IFLA_IF_NETNSID userspace should assume that
IFLA_IF_NETNSID is not supported on this kernel.
If the reply does contain an IFLA_IF_NETNSID property userspace
can send an RTM_SETLINK with a IFLA_IF_NETNSID property. If they receive
EOPNOTSUPP then the kernel does not support the IFLA_IF_NETNSID property
with RTM_SETLINK. Userpace should then fallback to other means.
To retain backwards compatibility the kernel will first check whether a
IFLA_NET_NS_PID or IFLA_NET_NS_FD property has been passed. If either
one is found it will be used to identify the target network namespace.
This implies that users who do not care whether their running kernel
supports IFLA_IF_NETNSID with RTM_SETLINK can pass both
IFLA_NET_NS_{FD,PID} and IFLA_IF_NETNSID referring to the same network
namespace.
- Security:
Callers must have CAP_NET_ADMIN in the owning user namespace of the
target network namespace.
Signed-off-by: Christian Brauner <[email protected]>
---
net/core/rtnetlink.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 54134187485b..a4d4409685e3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2546,9 +2546,6 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
if (err < 0)
goto errout;
- if (tb[IFLA_IF_NETNSID])
- return -EOPNOTSUPP;
-
if (tb[IFLA_IFNAME])
nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
else
--
2.14.1
RTM_{NEW,SET}LINK already allow operations on other network namespaces
by identifying the target network namespace through IFLA_NET_NS_{FD,PID}
properties. This is done by looking for the corresponding properties in
do_setlink(). Extend do_setlink() to also look for the IFLA_IF_NETNSID
property. This introduces no functional changes since all callers of
do_setlink() currently block IFLA_IF_NETNSID by reporting an error before
they reach do_setlink().
This introduces the helpers:
static struct net *rtnl_link_get_net_by_nlattr(struct net *src_net, struct
nlattr *tb[])
static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
struct net *src_net,
struct nlattr *tb[], int cap)
to simplify permission checks and target network namespace retrieval for
RTM_* requests that already support IFLA_NET_NS_{FD,PID} but get extended
to IFLA_IF_NETNSID. To perserve backwards compatibility the helpers look
for IFLA_NET_NS_{FD,PID} properties first before checking for
IFLA_IF_NETNSID.
Signed-off-by: Christian Brauner <[email protected]>
---
net/core/rtnetlink.c | 54 +++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 7 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 16d644a4f974..54134187485b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1893,6 +1893,49 @@ struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
}
EXPORT_SYMBOL(rtnl_link_get_net);
+/* Figure out which network namespace we are talking about by
+ * examining the link attributes in the following order:
+ *
+ * 1. IFLA_NET_NS_PID
+ * 2. IFLA_NET_NS_FD
+ * 3. IFLA_IF_NETNSID
+ */
+static struct net *rtnl_link_get_net_by_nlattr(struct net *src_net,
+ struct nlattr *tb[])
+{
+ struct net *net;
+
+ if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD])
+ return rtnl_link_get_net(src_net, tb);
+
+ if (!tb[IFLA_IF_NETNSID])
+ return get_net(src_net);
+
+ net = get_net_ns_by_id(src_net, nla_get_u32(tb[IFLA_IF_NETNSID]));
+ if (!net)
+ return ERR_PTR(-EINVAL);
+
+ return net;
+}
+
+static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
+ struct net *src_net,
+ struct nlattr *tb[], int cap)
+{
+ struct net *net;
+
+ net = rtnl_link_get_net_by_nlattr(src_net, tb);
+ if (IS_ERR(net))
+ return net;
+
+ if (!netlink_ns_capable(skb, net->user_ns, cap)) {
+ put_net(net);
+ return ERR_PTR(-EPERM);
+ }
+
+ return net;
+}
+
static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
{
if (dev) {
@@ -2155,17 +2198,14 @@ static int do_setlink(const struct sk_buff *skb,
const struct net_device_ops *ops = dev->netdev_ops;
int err;
- if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]) {
- struct net *net = rtnl_link_get_net(dev_net(dev), tb);
+ if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD] || tb[IFLA_IF_NETNSID]) {
+ struct net *net = rtnl_link_get_net_capable(skb, dev_net(dev),
+ tb, CAP_NET_ADMIN);
if (IS_ERR(net)) {
err = PTR_ERR(net);
goto errout;
}
- if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
- put_net(net);
- err = -EPERM;
- goto errout;
- }
+
err = dev_change_net_namespace(dev, net, ifname);
put_net(net);
if (err)
--
2.14.1
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
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
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
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
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
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
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
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
From: Christian Brauner <[email protected]>
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.
On Mon, Jan 29, 2018 at 11:31:57AM -0500, David Miller wrote:
> From: Christian Brauner <[email protected]>
> 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