[PATCH v3 0/6] Add namespace awareness to Netlink methods

[PATCH v3 0/6] Add namespace awareness to Netlink methods

[Jonas Bonn]

Changed in v3:
- added patch 6 for setting IPv6 address outside current namespace
- address checkpatch warnings
- address comment from Nicolas

Changed in v2:
- address comment from Nicolas
- add accumulated ACK's

Currently, Netlink has partial support for acting outside of the current
namespace.  It appears that the intention was to extend this to all the
methods eventually, but it hasn't been done to date.

With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
are extended to respect the selection of the namespace to work in.

/Jonas

Jonas Bonn (6):
  rtnetlink: allow RTM_SETLINK to reference other namespaces
  rtnetlink: skip namespace change if already effect
  rtnetlink: allow RTM_NEWLINK to act upon interfaces in arbitrary
    namespaces
  net: ipv4: allow setting address on interface outside current
    namespace
  net: namespace: allow setting NSIDs outside current namespace
  net: ipv6: allow setting address on interface outside current
    namespace

 net/core/net_namespace.c | 19 ++++++++++
 net/core/rtnetlink.c     | 80 ++++++++++++++++++++++++++++++++++------
 net/ipv4/devinet.c       | 61 ++++++++++++++++++++++--------
 net/ipv6/addrconf.c      | 13 +++++++
 4 files changed, 145 insertions(+), 28 deletions(-)

-- 
2.20.1

[PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Jonas Bonn]

Netlink currently has partial support for acting on interfaces outside
the current namespace.  This patch extends RTM_SETLINK with this
functionality.

The current implementation has an unfortunate semantic ambiguity in the
IFLA_TARGET_NETNSID attribute.  For setting the interface namespace, one
may pass the IFLA_TARGET_NETNSID attribute with the namespace to move the
interface to.  This conflicts with the meaning of this attribute for all
other methods where IFLA_TARGET_NETNSID identifies the namespace in
which to search for the interface to act upon:  the pair (namespace,
ifindex) is generally given by (IFLA_TARGET_NETNSID, ifi->ifi_index).

In order to change the namespace of an interface outside the current
namespace, we would need to specify both an IFLA_TARGET_NETNSID
attribute and a namespace to move to using IFLA_NET_NS_[PID|FD].  This is
currently now allowed as only one of these three flags may be specified.

This patch loosens the restrictions a bit but tries to maintain
compatibility with the previous behaviour:
i)  IFLA_TARGET_NETNSID may be passed together with one of
IFLA_NET_NS_[PID|FD]
ii)  IFLA_TARGET_NETNSID is primarily defined to be the namespace in
which to find the interface to act upon
iii)  In order to maintain backwards compatibility, if the device is not
found in the specified namespace, we also look for it in the current
namespace
iv)  If only IFLA_TARGET_NETNSID is given, the device is still moved to
that namespace, as before; and, as before, IFLA_NET_NS_[PID|FD] take
precedence as namespace selectors

Ideally, IFLA_TARGET_NETNSID would only ever have been used to select the
namespace of the device to act upon.  A separate flag, IFLA_NET_NS_ID
would have been made available for changing namespaces

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c81cd80114d9..aa3924c9813c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2109,13 +2109,7 @@ static int rtnl_ensure_unique_netns(struct nlattr *tb[],
 		return -EOPNOTSUPP;
 	}
 
-	if (tb[IFLA_TARGET_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]))
-		goto invalid_attr;
-
-	if (tb[IFLA_NET_NS_PID] && (tb[IFLA_TARGET_NETNSID] || tb[IFLA_NET_NS_FD]))
-		goto invalid_attr;
-
-	if (tb[IFLA_NET_NS_FD] && (tb[IFLA_TARGET_NETNSID] || tb[IFLA_NET_NS_PID]))
+	if (tb[IFLA_NET_NS_PID] && tb[IFLA_NET_NS_FD])
 		goto invalid_attr;
 
 	return 0;
@@ -2727,6 +2721,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
+	struct net *tgt_net = NULL;
 	struct ifinfomsg *ifm;
 	struct net_device *dev;
 	int err;
@@ -2742,6 +2737,15 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
+	if (tb[IFLA_TARGET_NETNSID]) {
+		s32 netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
+
+		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
+		if (IS_ERR(net))
+			return PTR_ERR(net);
+		net = tgt_net;
+	}
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -2756,6 +2760,23 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else
 		goto errout;
 
+	/* A hack to preserve kernel<->userspace interface.
+	 * It was previously allowed to pass the IFLA_TARGET_NETNSID
+	 * attribute as a way to _set_ the network namespace.  In this
+	 * case, the device interface was assumed to be in the  _current_
+	 * namespace.
+	 * If the device cannot be found in the target namespace then we
+	 * assume that the request is to set the device in the current
+	 * namespace and thus we attempt to find the device there.
+	 */
+	if (!dev && tgt_net) {
+		net = sock_net(skb->sk);
+		if (ifm->ifi_index > 0)
+			dev = __dev_get_by_index(net, ifm->ifi_index);
+		else if (tb[IFLA_IFNAME])
+			dev = __dev_get_by_name(net, ifname);
+	}
+
 	if (dev == NULL) {
 		err = -ENODEV;
 		goto errout;
@@ -2763,6 +2784,8 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	err = do_setlink(skb, dev, ifm, extack, tb, ifname, 0);
 errout:
+	if (tgt_net)
+		put_net(tgt_net);
 	return err;
 }
 
-- 
2.20.1

[PATCH v3 2/6] rtnetlink: skip namespace change if already effect

[Jonas Bonn]

RTM_SETLINK uses IFA_TARGET_NETNSID both as a selector for the device to
act upon and as a selection of the namespace to move a device in the
current namespace to.  As such, one ends up in the code path for setting
the namespace every time one calls setlink on a device outside the
current namespace.  This has the unfortunate side effect of setting the
'modified' flag on the device for every pass, resulting in Netlink
notifications even when nothing was changed.

This patch just makes the namespace switch dependent upon the namespace
the device currently resides in.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index aa3924c9813c..a21e7d47135b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2394,11 +2394,15 @@ static int do_setlink(const struct sk_buff *skb,
 			goto errout;
 		}
 
-		err = dev_change_net_namespace(dev, net, ifname);
-		put_net(net);
-		if (err)
-			goto errout;
-		status |= DO_SETLINK_MODIFIED;
+		if (!net_eq(dev_net(dev), net)) {
+			err = dev_change_net_namespace(dev, net, ifname);
+			put_net(net);
+			if (err)
+				goto errout;
+			status |= DO_SETLINK_MODIFIED;
+		} else {
+			put_net(net);
+		}
 	}
 
 	if (tb[IFLA_MAP]) {
-- 
2.20.1

[PATCH v3 3/6] rtnetlink: allow RTM_NEWLINK to act upon interfaces in arbitrary namespaces

[Jonas Bonn]

RTM_NEWLINK can be used mostly interchangeably with RTM_SETLINK for
modifying device configuration.  As such, this method requires the same
logic as RTM_SETLINK for finding the device to act on.

With this patch, the IFLA_TARGET_NETNSID selects the namespace in which
to search for the interface to act upon.  This allows, for example, to
set the namespace of an interface outside the current namespace by
selecting it with the (IFLA_TARGET_NETNSID,ifi->ifi_index) pair and
specifying the namespace with one of IFLA_NET_NS_[PID|FD].

Since rtnl_newlink branches off into do_setlink, we need to provide the
same backwards compatibility check as we do for RTM_SETLINK:  if the
device is not found in the namespace given by IFLA_TARGET_NETNSID then
we search for it in the current namespace.  If found there, it's
namespace will be changed, as before.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a21e7d47135b..bcfabda3fc73 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3021,6 +3021,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	const struct rtnl_link_ops *m_ops = NULL;
 	struct net_device *master_dev = NULL;
 	struct net *net = sock_net(skb->sk);
+	struct net *tgt_net = NULL;
 	const struct rtnl_link_ops *ops;
 	struct nlattr *tb[IFLA_MAX + 1];
 	struct net *dest_net, *link_net;
@@ -3049,6 +3050,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else
 		ifname[0] = '\0';
 
+	if (tb[IFLA_TARGET_NETNSID]) {
+		s32 netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
+
+		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
+		if (IS_ERR(tgt_net))
+			return PTR_ERR(tgt_net);
+		net = tgt_net;
+	}
+
 	ifm = nlmsg_data(nlh);
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(net, ifm->ifi_index);
@@ -3059,6 +3069,23 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			dev = NULL;
 	}
 
+	/* A hack to preserve kernel<->userspace interface.
+	 * It was previously allowed to pass the IFLA_TARGET_NETNSID
+	 * attribute as a way to _set_ the network namespace.  In this
+	 * case, the device interface was assumed to be in the  _current_
+	 * namespace.
+	 * If the device cannot be found in the target namespace then we
+	 * assume that the request is to set the device in the current
+	 * namespace and thus we attempt to find the device there.
+	 */
+	if (!dev && tgt_net) {
+		net = sock_net(skb->sk);
+		if (ifm->ifi_index > 0)
+			dev = __dev_get_by_index(net, ifm->ifi_index);
+		else if (tb[IFLA_IFNAME])
+			dev = __dev_get_by_name(net, ifname);
+	}
+
 	if (dev) {
 		master_dev = netdev_master_upper_dev_get(dev);
 		if (master_dev)
@@ -3253,6 +3280,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto out_unregister;
 	}
 out:
+	if (tgt_net)
+		put_net(tgt_net);
 	if (link_net)
 		put_net(link_net);
 	put_net(dest_net);
-- 
2.20.1

[PATCH v3 4/6] net: ipv4: allow setting address on interface outside current namespace

[Jonas Bonn]

This patch allows an interface outside of the current namespace to be
selected when setting a new IPv4 address for a device.  This uses the
IFA_TARGET_NETNSID attribute to select the namespace in which to search
for the interface to act upon.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
---
 net/ipv4/devinet.c | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index a4b5bd4d2c89..eecd1b0bb7d2 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -813,22 +813,17 @@ static void set_ifa_lifetime(struct in_ifaddr *ifa, __u32 valid_lft,
 		ifa->ifa_cstamp = ifa->ifa_tstamp;
 }
 
-static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,
+static struct in_ifaddr *rtm_to_ifaddr(struct nlattr *tb[],
+				       struct net *net, struct nlmsghdr *nlh,
 				       __u32 *pvalid_lft, __u32 *pprefered_lft,
 				       struct netlink_ext_ack *extack)
 {
-	struct nlattr *tb[IFA_MAX+1];
 	struct in_ifaddr *ifa;
 	struct ifaddrmsg *ifm;
 	struct net_device *dev;
 	struct in_device *in_dev;
 	int err;
 
-	err = nlmsg_parse_deprecated(nlh, sizeof(*ifm), tb, IFA_MAX,
-				     ifa_ipv4_policy, extack);
-	if (err < 0)
-		goto errout;
-
 	ifm = nlmsg_data(nlh);
 	err = -EINVAL;
 	if (ifm->ifa_prefixlen > 32 || !tb[IFA_LOCAL])
@@ -922,16 +917,38 @@ static int inet_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 			    struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
+	struct net *tgt_net = NULL;
 	struct in_ifaddr *ifa;
 	struct in_ifaddr *ifa_existing;
 	__u32 valid_lft = INFINITY_LIFE_TIME;
 	__u32 prefered_lft = INFINITY_LIFE_TIME;
+	struct nlattr *tb[IFA_MAX + 1];
+	int err;
 
 	ASSERT_RTNL();
 
-	ifa = rtm_to_ifaddr(net, nlh, &valid_lft, &prefered_lft, extack);
-	if (IS_ERR(ifa))
-		return PTR_ERR(ifa);
+	err = nlmsg_parse_deprecated(nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
+				     ifa_ipv4_policy, extack);
+	if (err < 0)
+		return err;
+
+	if (tb[IFA_TARGET_NETNSID]) {
+		s32 netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+
+		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
+		if (IS_ERR(tgt_net)) {
+			NL_SET_ERR_MSG(extack,
+				"ipv4: Invalid target network namespace id");
+			return PTR_ERR(tgt_net);
+		}
+		net = tgt_net;
+	}
+
+	ifa = rtm_to_ifaddr(tb, net, nlh, &valid_lft, &prefered_lft, extack);
+	if (IS_ERR(ifa)) {
+		err = PTR_ERR(ifa);
+		goto out;
+	}
 
 	ifa_existing = find_matching_ifa(ifa);
 	if (!ifa_existing) {
@@ -945,19 +962,24 @@ static int inet_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 			if (ret < 0) {
 				inet_free_ifa(ifa);
-				return ret;
+				err = ret;
+				goto out;
 			}
 		}
-		return __inet_insert_ifa(ifa, nlh, NETLINK_CB(skb).portid,
-					 extack);
+		err = __inet_insert_ifa(ifa, nlh, NETLINK_CB(skb).portid,
+					extack);
+		if (err < 0)
+			goto out;
 	} else {
 		u32 new_metric = ifa->ifa_rt_priority;
 
 		inet_free_ifa(ifa);
 
 		if (nlh->nlmsg_flags & NLM_F_EXCL ||
-		    !(nlh->nlmsg_flags & NLM_F_REPLACE))
-			return -EEXIST;
+		    !(nlh->nlmsg_flags & NLM_F_REPLACE)) {
+			err = -EEXIST;
+			goto out;
+		}
 		ifa = ifa_existing;
 
 		if (ifa->ifa_rt_priority != new_metric) {
@@ -971,7 +993,14 @@ static int inet_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 				&check_lifetime_work, 0);
 		rtmsg_ifa(RTM_NEWADDR, ifa, nlh, NETLINK_CB(skb).portid);
 	}
-	return 0;
+
+	err = 0;
+
+out:
+	if (tgt_net)
+		put_net(tgt_net);
+
+	return err;
 }
 
 /*
-- 
2.20.1

[PATCH v3 5/6] net: namespace: allow setting NSIDs outside current namespace

[Jonas Bonn]

Currently it is only possible to move an interface to a new namespace if
the destination namespace has an ID in the interface's current namespace.
If the interface already resides outside of the current namespace, then
we may need to assign the destination namespace an ID in the interface's
namespace in order to effect the move.

This patch allows namespace ID's to be created outside of the current
namespace.  With this, the following is possible:

i)    Our namespace is 'A'.
ii)   The interface resides in namespace 'B'
iii)  We can assign an ID for NS 'A' in NS 'B'
iv)   We can then move the interface into our own namespace.

and

i)   Our namespace is 'A'; namespaces 'B' and 'C' also exist
ii)  We can assign an ID for namespace 'C' in namespace 'B'
iii) We can then create a VETH interface directly in namespace 'B' with
the other end in 'C', all without ever leaving namespace 'A'

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/net_namespace.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 39402840025e..ebb01903d1f7 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -726,6 +726,7 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct nlattr *tb[NETNSA_MAX + 1];
 	struct nlattr *nla;
 	struct net *peer;
+	struct net *target = NULL;
 	int nsid, err;
 
 	err = nlmsg_parse_deprecated(nlh, sizeof(struct rtgenmsg), tb,
@@ -754,6 +755,21 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return PTR_ERR(peer);
 	}
 
+	if (tb[NETNSA_TARGET_NSID]) {
+		int id = nla_get_s32(tb[NETNSA_TARGET_NSID]);
+
+		target = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, id);
+		if (IS_ERR(target)) {
+			NL_SET_BAD_ATTR(extack, tb[NETNSA_TARGET_NSID]);
+			NL_SET_ERR_MSG(extack,
+				       "Target netns reference is invalid");
+			err = PTR_ERR(target);
+			goto out;
+		}
+
+		net = target;
+	}
+
 	spin_lock_bh(&net->nsid_lock);
 	if (__peernet2id(net, peer) >= 0) {
 		spin_unlock_bh(&net->nsid_lock);
@@ -775,6 +791,9 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
 		NL_SET_BAD_ATTR(extack, tb[NETNSA_NSID]);
 		NL_SET_ERR_MSG(extack, "The specified nsid is already used");
 	}
+
+	if (target)
+		put_net(target);
 out:
 	put_net(peer);
 	return err;
-- 
2.20.1

[PATCH v3 6/6] net: ipv6: allow setting address on interface outside current namespace

[Jonas Bonn]

This patch allows an interface outside of the current namespace to be
selected when setting a new IPv6 address for a device.  This uses the
IFA_TARGET_NETNSID attribute to select the namespace in which to search
for the interface to act upon.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
---
 net/ipv6/addrconf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 34ccef18b40e..06a49670fe62 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4721,6 +4721,7 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		  struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
+	struct net *tgt_net;
 	struct ifaddrmsg *ifm;
 	struct nlattr *tb[IFA_MAX+1];
 	struct in6_addr *peer_pfx;
@@ -4758,6 +4759,18 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		cfg.preferred_lft = ci->ifa_prefered;
 	}
 
+	if (tb[IFA_TARGET_NETNSID]) {
+		s32 netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+
+		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
+		if (IS_ERR(tgt_net)) {
+			NL_SET_ERR_MSG(extack,
+				"ipv6: Invalid target network namespace id");
+			return PTR_ERR(tgt_net);
+		}
+		net = tgt_net;
+	}
+
 	dev =  __dev_get_by_index(net, ifm->ifa_index);
 	if (!dev)
 		return -ENODEV;
-- 
2.20.1

[PATCH v3 1/1] net: ipv6: allow setting address on interface outside current namespace

[Jonas Bonn]

This patch allows an interface outside of the current namespace to be
selected when setting a new IPv6 address for a device.  This uses the
IFA_TARGET_NETNSID attribute to select the namespace in which to search
for the interface to act upon.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
---

I messed up this patch and the cleanup code path wasn't included.  It
should look like this.  Sorry for the noise.

/Jonas

 net/ipv6/addrconf.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 34ccef18b40e..8ef8297db150 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4721,6 +4721,7 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		  struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
+	struct net *tgt_net = NULL;
 	struct ifaddrmsg *ifm;
 	struct nlattr *tb[IFA_MAX+1];
 	struct in6_addr *peer_pfx;
@@ -4758,9 +4759,23 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		cfg.preferred_lft = ci->ifa_prefered;
 	}
 
+	if (tb[IFA_TARGET_NETNSID]) {
+		s32 netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+
+		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
+		if (IS_ERR(tgt_net)) {
+			NL_SET_ERR_MSG(extack,
+				"ipv6: Invalid target network namespace id");
+			return PTR_ERR(tgt_net);
+		}
+		net = tgt_net;
+	}
+
 	dev =  __dev_get_by_index(net, ifm->ifa_index);
-	if (!dev)
-		return -ENODEV;
+	if (!dev) {
+		err = -ENODEV;
+		goto out;
+	}
 
 	if (tb[IFA_FLAGS])
 		cfg.ifa_flags = nla_get_u32(tb[IFA_FLAGS]);
@@ -4773,8 +4788,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 			 IFA_F_MCAUTOJOIN | IFA_F_OPTIMISTIC;
 
 	idev = ipv6_find_idev(dev);
-	if (IS_ERR(idev))
-		return PTR_ERR(idev);
+	if (IS_ERR(idev)) {
+		err = PTR_ERR(idev);
+		goto out;
+	}
 
 	if (!ipv6_allow_optimistic_dad(net, idev))
 		cfg.ifa_flags &= ~IFA_F_OPTIMISTIC;
@@ -4782,7 +4799,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (cfg.ifa_flags & IFA_F_NODAD &&
 	    cfg.ifa_flags & IFA_F_OPTIMISTIC) {
 		NL_SET_ERR_MSG(extack, "IFA_F_NODAD and IFA_F_OPTIMISTIC are mutually exclusive");
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;
 	}
 
 	ifa = ipv6_get_ifaddr(net, cfg.pfx, dev, 1);
@@ -4791,7 +4809,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		 * It would be best to check for !NLM_F_CREATE here but
 		 * userspace already relies on not having to provide this.
 		 */
-		return inet6_addr_add(net, ifm->ifa_index, &cfg, extack);
+		err = inet6_addr_add(net, ifm->ifa_index, &cfg, extack);
+		goto out;
 	}
 
 	if (nlh->nlmsg_flags & NLM_F_EXCL ||
@@ -4802,6 +4821,9 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	in6_ifa_put(ifa);
 
+out:
+	if (tgt_net)
+		put_net(tgt_net);
 	return err;
 }
 
-- 
2.20.1

Re: [PATCH v3 1/1] net: ipv6: allow setting address on interface outside current namespace

[Nicolas Dichtel]

Le 07/11/2019 à 14:56, Jonas Bonn a écrit :
> This patch allows an interface outside of the current namespace to be
> selected when setting a new IPv6 address for a device.  This uses the
> IFA_TARGET_NETNSID attribute to select the namespace in which to search
> for the interface to act upon.
> 
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> ---
> 
> I messed up this patch and the cleanup code path wasn't included.  It
> should look like this.  Sorry for the noise.
You cannot resend just one patch, you have to resend the whole series.

Re: [PATCH v3 0/6] Add namespace awareness to Netlink methods

[David Miller]

From: Jonas Bonn <jonas@norrbonn.se>
Date: Thu,  7 Nov 2019 14:27:49 +0100

> Changed in v3:
> - added patch 6 for setting IPv6 address outside current namespace
> - address checkpatch warnings
> - address comment from Nicolas
> 
> Changed in v2:
> - address comment from Nicolas
> - add accumulated ACK's
> 
> Currently, Netlink has partial support for acting outside of the current
> namespace.  It appears that the intention was to extend this to all the
> methods eventually, but it hasn't been done to date.
> 
> With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
> are extended to respect the selection of the namespace to work in.

This patch series does not apply cleanly to net-next, please respin.

I think v2 had this problem too.

Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Mahesh Bandewar (महेश बंडेवार)]

On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Netlink currently has partial support for acting on interfaces outside
> the current namespace.  This patch extends RTM_SETLINK with this
> functionality.
>
> The current implementation has an unfortunate semantic ambiguity in the
> IFLA_TARGET_NETNSID attribute.  For setting the interface namespace, one
> may pass the IFLA_TARGET_NETNSID attribute with the namespace to move the
> interface to.  This conflicts with the meaning of this attribute for all
> other methods where IFLA_TARGET_NETNSID identifies the namespace in
> which to search for the interface to act upon:  the pair (namespace,
> ifindex) is generally given by (IFLA_TARGET_NETNSID, ifi->ifi_index).
>
> In order to change the namespace of an interface outside the current
> namespace, we would need to specify both an IFLA_TARGET_NETNSID
> attribute and a namespace to move to using IFLA_NET_NS_[PID|FD].  This is
> currently now allowed as only one of these three flags may be specified.
>
> This patch loosens the restrictions a bit but tries to maintain
> compatibility with the previous behaviour:
> i)  IFLA_TARGET_NETNSID may be passed together with one of
> IFLA_NET_NS_[PID|FD]
> ii)  IFLA_TARGET_NETNSID is primarily defined to be the namespace in
> which to find the interface to act upon
> iii)  In order to maintain backwards compatibility, if the device is not
> found in the specified namespace, we also look for it in the current
> namespace
> iv)  If only IFLA_TARGET_NETNSID is given, the device is still moved to
> that namespace, as before; and, as before, IFLA_NET_NS_[PID|FD] take
> precedence as namespace selectors
>
> Ideally, IFLA_TARGET_NETNSID would only ever have been used to select the
> namespace of the device to act upon.  A separate flag, IFLA_NET_NS_ID
> would have been made available for changing namespaces
>
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/core/rtnetlink.c | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index c81cd80114d9..aa3924c9813c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2109,13 +2109,7 @@ static int rtnl_ensure_unique_netns(struct nlattr *tb[],
>                 return -EOPNOTSUPP;
>         }
>
> -       if (tb[IFLA_TARGET_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]))
> -               goto invalid_attr;
> -
> -       if (tb[IFLA_NET_NS_PID] && (tb[IFLA_TARGET_NETNSID] || tb[IFLA_NET_NS_FD]))
> -               goto invalid_attr;
> -
> -       if (tb[IFLA_NET_NS_FD] && (tb[IFLA_TARGET_NETNSID] || tb[IFLA_NET_NS_PID]))
> +       if (tb[IFLA_NET_NS_PID] && tb[IFLA_NET_NS_FD])
>                 goto invalid_attr;
>
>         return 0;
> @@ -2727,6 +2721,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>                         struct netlink_ext_ack *extack)
>  {
>         struct net *net = sock_net(skb->sk);
> +       struct net *tgt_net = NULL;
>         struct ifinfomsg *ifm;
>         struct net_device *dev;
>         int err;
> @@ -2742,6 +2737,15 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>         if (err < 0)
>                 goto errout;
>
> +       if (tb[IFLA_TARGET_NETNSID]) {
> +               s32 netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
> +
> +               tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, netnsid);
> +               if (IS_ERR(net))
> +                       return PTR_ERR(net);
> +               net = tgt_net;
> +       }
> +
>         if (tb[IFLA_IFNAME])
>                 nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
>         else
> @@ -2756,6 +2760,23 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>         else
>                 goto errout;
>
> +       /* A hack to preserve kernel<->userspace interface.
> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
> +        * attribute as a way to _set_ the network namespace.  In this
> +        * case, the device interface was assumed to be in the  _current_
> +        * namespace.
> +        * If the device cannot be found in the target namespace then we
> +        * assume that the request is to set the device in the current
> +        * namespace and thus we attempt to find the device there.
> +        */
Could this bypasses the ns_capable() check? i.e. if the target is
"foo" but your current ns is bar. The process may be "capable" is foo
but the interface is not found in foo but present in bar and ends up
modifying it (especially when you are not capable in bar)?

> +       if (!dev && tgt_net) {
> +               net = sock_net(skb->sk);
> +               if (ifm->ifi_index > 0)
> +                       dev = __dev_get_by_index(net, ifm->ifi_index);
> +               else if (tb[IFLA_IFNAME])
> +                       dev = __dev_get_by_name(net, ifname);
> +       }
> +
>         if (dev == NULL) {
>                 err = -ENODEV;
>                 goto errout;
> @@ -2763,6 +2784,8 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>
>         err = do_setlink(skb, dev, ifm, extack, tb, ifname, 0);
>  errout:
> +       if (tgt_net)
> +               put_net(tgt_net);
>         return err;
>  }
>
> --
> 2.20.1
>

Re: [PATCH v3 0/6] Add namespace awareness to Netlink methods

[Mahesh Bandewar (महेश बंडेवार)]

On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Changed in v3:
> - added patch 6 for setting IPv6 address outside current namespace
> - address checkpatch warnings
> - address comment from Nicolas
>
> Changed in v2:
> - address comment from Nicolas
> - add accumulated ACK's
>
> Currently, Netlink has partial support for acting outside of the current
> namespace.  It appears that the intention was to extend this to all the
> methods eventually, but it hasn't been done to date.
>
> With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
> are extended to respect the selection of the namespace to work in.
>
This is nice, is there a plan to update userspace commands using this?

> /Jonas
>
> Jonas Bonn (6):
>   rtnetlink: allow RTM_SETLINK to reference other namespaces
>   rtnetlink: skip namespace change if already effect
>   rtnetlink: allow RTM_NEWLINK to act upon interfaces in arbitrary
>     namespaces
>   net: ipv4: allow setting address on interface outside current
>     namespace
>   net: namespace: allow setting NSIDs outside current namespace
>   net: ipv6: allow setting address on interface outside current
>     namespace
>
>  net/core/net_namespace.c | 19 ++++++++++
>  net/core/rtnetlink.c     | 80 ++++++++++++++++++++++++++++++++++------
>  net/ipv4/devinet.c       | 61 ++++++++++++++++++++++--------
>  net/ipv6/addrconf.c      | 13 +++++++
>  4 files changed, 145 insertions(+), 28 deletions(-)
>
> --
> 2.20.1
>

Re: [PATCH v3 0/6] Add namespace awareness to Netlink methods

[David Ahern]

On 11/7/19 1:40 PM, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Changed in v3:
>> - added patch 6 for setting IPv6 address outside current namespace
>> - address checkpatch warnings
>> - address comment from Nicolas
>>
>> Changed in v2:
>> - address comment from Nicolas
>> - add accumulated ACK's
>>
>> Currently, Netlink has partial support for acting outside of the current
>> namespace.  It appears that the intention was to extend this to all the
>> methods eventually, but it hasn't been done to date.
>>
>> With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
>> are extended to respect the selection of the namespace to work in.
>>
> This is nice, is there a plan to update userspace commands using this?

I'm hoping for an iproute2 update and test cases to validate the changes.

Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Jonas Bonn]

Hi Mahesh,

On 07/11/2019 21:36, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>>
>> +       /* A hack to preserve kernel<->userspace interface.
>> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
>> +        * attribute as a way to _set_ the network namespace.  In this
>> +        * case, the device interface was assumed to be in the  _current_
>> +        * namespace.
>> +        * If the device cannot be found in the target namespace then we
>> +        * assume that the request is to set the device in the current
>> +        * namespace and thus we attempt to find the device there.
>> +        */
> Could this bypasses the ns_capable() check? i.e. if the target is
> "foo" but your current ns is bar. The process may be "capable" is foo
> but the interface is not found in foo but present in bar and ends up
> modifying it (especially when you are not capable in bar)?

I don't think so.  There was never any capable-check for the "current" 
namespace so there's no change in that regard.

I do think there is an issue with this hack that I can't see any 
workaround for.  If the user specifies an interface (by name or index) 
for another namespace that doesn't exist, there's a potential problem if 
that name/index happens to exist in the "current" namespace.  In that 
case, one many end up inadvertently modifying the interface in the 
current namespace.  I don't see how to avoid that while maintaining the 
backwards compatibility.

My absolute preference would be to drop this compat-hack altogether. 
iproute2 doesn't use a bare TARGET_NETNSID in this manner (for changing 
namespaces) and I didn't find any other users by a quick search of other 
prominent Netlink users:  systemd, network-manager, connman.  This 
compat-hack is there for the _potential ab-user_ of the interface, not 
for any known such.

> 
>> +       if (!dev && tgt_net) {
>> +               net = sock_net(skb->sk);
>> +               if (ifm->ifi_index > 0)
>> +                       dev = __dev_get_by_index(net, ifm->ifi_index);
>> +               else if (tb[IFLA_IFNAME])
>> +                       dev = __dev_get_by_name(net, ifname);
>> +       }


/Jonas

Re: [PATCH v3 0/6] Add namespace awareness to Netlink methods

[Jonas Bonn]

On 07/11/2019 22:11, David Ahern wrote:
> On 11/7/19 1:40 PM, Mahesh Bandewar (महेश बंडेवार) wrote:
>> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>>
>>> Changed in v3:
>>> - added patch 6 for setting IPv6 address outside current namespace
>>> - address checkpatch warnings
>>> - address comment from Nicolas
>>>
>>> Changed in v2:
>>> - address comment from Nicolas
>>> - add accumulated ACK's
>>>
>>> Currently, Netlink has partial support for acting outside of the current
>>> namespace.  It appears that the intention was to extend this to all the
>>> methods eventually, but it hasn't been done to date.
>>>
>>> With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
>>> are extended to respect the selection of the namespace to work in.
>>>
>> This is nice, is there a plan to update userspace commands using this?
> 
> I'm hoping for an iproute2 update and test cases to validate the changes.
> 

I'm looking into it.  The change to iproute2 to support 
(namespace,index) pairs instead of just (index) to identify interfaces 
looks to be invasive.  The rest of it looks like trivial changes.

I've got all these kernel patches tested against my own "namespace aware 
network manager" that I'm writing for a customer with a particular use 
case.  iproute2 wasn't actually in play here.

/Jonas

Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Mahesh Bandewar (महेश बंडेवार)]

Hi Jonas, thanks for the response.

On Fri, Nov 8, 2019 at 12:20 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Mahesh,
>
> On 07/11/2019 21:36, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>
> >>
> >> +       /* A hack to preserve kernel<->userspace interface.
> >> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
> >> +        * attribute as a way to _set_ the network namespace.  In this
> >> +        * case, the device interface was assumed to be in the  _current_
> >> +        * namespace.
> >> +        * If the device cannot be found in the target namespace then we
> >> +        * assume that the request is to set the device in the current
> >> +        * namespace and thus we attempt to find the device there.
> >> +        */
> > Could this bypasses the ns_capable() check? i.e. if the target is
> > "foo" but your current ns is bar. The process may be "capable" is foo
> > but the interface is not found in foo but present in bar and ends up
> > modifying it (especially when you are not capable in bar)?
>
> I don't think so.  There was never any capable-check for the "current"
> namespace so there's no change in that regard.
>
not having capable-check seems wrong as we don't want random
not-capable processes to alter settings. However, it may be at the API
entry level, which will provide necessary protection (haven't
checked!). Having said that, this could be bad for the stuff that you
are implementing since I could be in "foo" and attempting to change
"bar". For this I must be capable in "bar" but the top-level capable
check will by default check me in "foo" as well which is not required
and could potentially block me from performing legal operation in
"bar".

Not saying this is a problem, but without having an implementation to
use this would be hard to try. You would most likely have a way to
verify this, so please check it.

> I do think there is an issue with this hack that I can't see any
> workaround for.  If the user specifies an interface (by name or index)
> for another namespace that doesn't exist, there's a potential problem if
> that name/index happens to exist in the "current" namespace.  In that
> case, one many end up inadvertently modifying the interface in the
> current namespace.  I don't see how to avoid that while maintaining the
> backwards compatibility.
>
This could very well be the case always for single digit ifindex
values. (We recently suffered a local scare because of something very
similar).

> My absolute preference would be to drop this compat-hack altogether.
> iproute2 doesn't use a bare TARGET_NETNSID in this manner (for changing
> namespaces) and I didn't find any other users by a quick search of other
> prominent Netlink users:  systemd, network-manager, connman.  This
> compat-hack is there for the _potential ab-user_ of the interface, not
> for any known such.
>
what is forcing you keeping you keeping / implementing this hack? I
would also prefer simple solution without creating a potential problem
/ vulnerability (problem: potentially modifying unintended interface,
vulnerability: potentially allow changing without proper credentials;
both not proven but are possibilities) down the line. One possibility
is to drop the compatibility hack and keep it as a backup if something
breaks / someone complains.

thanks,
--mahesh..

> >
> >> +       if (!dev && tgt_net) {
> >> +               net = sock_net(skb->sk);
> >> +               if (ifm->ifi_index > 0)
> >> +                       dev = __dev_get_by_index(net, ifm->ifi_index);
> >> +               else if (tb[IFLA_IFNAME])
> >> +                       dev = __dev_get_by_name(net, ifname);
> >> +       }
>
>
> /Jonas

Re: [PATCH v3 0/6] Add namespace awareness to Netlink methods

[Mahesh Bandewar (महेश बंडेवार)]

On Fri, Nov 8, 2019 at 7:36 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
>
>
> On 07/11/2019 22:11, David Ahern wrote:
> > On 11/7/19 1:40 PM, Mahesh Bandewar (महेश बंडेवार) wrote:
> >> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>>
> >>> Changed in v3:
> >>> - added patch 6 for setting IPv6 address outside current namespace
> >>> - address checkpatch warnings
> >>> - address comment from Nicolas
> >>>
> >>> Changed in v2:
> >>> - address comment from Nicolas
> >>> - add accumulated ACK's
> >>>
> >>> Currently, Netlink has partial support for acting outside of the current
> >>> namespace.  It appears that the intention was to extend this to all the
> >>> methods eventually, but it hasn't been done to date.
> >>>
> >>> With this series RTM_SETLINK, RTM_NEWLINK, RTM_NEWADDR, and RTM_NEWNSID
> >>> are extended to respect the selection of the namespace to work in.
> >>>
> >> This is nice, is there a plan to update userspace commands using this?
> >
> > I'm hoping for an iproute2 update and test cases to validate the changes.
> >
>
> I'm looking into it.  The change to iproute2 to support
> (namespace,index) pairs instead of just (index) to identify interfaces
> looks to be invasive.  The rest of it looks like trivial changes.
>
> I've got all these kernel patches tested against my own "namespace aware
> network manager" that I'm writing for a customer with a particular use
> case.  iproute2 wasn't actually in play here.
>
I'll echo David's comment for iproute2 as well as tests to ensure this
new behavior is usable and healthy.

> /Jonas

Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Jonas Bonn]

Hi Mahesh,

Thanks for the detailed response.  It provided valuable insight.

On 08/11/2019 19:55, Mahesh Bandewar (महेश बंडेवार) wrote:
> Hi Jonas, thanks for the response.
> 
> On Fri, Nov 8, 2019 at 12:20 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Hi Mahesh,
>>
>> On 07/11/2019 21:36, Mahesh Bandewar (महेश बंडेवार) wrote:
>>> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>>>
>>>>
>>>> +       /* A hack to preserve kernel<->userspace interface.
>>>> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
>>>> +        * attribute as a way to _set_ the network namespace.  In this
>>>> +        * case, the device interface was assumed to be in the  _current_
>>>> +        * namespace.
>>>> +        * If the device cannot be found in the target namespace then we
>>>> +        * assume that the request is to set the device in the current
>>>> +        * namespace and thus we attempt to find the device there.
>>>> +        */
>>> Could this bypasses the ns_capable() check? i.e. if the target is
>>> "foo" but your current ns is bar. The process may be "capable" is foo
>>> but the interface is not found in foo but present in bar and ends up
>>> modifying it (especially when you are not capable in bar)?
>>
>> I don't think so.  There was never any capable-check for the "current"
>> namespace so there's no change in that regard.

I was wrong on this point.  There IS a capable-check for the "current" 
net.  The code to create interfaces in 'other' namespaces was already in 
place before my patch and that code does the right thing with respect to 
checking NS capabilities on the "destination" and "link" nets.

My patch is mostly just accounting for the "setlink" aspect of NEWLINK 
where the device already exists in a foreign namespace and needs to be 
searched for there.  Even in that code path, all the ns-capable checks 
are in place and the behaviour is the same as before.

>>
> not having capable-check seems wrong as we don't want random
> not-capable processes to alter settings. However, it may be at the API
> entry level, which will provide necessary protection (haven't
> checked!). Having said that, this could be bad for the stuff that you
> are implementing since I could be in "foo" and attempting to change
> "bar". For this I must be capable in "bar" but the top-level capable
> check will by default check me in "foo" as well which is not required
> and could potentially block me from performing legal operation in
> "bar".
> 
> Not saying this is a problem, but without having an implementation to
> use this would be hard to try. You would most likely have a way to
> verify this, so please check it.

The above shouldn't be an issue with the current implementation.

> 
>> I do think there is an issue with this hack that I can't see any
>> workaround for.  If the user specifies an interface (by name or index)
>> for another namespace that doesn't exist, there's a potential problem if
>> that name/index happens to exist in the "current" namespace.  In that
>> case, one many end up inadvertently modifying the interface in the
>> current namespace.  I don't see how to avoid that while maintaining the
>> backwards compatibility.
>>
> This could very well be the case always for single digit ifindex
> values. (We recently suffered a local scare because of something very
> similar).
> 
>> My absolute preference would be to drop this compat-hack altogether.
>> iproute2 doesn't use a bare TARGET_NETNSID in this manner (for changing
>> namespaces) and I didn't find any other users by a quick search of other
>> prominent Netlink users:  systemd, network-manager, connman.  This
>> compat-hack is there for the _potential ab-user_ of the interface, not
>> for any known such.
>>
> what is forcing you keeping you keeping / implementing this hack? I
> would also prefer simple solution without creating a potential problem
> / vulnerability (problem: potentially modifying unintended interface,
> vulnerability: potentially allow changing without proper credentials;
> both not proven but are possibilities) down the line. One possibility
> is to drop the compatibility hack and keep it as a backup if something
> breaks / someone complains.

OK, this would be my preference, too.  If we can work on the assumption 
that this isn't actually providing compatibility for anybody in 
practice, then we can drop it.  With that, the potential problem of 
inadvertently modifying the wrong device disappears.  There's no problem 
of being able to access a namespace that one isn't capable in, but 
leaving a hole through which the user may end up doing something 
unexpected is pretty ugly.

I'll remove this and repost the series.

Thanks for your insight into this issue.  It was helpful.

/Jonas

Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

[Mahesh Bandewar (महेश बंडेवार)]

On Sat, Nov 9, 2019 at 6:17 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Mahesh,
>
> Thanks for the detailed response.  It provided valuable insight.
>
> On 08/11/2019 19:55, Mahesh Bandewar (महेश बंडेवार) wrote:
> > Hi Jonas, thanks for the response.
> >
> > On Fri, Nov 8, 2019 at 12:20 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>
> >> Hi Mahesh,
> >>
> >> On 07/11/2019 21:36, Mahesh Bandewar (महेश बंडेवार) wrote:
> >>> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>>>
> >>>>
> >>>> +       /* A hack to preserve kernel<->userspace interface.
> >>>> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
> >>>> +        * attribute as a way to _set_ the network namespace.  In this
> >>>> +        * case, the device interface was assumed to be in the  _current_
> >>>> +        * namespace.
> >>>> +        * If the device cannot be found in the target namespace then we
> >>>> +        * assume that the request is to set the device in the current
> >>>> +        * namespace and thus we attempt to find the device there.
> >>>> +        */
> >>> Could this bypasses the ns_capable() check? i.e. if the target is
> >>> "foo" but your current ns is bar. The process may be "capable" is foo
> >>> but the interface is not found in foo but present in bar and ends up
> >>> modifying it (especially when you are not capable in bar)?
> >>
> >> I don't think so.  There was never any capable-check for the "current"
> >> namespace so there's no change in that regard.
>
> I was wrong on this point.  There IS a capable-check for the "current"
> net.  The code to create interfaces in 'other' namespaces was already in
> place before my patch and that code does the right thing with respect to
> checking NS capabilities on the "destination" and "link" nets.
>
> My patch is mostly just accounting for the "setlink" aspect of NEWLINK
> where the device already exists in a foreign namespace and needs to be
> searched for there.  Even in that code path, all the ns-capable checks
> are in place and the behaviour is the same as before.
>
> >>
> > not having capable-check seems wrong as we don't want random
> > not-capable processes to alter settings. However, it may be at the API
> > entry level, which will provide necessary protection (haven't
> > checked!). Having said that, this could be bad for the stuff that you
> > are implementing since I could be in "foo" and attempting to change
> > "bar". For this I must be capable in "bar" but the top-level capable
> > check will by default check me in "foo" as well which is not required
> > and could potentially block me from performing legal operation in
> > "bar".
> >
> > Not saying this is a problem, but without having an implementation to
> > use this would be hard to try. You would most likely have a way to
> > verify this, so please check it.
>
> The above shouldn't be an issue with the current implementation.
>
> >
> >> I do think there is an issue with this hack that I can't see any
> >> workaround for.  If the user specifies an interface (by name or index)
> >> for another namespace that doesn't exist, there's a potential problem if
> >> that name/index happens to exist in the "current" namespace.  In that
> >> case, one many end up inadvertently modifying the interface in the
> >> current namespace.  I don't see how to avoid that while maintaining the
> >> backwards compatibility.
> >>
> > This could very well be the case always for single digit ifindex
> > values. (We recently suffered a local scare because of something very
> > similar).
> >
> >> My absolute preference would be to drop this compat-hack altogether.
> >> iproute2 doesn't use a bare TARGET_NETNSID in this manner (for changing
> >> namespaces) and I didn't find any other users by a quick search of other
> >> prominent Netlink users:  systemd, network-manager, connman.  This
> >> compat-hack is there for the _potential ab-user_ of the interface, not
> >> for any known such.
> >>
> > what is forcing you keeping you keeping / implementing this hack? I
> > would also prefer simple solution without creating a potential problem
> > / vulnerability (problem: potentially modifying unintended interface,
> > vulnerability: potentially allow changing without proper credentials;
> > both not proven but are possibilities) down the line. One possibility
> > is to drop the compatibility hack and keep it as a backup if something
> > breaks / someone complains.
>
> OK, this would be my preference, too.  If we can work on the assumption
> that this isn't actually providing compatibility for anybody in
> practice, then we can drop it.  With that, the potential problem of
> inadvertently modifying the wrong device disappears.  There's no problem
> of being able to access a namespace that one isn't capable in, but
> leaving a hole through which the user may end up doing something
> unexpected is pretty ugly.
>
> I'll remove this and repost the series.
>
sgtm

thanks,
--mahesh..

> Thanks for your insight into this issue.  It was helpful.
>
> /Jonas