[PATCH v2 iproute2 0/2] iplink: Fix link-netns handling

[PATCH v2 iproute2 0/2] iplink: Fix link-netns handling

[Xiao Liang]

When handling something like:

    # ip -n ns1 link add netns ns2 link-netns ns3 link eth1 eth1.100 type vlan id 100

should lookup eth1 in ns3 and set IFLA_LINK_NETNSID to the id of ns3 from ns2.
But currently ip-link tries to find eth1 in ns1 and failes. This series fixes
it.

---

v2:
- Rebase in regard to
    57daf8ff8c6c ("iplink: fix fd leak when playing with netns")


Xiao Liang (2):
  ip: Move of set_netnsid_from_name() to namespace.c
  iplink: Fix link-netns id and link ifindex

 include/namespace.h |   2 +
 ip/ip_common.h      |   2 -
 ip/iplink.c         | 143 ++++++++++++++++++++++++++++++++++++--------
 ip/ipnetns.c        |  28 +--------
 lib/namespace.c     |  27 +++++++++
 5 files changed, 150 insertions(+), 52 deletions(-)

-- 
2.47.0

[PATCH v2 iproute2 1/2] ip: Move of set_netnsid_from_name() to namespace.c

[Xiao Liang]

Move set_netnsid_from_name() outside for reuse, like what's done for
netns_id_from_name().

Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
---
 include/namespace.h |  2 ++
 ip/ip_common.h      |  2 --
 ip/iplink.c         |  6 +++---
 ip/ipnetns.c        | 28 +++-------------------------
 lib/namespace.c     | 27 +++++++++++++++++++++++++++
 5 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/include/namespace.h b/include/namespace.h
index 86000543..98f4af59 100644
--- a/include/namespace.h
+++ b/include/namespace.h
@@ -60,6 +60,8 @@ struct netns_func {
 };
 
 int netns_id_from_name(struct rtnl_handle *rtnl, const char *name);
+int set_netns_id_from_name(struct rtnl_handle *rtnl, const char *name,
+			   int nsid);
 char *netns_name_from_id(int32_t id);
 
 #endif /* __NAMESPACE_H__ */
diff --git a/ip/ip_common.h b/ip/ip_common.h
index 625311c2..726262ab 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -63,8 +63,6 @@ void netns_nsid_socket_init(void);
 int print_nsid(struct nlmsghdr *n, void *arg);
 int ipstats_print(struct nlmsghdr *n, void *arg);
 char *get_name_from_nsid(int nsid);
-int get_netnsid_from_name(const char *name);
-int set_netnsid_from_name(const char *name, int nsid);
 int do_ipaddr(int argc, char **argv);
 int do_ipaddrlabel(int argc, char **argv);
 int do_iproute(int argc, char **argv);
diff --git a/ip/iplink.c b/ip/iplink.c
index 0dd83ff4..c9168985 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -819,11 +819,11 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			NEXT_ARG();
 			if (link_netnsid != -1)
 				duparg("link-netns/link-netnsid", *argv);
-			link_netnsid = get_netnsid_from_name(*argv);
+			link_netnsid = netns_id_from_name(&rth, *argv);
 			/* No nsid? Try to assign one. */
 			if (link_netnsid < 0)
-				set_netnsid_from_name(*argv, -1);
-			link_netnsid = get_netnsid_from_name(*argv);
+				set_netns_id_from_name(&rth, *argv, -1);
+			link_netnsid = netns_id_from_name(&rth, *argv);
 			if (link_netnsid < 0)
 				invarg("Invalid \"link-netns\" value\n",
 				       *argv);
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 972d7e9c..5c943400 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -104,7 +104,7 @@ static int ipnetns_have_nsid(void)
 	return have_rtnl_getnsid;
 }
 
-int get_netnsid_from_name(const char *name)
+static int get_netnsid_from_name(const char *name)
 {
 	netns_nsid_socket_init();
 
@@ -896,33 +896,11 @@ out_delete:
 	return -1;
 }
 
-int set_netnsid_from_name(const char *name, int nsid)
+static int set_netnsid_from_name(const char *name, int nsid)
 {
-	struct {
-		struct nlmsghdr n;
-		struct rtgenmsg g;
-		char            buf[1024];
-	} req = {
-		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)),
-		.n.nlmsg_flags = NLM_F_REQUEST,
-		.n.nlmsg_type = RTM_NEWNSID,
-		.g.rtgen_family = AF_UNSPEC,
-	};
-	int fd, err = 0;
-
 	netns_nsid_socket_init();
 
-	fd = netns_get_fd(name);
-	if (fd < 0)
-		return fd;
-
-	addattr32(&req.n, 1024, NETNSA_FD, fd);
-	addattr32(&req.n, 1024, NETNSA_NSID, nsid);
-	if (rtnl_talk(&rth, &req.n, NULL) < 0)
-		err = -2;
-
-	close(fd);
-	return err;
+	return set_netns_id_from_name(&rth, name, nsid);
 }
 
 static int netns_set(int argc, char **argv)
diff --git a/lib/namespace.c b/lib/namespace.c
index d3aeb965..74b7e7ca 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -189,6 +189,33 @@ out:
 	return ret;
 }
 
+int set_netns_id_from_name(struct rtnl_handle *rtnl, const char *name, int nsid)
+{
+	struct {
+		struct nlmsghdr n;
+		struct rtgenmsg g;
+		char            buf[1024];
+	} req = {
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)),
+		.n.nlmsg_flags = NLM_F_REQUEST,
+		.n.nlmsg_type = RTM_NEWNSID,
+		.g.rtgen_family = AF_UNSPEC,
+	};
+	int fd, err = 0;
+
+	fd = netns_get_fd(name);
+	if (fd < 0)
+		return fd;
+
+	addattr32(&req.n, 1024, NETNSA_FD, fd);
+	addattr32(&req.n, 1024, NETNSA_NSID, nsid);
+	if (rtnl_talk(rtnl, &req.n, NULL) < 0)
+		err = -2;
+
+	close(fd);
+	return err;
+}
+
 struct netns_name_from_id_ctx {
 	int32_t id;
 	char *name;
-- 
2.47.0

[PATCH v2 iproute2 2/2] iplink: Fix link-netns id and link ifindex

[Xiao Liang]

When link-netns or link-netnsid is supplied, lookup link in that netns.
And if both netns and link-netns are given, IFLA_LINK_NETNSID should be
the nsid of link-netns from the view of target netns, not from current
one.

For example, when handling:

    # ip -n ns1 link add netns ns2 link-netns ns3 link eth1 eth1.100 type vlan id 100

should lookup eth1 in ns3 and IFLA_LINK_NETNSID is the id of ns3 from
ns2.

Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
---
 ip/iplink.c | 143 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index c9168985..c2c0e371 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -240,6 +240,38 @@ static int nl_get_ll_addr_len(const char *ifname)
 	return len;
 }
 
+static int get_ifindex_in_netns(struct rtnl_handle *rtnl, int netnsid,
+				const char *ifname)
+{
+	struct {
+		struct nlmsghdr		n;
+		struct ifinfomsg	ifm;
+		char			buf[1024];
+	} req = {
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
+		.n.nlmsg_flags = NLM_F_REQUEST,
+		.n.nlmsg_type = RTM_GETLINK,
+	};
+	struct nlmsghdr *answer;
+	int ifindex;
+
+	addattr32(&req.n, sizeof(req), IFLA_TARGET_NETNSID, netnsid);
+	addattr_l(&req.n, sizeof(req),
+		  !check_ifname(ifname) ? IFLA_IFNAME : IFLA_ALT_IFNAME,
+		  ifname, strlen(ifname) + 1);
+
+	if (rtnl_talk(rtnl, &req.n, &answer) < 0)
+		return 0;
+
+	if (answer->nlmsg_len < NLMSG_LENGTH(sizeof(struct ifinfomsg))) {
+		free(answer);
+		return 0;
+	}
+	ifindex = ((struct ifinfomsg *)NLMSG_DATA(answer))->ifi_index;
+	free(answer);
+	return ifindex;
+}
+
 static void iplink_parse_vf_vlan_info(int vf, int *argcp, char ***argvp,
 				      struct ifla_vf_vlan_info *ivvip)
 {
@@ -536,7 +568,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 	int vf = -1;
 	int numtxqueues = -1;
 	int numrxqueues = -1;
+	char *link_netns = NULL;
 	int link_netnsid = -1;
+	struct rtnl_handle netns_rtnl;
+	struct rtnl_handle *rtnl = &rth;
 	int index = 0;
 	int group = -1;
 	int addr_len = 0;
@@ -618,20 +653,25 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			if (offload && name == dev)
 				dev = NULL;
 		} else if (strcmp(*argv, "netns") == 0) {
+			int pid;
+
 			NEXT_ARG();
 			if (netns != -1)
 				duparg("netns", *argv);
 			netns = netns_get_fd(*argv);
-			if (netns >= 0) {
-				open_fds_add(netns);
-				addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
-					  &netns, 4);
+			if (netns < 0 && get_integer(&pid, *argv, 0) == 0) {
+				char path[PATH_MAX];
+
+				snprintf(path, sizeof(path), "/proc/%d/ns/net",
+					 pid);
+				netns = open(path, O_RDONLY);
 			}
-			else if (get_integer(&netns, *argv, 0) == 0)
-				addattr_l(&req->n, sizeof(*req),
-					  IFLA_NET_NS_PID, &netns, 4);
-			else
+			if (netns < 0)
 				invarg("Invalid \"netns\" value\n", *argv);
+
+			open_fds_add(netns);
+			addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
+				  &netns, 4);
 			move_netns = true;
 		} else if (strcmp(*argv, "multicast") == 0) {
 			NEXT_ARG();
@@ -817,21 +857,12 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			addattr_nest_end(&req->n, afs);
 		} else if (matches(*argv, "link-netns") == 0) {
 			NEXT_ARG();
-			if (link_netnsid != -1)
+			if (link_netnsid != -1 || link_netns)
 				duparg("link-netns/link-netnsid", *argv);
-			link_netnsid = netns_id_from_name(&rth, *argv);
-			/* No nsid? Try to assign one. */
-			if (link_netnsid < 0)
-				set_netns_id_from_name(&rth, *argv, -1);
-			link_netnsid = netns_id_from_name(&rth, *argv);
-			if (link_netnsid < 0)
-				invarg("Invalid \"link-netns\" value\n",
-				       *argv);
-			addattr32(&req->n, sizeof(*req), IFLA_LINK_NETNSID,
-				  link_netnsid);
+			link_netns = *argv;
 		} else if (matches(*argv, "link-netnsid") == 0) {
 			NEXT_ARG();
-			if (link_netnsid != -1)
+			if (link_netnsid != -1 || link_netns)
 				duparg("link-netns/link-netnsid", *argv);
 			if (get_integer(&link_netnsid, *argv, 0))
 				invarg("Invalid \"link-netnsid\" value\n",
@@ -983,6 +1014,53 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 		}
 	}
 
+	if (netns != -1 && (link_netnsid != -1 || link_netns)) {
+		int orig_netns;
+
+		/*
+		 * When both link-netns and netns are set, open an RTNL in
+		 * target netns, to
+		 *   1) get link-netns id from the view of target netns, and
+		 *   2) get link ifindex from link-netns.
+		 */
+		orig_netns = open("/proc/self/ns/net", O_RDONLY);
+		if (orig_netns == -1) {
+			fprintf(stderr, "Cannot open namespace: %s\n",
+				strerror(errno));
+			exit(-1);
+		}
+		if (setns(netns, CLONE_NEWNET) < 0) {
+			fprintf(stderr, "Cannot set namespace: %s\n",
+				strerror(errno));
+			exit(-1);
+		}
+		if (rtnl_open(&netns_rtnl, 0) < 0) {
+			fprintf(stderr, "Cannot open rtnetlink\n");
+			exit(-1);
+		}
+		if (setns(orig_netns, CLONE_NEWNET) < 0) {
+			fprintf(stderr, "Cannot set namespace: %s\n",
+				strerror(errno));
+			exit(-1);
+		}
+		close(orig_netns);
+		rtnl = &netns_rtnl;
+	}
+
+	if (link_netns) {
+		link_netnsid = netns_id_from_name(rtnl, link_netns);
+		/* No nsid? Try to assign one. */
+		if (link_netnsid < 0) {
+			set_netns_id_from_name(rtnl, link_netns, -1);
+			link_netnsid = netns_id_from_name(rtnl, link_netns);
+		}
+		if (link_netnsid < 0)
+			invarg("Invalid \"link-netns\" value\n",
+			       *argv);
+		addattr32(&req->n, sizeof(*req), IFLA_LINK_NETNSID,
+			  link_netnsid);
+	}
+
 	if (!(req->n.nlmsg_flags & NLM_F_CREATE)) {
 		if (!dev) {
 			fprintf(stderr,
@@ -991,8 +1069,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 		}
 
 		req->i.ifi_index = ll_name_to_index(dev);
-		if (!req->i.ifi_index)
-			return nodev(dev);
+		if (!req->i.ifi_index) {
+			ret = nodev(dev);
+			goto out;
+		}
 
 		/* Not renaming to the same name */
 		if (name == dev)
@@ -1010,9 +1090,17 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 		if (link) {
 			int ifindex;
 
-			ifindex = ll_name_to_index(link);
-			if (!ifindex)
-				return nodev(link);
+			if (link_netnsid == -1)
+				ifindex = ll_name_to_index(link);
+			else
+				ifindex = get_ifindex_in_netns(rtnl,
+							       link_netnsid,
+							       link);
+
+			if (!ifindex) {
+				ret = nodev(link);
+				goto out;
+			}
 			addattr32(&req->n, sizeof(*req), IFLA_LINK, ifindex);
 		}
 
@@ -1024,6 +1112,11 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 
+out:
+	if (rtnl == &netns_rtnl) {
+		rtnl_close(rtnl);
+	}
+
 	return ret;
 }
 
-- 
2.47.0

Re: [PATCH v2 iproute2 2/2] iplink: Fix link-netns id and link ifindex

[Nicolas Dichtel]

Le 11/10/2024 à 10:01, Xiao Liang a écrit :
> When link-netns or link-netnsid is supplied, lookup link in that netns.
> And if both netns and link-netns are given, IFLA_LINK_NETNSID should be
> the nsid of link-netns from the view of target netns, not from current
> one.
> 
> For example, when handling:
> 
>     # ip -n ns1 link add netns ns2 link-netns ns3 link eth1 eth1.100 type vlan id 100
> 
> should lookup eth1 in ns3 and IFLA_LINK_NETNSID is the id of ns3 from
> ns2.
Indeed.

> 
> Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
> ---
>  ip/iplink.c | 143 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 118 insertions(+), 25 deletions(-)
> 

[snip]

> @@ -618,20 +653,25 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
>  			if (offload && name == dev)
>  				dev = NULL;
>  		} else if (strcmp(*argv, "netns") == 0) {
> +			int pid;
> +
>  			NEXT_ARG();
>  			if (netns != -1)
>  				duparg("netns", *argv);
>  			netns = netns_get_fd(*argv);
> -			if (netns >= 0) {
> -				open_fds_add(netns);
> -				addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
> -					  &netns, 4);
> +			if (netns < 0 && get_integer(&pid, *argv, 0) == 0) {
> +				char path[PATH_MAX];
> +
> +				snprintf(path, sizeof(path), "/proc/%d/ns/net",
> +					 pid);
> +				netns = open(path, O_RDONLY);
>  			}
This chunk is added to allow the user to give a pid instead of a netns name.
It's not directly related to the patch topic. Could you put in a separate patch?

> -			else if (get_integer(&netns, *argv, 0) == 0)
> -				addattr_l(&req->n, sizeof(*req),
> -					  IFLA_NET_NS_PID, &netns, 4);
> -			else
> +			if (netns < 0)
>  				invarg("Invalid \"netns\" value\n", *argv);
> +
> +			open_fds_add(netns);
> +			addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
> +				  &netns, 4);
>  			move_netns = true;
>  		} else if (strcmp(*argv, "multicast") == 0) {
>  			NEXT_ARG();

Re: [PATCH v2 iproute2 2/2] iplink: Fix link-netns id and link ifindex

[Xiao Liang]

On Tue, Oct 15, 2024 at 3:45 PM Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:

> > @@ -618,20 +653,25 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
> >                       if (offload && name == dev)
> >                               dev = NULL;
> >               } else if (strcmp(*argv, "netns") == 0) {
> > +                     int pid;
> > +
> >                       NEXT_ARG();
> >                       if (netns != -1)
> >                               duparg("netns", *argv);
> >                       netns = netns_get_fd(*argv);
> > -                     if (netns >= 0) {
> > -                             open_fds_add(netns);
> > -                             addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
> > -                                       &netns, 4);
> > +                     if (netns < 0 && get_integer(&pid, *argv, 0) == 0) {
> > +                             char path[PATH_MAX];
> > +
> > +                             snprintf(path, sizeof(path), "/proc/%d/ns/net",
> > +                                      pid);
> > +                             netns = open(path, O_RDONLY);
> >                       }
> This chunk is added to allow the user to give a pid instead of a netns name.
> It's not directly related to the patch topic. Could you put in a separate patch?

Currently ip-link already accepts pid as netns argument, and passes to
kernel as IFLA_NET_NS_PID. This patch converts it to fd for
simplicity, so that it can be reused in later setns() call (before
opening RTNL in target netns).

Re: [PATCH v2 iproute2 2/2] iplink: Fix link-netns id and link ifindex

[Nicolas Dichtel]

Le 15/10/2024 à 11:06, Xiao Liang a écrit :
> On Tue, Oct 15, 2024 at 3:45 PM Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
> 
>>> @@ -618,20 +653,25 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
>>>                       if (offload && name == dev)
>>>                               dev = NULL;
>>>               } else if (strcmp(*argv, "netns") == 0) {
>>> +                     int pid;
>>> +
>>>                       NEXT_ARG();
>>>                       if (netns != -1)
>>>                               duparg("netns", *argv);
>>>                       netns = netns_get_fd(*argv);
>>> -                     if (netns >= 0) {
>>> -                             open_fds_add(netns);
>>> -                             addattr_l(&req->n, sizeof(*req), IFLA_NET_NS_FD,
>>> -                                       &netns, 4);
>>> +                     if (netns < 0 && get_integer(&pid, *argv, 0) == 0) {
>>> +                             char path[PATH_MAX];
>>> +
>>> +                             snprintf(path, sizeof(path), "/proc/%d/ns/net",
>>> +                                      pid);
>>> +                             netns = open(path, O_RDONLY);
>>>                       }
>> This chunk is added to allow the user to give a pid instead of a netns name.
>> It's not directly related to the patch topic. Could you put in a separate patch?
> 
> Currently ip-link already accepts pid as netns argument, and passes to
> kernel as IFLA_NET_NS_PID. This patch converts it to fd for
> simplicity, so that it can be reused in later setns() call (before
> opening RTNL in target netns).
Right, I've misread the diff.

Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Re: [PATCH v2 iproute2 0/2] iplink: Fix link-netns handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2.git (main)
by Stephen Hemminger <stephen@networkplumber.org>:

On Fri, 11 Oct 2024 16:01:07 +0800 you wrote:
> When handling something like:
> 
>     # ip -n ns1 link add netns ns2 link-netns ns3 link eth1 eth1.100 type vlan id 100
> 
> should lookup eth1 in ns3 and set IFLA_LINK_NETNSID to the id of ns3 from ns2.
> But currently ip-link tries to find eth1 in ns1 and failes. This series fixes
> it.
> 
> [...]

Here is the summary with links:
  - [v2,iproute2,1/2] ip: Move of set_netnsid_from_name() to namespace.c
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=18bbd74b345f
  - [v2,iproute2,2/2] iplink: Fix link-netns id and link ifindex
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html