[PATCH] netlink: Use correct interface index in NL_SET mode

[PATCH] netlink: Use correct interface index in NL_SET mode

[David Gibson]

nl_addr() and nl_route() take an 'op' selector which affects a number of
parameters to the netlink call.  Unfortunately when we introduced this
option a bug was introduced so that we always use the interface index for
the host side, rather than the one for the pasta namespace.

Really, the entire interface to nl_addr() and nl_route() is pretty bad:
it's tightly coupled with the use cases of its callers.  This is a minimal
fix which doesn't address that, but also doesn't make it significantly
worse.

Bugzilla: https://bugs.passt.top/show_bug.cgi?id=59
Fixes: 2fe046185634 ("netlink: Add functionality to copy routes from outer namespace")
Fixes: e89da3cf03b2 ("netlink: Add functionality to copy addresses from outer namespace")

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/netlink.c b/netlink.c
index bc5b2bf9..e15e23f9 100644
--- a/netlink.c
+++ b/netlink.c
@@ -226,7 +226,7 @@ void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 
 		.rta.rta_type	  = RTA_OIF,
 		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
-		.ifi		  = ifi,
+		.ifi		  = op == NL_SET ? ifi_ns : ifi,
 	};
 	unsigned dup_routes = 0;
 	ssize_t n, nlmsgs_size;
@@ -370,7 +370,7 @@ void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 		.nlh.nlmsg_seq     = nl_seq++,
 
 		.ifa.ifa_family    = af,
-		.ifa.ifa_index     = ifi,
+		.ifa.ifa_index     = op == NL_SET ? ifi_ns : ifi,
 		.ifa.ifa_prefixlen = op == NL_SET ? *prefix_len : 0,
 	};
 	ssize_t n, nlmsgs_size;
-- 
@@ -226,7 +226,7 @@ void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 
 		.rta.rta_type	  = RTA_OIF,
 		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
-		.ifi		  = ifi,
+		.ifi		  = op == NL_SET ? ifi_ns : ifi,
 	};
 	unsigned dup_routes = 0;
 	ssize_t n, nlmsgs_size;
@@ -370,7 +370,7 @@ void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 		.nlh.nlmsg_seq     = nl_seq++,
 
 		.ifa.ifa_family    = af,
-		.ifa.ifa_index     = ifi,
+		.ifa.ifa_index     = op == NL_SET ? ifi_ns : ifi,
 		.ifa.ifa_prefixlen = op == NL_SET ? *prefix_len : 0,
 	};
 	ssize_t n, nlmsgs_size;
-- 
2.41.0

Re: [PATCH] netlink: Use correct interface index in NL_SET mode

[Stefano Brivio]

On Tue, 27 Jun 2023 20:22:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> nl_addr() and nl_route() take an 'op' selector which affects a number of
> parameters to the netlink call.  Unfortunately when we introduced this
> option a bug was introduced so that we always use the interface index for
> the host side, rather than the one for the pasta namespace.

Oops, right. Not so luckily, in the tests in my environments, as well
as in Podman's CI environment, interface indices actually match...

> Really, the entire interface to nl_addr() and nl_route() is pretty bad:
> it's tightly coupled with the use cases of its callers.

I wouldn't call that specifically a bad thing... with no users, it's,
strictly speaking, useless. What's worse in my opinion is the resulting
duplication (i.e. each function being specific to *one* caller).

I was considering to introduce in that same series a struct
representing possible configuration actions, including, say, enum
nl_conf { NL_CONF_ADDR, NL_CONF_ROUTE, ... }, but I realised it would
be kind of invasive, so I gave up for the moment.

> This is a minimal
> fix which doesn't address that, but also doesn't make it significantly
> worse.
> 
> Bugzilla: https://bugs.passt.top/show_bug.cgi?id=59
> Fixes: 2fe046185634 ("netlink: Add functionality to copy routes from outer namespace")
> Fixes: e89da3cf03b2 ("netlink: Add functionality to copy addresses from outer namespace")
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied, thanks.

-- 
Stefano