[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter]

This is a rewrite of my patches to fix issue #221.

[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter]

Fixes: #221

Eliminate the route entries capacity limitations of nl_route_dup
by copping them to the destination namespace in three successive
steps to bypass dependency issues. This approach is taken from
the handling used in the restore subcommand of iproute2.

Signed-off-by: Martin Schitter <ms+git@mur.at>
---
 netlink.c | 207 ++++++++++++++++++++++++++----------------------------
 1 file changed, 99 insertions(+), 108 deletions(-)

diff --git a/netlink.c b/netlink.c
index 225b666..803cb20 100644
--- a/netlink.c
+++ b/netlink.c
@@ -525,6 +525,20 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
 	return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
 }
 
+/**
+ * nl_rtattr_cmp() -- compare two routing attributes
+ * @rta1:	Routing attribute
+ * @rta2:	Routing attribute
+ *
+ * Return: 0 if equal
+  */
+static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtattr *rta2)
+{
+	if (!rta1 || !rta2 || rta1->rta_len != rta2->rta_len)
+		return 1;
+	return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1));
+}
+
 /**
  * nl_route_dup() - Copy routes for given interface and address family
  * @s_src:	Netlink socket in source namespace
@@ -535,56 +549,73 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
  *
  * Return: 0 on success, negative error code on failure
  */
-int nl_route_dup(int s_src, unsigned int ifi_src,
-		 int s_dst, unsigned int ifi_dst, sa_family_t af)
+int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst,
+		 unsigned int ifi_dst, sa_family_t af)
 {
-	struct req_t {
-		struct nlmsghdr nlh;
-		struct rtmsg rtm;
-		struct rtattr rta;
-		unsigned int ifi;
-	} req = {
-		.rtm.rtm_family	  = af,
-		.rtm.rtm_table	  = RT_TABLE_MAIN,
-		.rtm.rtm_scope	  = RT_SCOPE_UNIVERSE,
-		.rtm.rtm_type	  = RTN_UNICAST,
+	/* Duplicate routes in correct order using 3 succesive stages:
+	 * 0. ones for local addresses,
+	 * 1. ones for local networks,
+	 * 2. others (remote networks/hosts).
+	 */
+	for (int prio = 0; prio < 3; prio++) {
+		struct req_t {
+			struct nlmsghdr nlh;
+			struct rtmsg rtm;
+			struct rtattr rta;
+			unsigned int ifi;
+		} req = {
+			.rtm.rtm_family = af,
+			.rtm.rtm_table = RT_TABLE_MAIN,
+			.rtm.rtm_scope = RT_SCOPE_UNIVERSE,
+			.rtm.rtm_type = RTN_UNICAST,
+
+			.rta.rta_type = RTA_OIF,
+			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
+			.ifi = ifi_src,
+		};
+		ssize_t status;
+		struct nlmsghdr *nh;
+		char buf[NLBUFSIZ];
+		uint32_t seq;
+
+		seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP,
+			      sizeof(req));
+
+		nl_foreach(nh, status, s_src, buf, seq) {
+			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
+			struct rtattr *rta;
+			size_t na;
+			uint16_t flags = nh->nlmsg_flags;
+			int rc;
 
-		.rta.rta_type	  = RTA_OIF,
-		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
-		.ifi		  = ifi_src,
-	};
-	ssize_t nlmsgs_size, left, status;
-	unsigned dup_routes = 0;
-	struct nlmsghdr *nh;
-	char buf[NLBUFSIZ];
-	uint32_t seq;
-	unsigned i;
 
-	seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
+			if (nh->nlmsg_type != RTM_NEWROUTE)
+				continue;
 
-	/* nl_foreach() will step through multiple response datagrams,
-	 * which we don't want here because we need to have all the
-	 * routes in the buffer at once.
-	 */
-	nh = nl_next(s_src, buf, NULL, &nlmsgs_size);
-	for (left = nlmsgs_size;
-	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
-	     nh = NLMSG_NEXT(nh, left)) {
-		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
-		bool discard = false;
-		struct rtattr *rta;
-		size_t na;
+			struct rtattr *tb[RTA_MAX+1] = { 0 };
 
-		if (nh->nlmsg_type != RTM_NEWROUTE)
-			continue;
+			for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh);
+			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
+				tb[rta->rta_type] = rta;
+			}
 
-		/* nexthop state flags don't apply to freshly created routes,
-		 * and the kernel will refuse our route if they are set.
-		 */
-		rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
+			/* Process only matching routes of current priority
+			 * (variant of an approach used in iproute2s restore_handler)
+			 */
+			if (!(prio == 2 && tb[RTA_GATEWAY]) &&
+			    !(prio == 1 && !tb[RTA_GATEWAY] &&
+			      tb[RTA_PREFSRC] &&
+			      nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) &&
+			    !(prio == 0 && !tb[RTA_GATEWAY] &&
+			      (!tb[RTA_PREFSRC] ||
+			       !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST]))))
+				continue;
+
+			/* nexthop state flags don't apply to freshly created routes,
+			 * and the kernel will refuse our route if they are set.
+			 */
+			rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
 
-		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
-		     rta = RTA_NEXT(rta, na)) {
 			/* RTA_OIF and RTA_MULTIPATH attributes carry the
 			 * identifier of a host interface. If they match the
 			 * host interface we're copying from, change them to
@@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 			 * available), or if any interface index in nexthop
 			 * objects differ from the host interface, discard the
 			 * route altogether.
-			*/
-			if (rta->rta_type == RTA_OIF) {
-				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
-					discard = true;
-					break;
+			 */
+			if (tb[RTA_OIF]) {
+				if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=
+				    ifi_src) {
+					continue;
 				}
-
-				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
-			} else if (rta->rta_type == RTA_MULTIPATH) {
-				int nh_len = RTA_PAYLOAD(rta);
+				*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =
+					ifi_dst;
+			} else if (tb[RTA_MULTIPATH]) {
+				int nh_len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
 				struct rtnexthop *rtnh;
-
-				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
+				bool discard = false;
+				for (rtnh = (struct rtnexthop *)
+					     RTA_DATA(tb[RTA_MULTIPATH]);
 				     RTNH_OK(rtnh, nh_len);
-				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
+				     rtnh = RTNH_NEXT_AND_DEC(rtnh,
+							      nh_len)) {
 					int src = (int)ifi_src;
-
 					if (rtnh->rtnh_ifindex != src) {
 						discard = true;
 						break;
 					}
-
 					rtnh->rtnh_ifindex = ifi_dst;
 				}
-
 				if (discard)
-					break;
-			} else if (rta->rta_type == RTA_PREFSRC ||
-				   rta->rta_type == RTA_NH_ID) {
+					continue;
+			} else if (tb[RTA_PREFSRC]) {
 				/* Strip RTA_PREFSRC attributes: host routes
 				 * might include a preferred source address,
 				 * which must be one of the host's addresses.
 				 * However, with -a, pasta will use a different
 				 * namespace address, making such a route
 				 * invalid in the namespace.
-				 *
-				 * Strip RTA_NH_ID attributes: host routes set
+				 */
+				tb[RTA_PREFSRC]->rta_type = RTA_UNSPEC;
+			} else if (tb[RTA_NH_ID]) {
+				/* Strip RTA_NH_ID attributes: host routes set
 				 * up via routing protocols (e.g. OSPF) might
 				 * contain a nexthop ID (and not nexthop
 				 * objects, which are taken care of in the
 				 * RTA_MULTIPATH case above) that's not valid
 				 * in the target namespace.
 				 */
-				rta->rta_type = RTA_UNSPEC;
+				tb[RTA_NH_ID]->rta_type = RTA_UNSPEC;
 			}
-		}
-
-		if (discard)
-			nh->nlmsg_type = NLMSG_NOOP;
-		else
-			dup_routes++;
-	}
-
-	if (!NLMSG_OK(nh, left)) {
-		/* Process any remaining datagrams in a different
-		 * buffer so we don't overwrite the first one.
-		 */
-		char tail[NLBUFSIZ];
-		unsigned extra = 0;
-
-		nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE)
-			extra++;
-
-		if (extra) {
-			err("netlink: Too many routes to duplicate");
-			return -E2BIG;
-		}
-	}
-	if (status < 0)
-		return status;
-
-	/* Routes might have dependencies between each other, and the kernel
-	 * processes RTM_NEWROUTE messages sequentially. For n routes, we might
-	 * need to send the requests up to n times to get all of them inserted.
-	 * Routes that have been already inserted will return -EEXIST, but we
-	 * can safely ignore that and repeat the requests. This avoids the need
-	 * to calculate dependencies: let the kernel do that.
-	 */
-	for (i = 0; i < dup_routes; i++) {
-		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
-		     NLMSG_OK(nh, left);
-		     nh = NLMSG_NEXT(nh, left)) {
-			uint16_t flags = nh->nlmsg_flags;
-			int rc;
-
-			if (nh->nlmsg_type != RTM_NEWROUTE)
-				continue;
 
 			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
-				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
+				   (flags & ~NLM_F_DUMP_FILTERED) |
+					   NLM_F_CREATE,
 				   nh->nlmsg_len);
 			if (rc < 0 && rc != -EEXIST &&
 			    rc != -ENETUNREACH && rc != -EHOSTUNREACH)
 				return rc;
 		}
+		if (status < 0)
+			return status;
 	}
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Stefano Brivio]

Martin, thanks for the updated patch, and apologies for the
embarrassing delay.

It all seems to work for me. I thought I could break this with
multipath routes but, even with those, I can't actually build
interdependent routes, not even under iproute2's category 2.

Here comes my detailed review, mostly small coding style stuff:

On Sat, 26 Sep 2026 12:02:11 +0000
Martin Schitter <ms+git@mur.at> wrote:

> Fixes: #221

That's for Debian bugs. We use tags just like Linux kernel
contributions, that is:

> 
> Eliminate the route entries capacity limitations of nl_route_dup
> by copping them to the destination namespace in three successive

(copying)

> steps to bypass dependency issues. This approach is taken from
> the handling used in the restore subcommand of iproute2.
> 

Link: https://bugs.passt.top/show_bug.cgi?id=221

> Signed-off-by: Martin Schitter <ms+git@mur.at>
> ---
>  netlink.c | 207 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 99 insertions(+), 108 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 225b666..803cb20 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -525,6 +525,20 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
>  	return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
>  }
>  
> +/**
> + * nl_rtattr_cmp() -- compare two routing attributes

With one '-' (kerneldoc), and "Compare" (for consistency).

But what about returning a bool (we use C11 now) and turn this into
nl_rtrattr_eq(), so that one doesn't have to wonder if it behaves like
memcmp() or the other way around? There's a single non-zero value we
return anyway.

> + * @rta1:	Routing attribute
> + * @rta2:	Routing attribute

Maybe "First" and "Second", just for the day we finally manage to get
all this through Sphinx or similar (same name for two arguments results
in some warnings in some documentation system, I can't remember which
one, but it kind of makes sense).

> + *
> + * Return: 0 if equal
> +  */
> +static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtattr *rta2)
> +{
> +	if (!rta1 || !rta2 || rta1->rta_len != rta2->rta_len)
> +		return 1;

We would usually insert an extra newline here for readability.

> +	return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1));
> +}
> +
>  /**
>   * nl_route_dup() - Copy routes for given interface and address family
>   * @s_src:	Netlink socket in source namespace
> @@ -535,56 +549,73 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
>   *
>   * Return: 0 on success, negative error code on failure
>   */
> -int nl_route_dup(int s_src, unsigned int ifi_src,
> -		 int s_dst, unsigned int ifi_dst, sa_family_t af)
> +int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst,
> +		 unsigned int ifi_dst, sa_family_t af)

Unrelated change.

>  {
> -	struct req_t {
> -		struct nlmsghdr nlh;
> -		struct rtmsg rtm;
> -		struct rtattr rta;
> -		unsigned int ifi;
> -	} req = {
> -		.rtm.rtm_family	  = af,
> -		.rtm.rtm_table	  = RT_TABLE_MAIN,
> -		.rtm.rtm_scope	  = RT_SCOPE_UNIVERSE,
> -		.rtm.rtm_type	  = RTN_UNICAST,
> +	/* Duplicate routes in correct order using 3 succesive stages:

"successive"

> +	 * 0. ones for local addresses,
> +	 * 1. ones for local networks,
> +	 * 2. others (remote networks/hosts).
> +	 */
> +	for (int prio = 0; prio < 3; prio++) {

What about making this an enum, so that it's self-documenting? Say:

	enum route_dup_prio {
		ROUTE_DUP_LOCAL_ADDR,
		ROUTE_DUP_LOCAL_NET,
		ROUTE_DUP_OTHER,
		ROUTE_DUP_COUNT,
	};

	for (enum prio = 0; prio < ROUTE_DUP_COUNT; prio++) {
		...

> +		struct req_t {
> +			struct nlmsghdr nlh;
> +			struct rtmsg rtm;
> +			struct rtattr rta;
> +			unsigned int ifi;
> +		} req = {
> +			.rtm.rtm_family = af,
> +			.rtm.rtm_table = RT_TABLE_MAIN,
> +			.rtm.rtm_scope = RT_SCOPE_UNIVERSE,
> +			.rtm.rtm_type = RTN_UNICAST,

I haven't tried, but what happens if you try to filter on rtm_dst_len
in struct rtmsg so that you only get the category of routes you want
(at least for the ones representing local addresses that might be
possible?), so that you have to go through fewer elements in the loop?

> +
> +			.rta.rta_type = RTA_OIF,
> +			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
> +			.ifi = ifi_src,

The way this was indented before (with tabs) looked more readable,
because it built some kind of "table".

> +		};
> +		ssize_t status;
> +		struct nlmsghdr *nh;
> +		char buf[NLBUFSIZ];
> +		uint32_t seq;
> +
> +		seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP,
> +			      sizeof(req));
> +
> +		nl_foreach(nh, status, s_src, buf, seq) {
> +			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
> +			struct rtattr *rta;
> +			size_t na;
> +			uint16_t flags = nh->nlmsg_flags;
> +			int rc;

The usual pyramid tree stuff if possible (see CONTRIBUTING.md).

>  
> -		.rta.rta_type	  = RTA_OIF,
> -		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
> -		.ifi		  = ifi_src,
> -	};
> -	ssize_t nlmsgs_size, left, status;
> -	unsigned dup_routes = 0;
> -	struct nlmsghdr *nh;
> -	char buf[NLBUFSIZ];
> -	uint32_t seq;
> -	unsigned i;
>  

There's an excess newline here now.

> -	seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
> +			if (nh->nlmsg_type != RTM_NEWROUTE)
> +				continue;
>  
> -	/* nl_foreach() will step through multiple response datagrams,
> -	 * which we don't want here because we need to have all the
> -	 * routes in the buffer at once.
> -	 */
> -	nh = nl_next(s_src, buf, NULL, &nlmsgs_size);
> -	for (left = nlmsgs_size;
> -	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
> -	     nh = NLMSG_NEXT(nh, left)) {
> -		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
> -		bool discard = false;
> -		struct rtattr *rta;
> -		size_t na;
> +			struct rtattr *tb[RTA_MAX+1] = { 0 };

Coding style: [RTA_MAX + 1]

>  
> -		if (nh->nlmsg_type != RTM_NEWROUTE)
> -			continue;
> +			for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh);
> +			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
> +				tb[rta->rta_type] = rta;

Coding style: single line for statement, no curly brackets.

> +			}
>  
> -		/* nexthop state flags don't apply to freshly created routes,
> -		 * and the kernel will refuse our route if they are set.
> -		 */
> -		rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
> +			/* Process only matching routes of current priority
> +			 * (variant of an approach used in iproute2s restore_handler)

iproute2's (Saxon genitive in English needs an apostrophe),
restore_handler() (it's a function name). It should all be wrapped at
80 columns.

> +			 */
> +			if (!(prio == 2 && tb[RTA_GATEWAY]) &&

Can we split this a bit? I think it would be easier to read with
multiple conditions and multiple continue statements (three of them
perhaps?) rather than all together.

> +			    !(prio == 1 && !tb[RTA_GATEWAY] &&

If the enum labels I proposed earlier are too long for this to be
readable, maybe drop "_DUP_" from them.

> +			      tb[RTA_PREFSRC] &&
> +			      nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) &&
> +			    !(prio == 0 && !tb[RTA_GATEWAY] &&
> +			      (!tb[RTA_PREFSRC] ||
> +			       !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST]))))
> +				continue;
> +
> +			/* nexthop state flags don't apply to freshly created routes,

Same here, wrap at 80 columns.

> +			 * and the kernel will refuse our route if they are set.
> +			 */
> +			rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
>  
> -		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
> -		     rta = RTA_NEXT(rta, na)) {
>  			/* RTA_OIF and RTA_MULTIPATH attributes carry the
>  			 * identifier of a host interface. If they match the
>  			 * host interface we're copying from, change them to
> @@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  			 * available), or if any interface index in nexthop
>  			 * objects differ from the host interface, discard the
>  			 * route altogether.
> -			*/
> -			if (rta->rta_type == RTA_OIF) {
> -				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
> -					discard = true;
> -					break;
> +			 */
> +			if (tb[RTA_OIF]) {
> +				if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=
> +				    ifi_src) {

Maybe extract it into a temporary variable ('ifi'?).

> +					continue;
>  				}

No curly brackets needed if it's a single statement (syntax) on a single
line (coding style).

> -
> -				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
> -			} else if (rta->rta_type == RTA_MULTIPATH) {
> -				int nh_len = RTA_PAYLOAD(rta);
> +				*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =
> +					ifi_dst;
> +			} else if (tb[RTA_MULTIPATH]) {
> +				int nh_len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
>  				struct rtnexthop *rtnh;
> -
> -				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
> +				bool discard = false;
> +				for (rtnh = (struct rtnexthop *)
> +					     RTA_DATA(tb[RTA_MULTIPATH]);
>  				     RTNH_OK(rtnh, nh_len);
> -				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
> +				     rtnh = RTNH_NEXT_AND_DEC(rtnh,
> +							      nh_len)) {

This part could remain on a single line (it hurts my eyes but the
ugliness is pre-existing, and actually my fault).

>  					int src = (int)ifi_src;
> -
>  					if (rtnh->rtnh_ifindex != src) {
>  						discard = true;
>  						break;
>  					}
> -
>  					rtnh->rtnh_ifindex = ifi_dst;
>  				}
> -
>  				if (discard)
> -					break;
> -			} else if (rta->rta_type == RTA_PREFSRC ||
> -				   rta->rta_type == RTA_NH_ID) {
> +					continue;
> +			} else if (tb[RTA_PREFSRC]) {
>  				/* Strip RTA_PREFSRC attributes: host routes
>  				 * might include a preferred source address,
>  				 * which must be one of the host's addresses.
>  				 * However, with -a, pasta will use a different
>  				 * namespace address, making such a route
>  				 * invalid in the namespace.
> -				 *
> -				 * Strip RTA_NH_ID attributes: host routes set
> +				 */
> +				tb[RTA_PREFSRC]->rta_type = RTA_UNSPEC;
> +			} else if (tb[RTA_NH_ID]) {
> +				/* Strip RTA_NH_ID attributes: host routes set
>  				 * up via routing protocols (e.g. OSPF) might
>  				 * contain a nexthop ID (and not nexthop
>  				 * objects, which are taken care of in the
>  				 * RTA_MULTIPATH case above) that's not valid
>  				 * in the target namespace.
>  				 */
> -				rta->rta_type = RTA_UNSPEC;
> +				tb[RTA_NH_ID]->rta_type = RTA_UNSPEC;
>  			}
> -		}
> -
> -		if (discard)
> -			nh->nlmsg_type = NLMSG_NOOP;
> -		else
> -			dup_routes++;
> -	}
> -
> -	if (!NLMSG_OK(nh, left)) {
> -		/* Process any remaining datagrams in a different
> -		 * buffer so we don't overwrite the first one.
> -		 */
> -		char tail[NLBUFSIZ];
> -		unsigned extra = 0;
> -
> -		nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE)
> -			extra++;
> -
> -		if (extra) {
> -			err("netlink: Too many routes to duplicate");
> -			return -E2BIG;
> -		}
> -	}
> -	if (status < 0)
> -		return status;
> -
> -	/* Routes might have dependencies between each other, and the kernel
> -	 * processes RTM_NEWROUTE messages sequentially. For n routes, we might
> -	 * need to send the requests up to n times to get all of them inserted.
> -	 * Routes that have been already inserted will return -EEXIST, but we
> -	 * can safely ignore that and repeat the requests. This avoids the need
> -	 * to calculate dependencies: let the kernel do that.
> -	 */
> -	for (i = 0; i < dup_routes; i++) {
> -		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
> -		     NLMSG_OK(nh, left);
> -		     nh = NLMSG_NEXT(nh, left)) {
> -			uint16_t flags = nh->nlmsg_flags;
> -			int rc;
> -
> -			if (nh->nlmsg_type != RTM_NEWROUTE)
> -				continue;
>  
>  			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
> -				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
> +				   (flags & ~NLM_F_DUMP_FILTERED) |
> +					   NLM_F_CREATE,

Reasonable to fix this here, but it should be aligned to its logical
meaning, that is:

				   (flags & ~NLM_F_DUMP_FILTERED) |
				   NLM_F_CREATE,


>  				   nh->nlmsg_len);
>  			if (rc < 0 && rc != -EEXIST &&
>  			    rc != -ENETUNREACH && rc != -EHOSTUNREACH)

On one hand, EEXIST, ENETUNREACH, and EHOSTUNREACH should all be
considered actual errors now. On the other hand it's probably more
robust to carry on as if nothing happened even if we hit them. I'm a
bit undecided but I'd rather leave this part as it is.

>  				return rc;
>  		}
> +		if (status < 0)
> +			return status;
>  	}
> -

I think you could leave this one, it's a bit easier on eyes.

>  	return 0;
>  }
>  

Hah, so simple? If I had known. :)

-- 
Stefano

Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter]

On 10/21/26 01:56, Stefano Brivio wrote:
> Martin, thanks for the updated patch, and apologies for the
> embarrassing delay.

I would like to answer: "no problem at all!", but since this issue could 
be easily exploited for denial-of-service attacks it may not be wise to 
delay applying these fixes any longer than necessary.

> It all seems to work for me. I thought I could break this with
> multipath routes but, even with those, I can't actually build
> interdependent routes, not even under iproute2's category 2.

That's nice to hear!

>> Fixes: #221
> That's for Debian bugs. We use tags just like Linux kernel
> contributions, that is:


It's one of the keywords recognized by nearly all current more 
comfortable CI/CD solutions and experienced developers. It also 
shouldn't do any harm otherwise:

https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue
https://forgejo.org/docs/v15.0/user/collaboration/linked-references/#actionable-references-in-pull-requests-and-commit-messages
...

>> Eliminate the route entries capacity limitations of nl_route_dup
>> by copping them to the destination namespace in three successive
> 
> (copying)

corrected

>> steps to bypass dependency issues. This approach is taken from
>> the handling used in the restore subcommand of iproute2.
>>
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=221

added

>> Signed-off-by: Martin Schitter <ms+git@mur.at>
>> ---
>>   netlink.c | 207 ++++++++++++++++++++++++++----------------------------
>>   1 file changed, 99 insertions(+), 108 deletions(-)
>>
>> diff --git a/netlink.c b/netlink.c
>> index 225b666..803cb20 100644
>> --- a/netlink.c
>> +++ b/netlink.c
>> @@ -525,6 +525,20 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
>>   	return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
>>   }
>>   
>> +/**
>> + * nl_rtattr_cmp() -- compare two routing attributes
> 
> With one '-' (kerneldoc), and "Compare" (for consistency).

done

> But what about returning a bool (we use C11 now) and turn this into
> nl_rtrattr_eq(), so that one doesn't have to wonder if it behaves like
> memcmp() or the other way around? There's a single non-zero value we
> return anyway.

yes -- it definitely could be done this way as well.
But isn't it more useful to keep this code sequence as close as possible 
to its origin in iproute2?

At the end it is just a memcmp with some additional protection for 
problematic corner cases.

>> + * @rta1:	Routing attribute
>> + * @rta2:	Routing attribute
> 
> Maybe "First" and "Second", just for the day we finally manage to get
> all this through Sphinx or similar (same name for two arguments results
> in some warnings in some documentation system, I can't remember which
> one, but it kind of makes sense).

It's already perfectly correct C code and already used/accepted by 
another prestigious software project.
(but you are free to modify the labels anytime if you don't like them!)

>> + *
>> + * Return: 0 if equal
>> +  */
>> +static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtattr *rta2)
>> +{
>> +	if (!rta1 || !rta2 || rta1->rta_len != rta2->rta_len)
>> +		return 1;
> 
> We would usually insert an extra newline here for readability.

done

>> +	return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1));
>> +}
>> +
>>   /**
>>    * nl_route_dup() - Copy routes for given interface and address family
>>    * @s_src:	Netlink socket in source namespace
>> @@ -535,56 +549,73 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
>>    *
>>    * Return: 0 on success, negative error code on failure
>>    */
>> -int nl_route_dup(int s_src, unsigned int ifi_src,
>> -		 int s_dst, unsigned int ifi_dst, sa_family_t af)
>> +int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst,
>> +		 unsigned int ifi_dst, sa_family_t af)
> 
> Unrelated change.

(modified by autoformatting operation based on passt/.clang-format)

reverted

>>   {
>> -	struct req_t {
>> -		struct nlmsghdr nlh;
>> -		struct rtmsg rtm;
>> -		struct rtattr rta;
>> -		unsigned int ifi;
>> -	} req = {
>> -		.rtm.rtm_family	  = af,
>> -		.rtm.rtm_table	  = RT_TABLE_MAIN,
>> -		.rtm.rtm_scope	  = RT_SCOPE_UNIVERSE,
>> -		.rtm.rtm_type	  = RTN_UNICAST,
>> +	/* Duplicate routes in correct order using 3 succesive stages:
> 
> "successive"

corrected

>> +	 * 0. ones for local addresses,
>> +	 * 1. ones for local networks,
>> +	 * 2. others (remote networks/hosts).
>> +	 */
>> +	for (int prio = 0; prio < 3; prio++) {
> 
> What about making this an enum, so that it's self-documenting? Say:
> 
> 	enum route_dup_prio {
> 		ROUTE_DUP_LOCAL_ADDR,
> 		ROUTE_DUP_LOCAL_NET,
> 		ROUTE_DUP_OTHER,
> 		ROUTE_DUP_COUNT,
> 	};
> 
> 	for (enum prio = 0; prio < ROUTE_DUP_COUNT; prio++) {
> 		...

O.k. -- applied in a slightly less verbose variant.

>> +		struct req_t {
>> +			struct nlmsghdr nlh;
>> +			struct rtmsg rtm;
>> +			struct rtattr rta;
>> +			unsigned int ifi;
>> +		} req = {
>> +			.rtm.rtm_family = af,
>> +			.rtm.rtm_table = RT_TABLE_MAIN,
>> +			.rtm.rtm_scope = RT_SCOPE_UNIVERSE,
>> +			.rtm.rtm_type = RTN_UNICAST,
> 
> I haven't tried, but what happens if you try to filter on rtm_dst_len
> in struct rtmsg so that you only get the category of routes you want
> (at least for the ones representing local addresses that might be
> possible?), so that you have to go through fewer elements in the loop?

I searched the rtnetlink(7) man page and a few other documentation 
sources, but I couldn't figure out any more useful selective 
netmask/prefix setting for rtm_dst_len than this current implicit value 
of 0, which returns all entries.
>> +
>> +			.rta.rta_type = RTA_OIF,
>> +			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
>> +			.ifi = ifi_src,
> 
> The way this was indented before (with tabs) looked more readable,
> because it built some kind of "table".

It was using an unpleasant mix of tabs and space characters, which will 
not always look nice and better readable in all possible code views.

Nevertheless, I'll try to revert the change.

>> +		};
>> +		ssize_t status;
>> +		struct nlmsghdr *nh;
>> +		char buf[NLBUFSIZ];
>> +		uint32_t seq;
>> +
>> +		seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP,
>> +			      sizeof(req));
>> +
>> +		nl_foreach(nh, status, s_src, buf, seq) {
>> +			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
>> +			struct rtattr *rta;
>> +			size_t na;
>> +			uint16_t flags = nh->nlmsg_flags;
>> +			int rc;
> 
> The usual pyramid tree stuff if possible (see CONTRIBUTING.md).

done

>>   
>> -		.rta.rta_type	  = RTA_OIF,
>> -		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
>> -		.ifi		  = ifi_src,
>> -	};
>> -	ssize_t nlmsgs_size, left, status;
>> -	unsigned dup_routes = 0;
>> -	struct nlmsghdr *nh;
>> -	char buf[NLBUFSIZ];
>> -	uint32_t seq;
>> -	unsigned i;
>>   
> 
> There's an excess newline here now.
> 
>> -	seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
>> +			if (nh->nlmsg_type != RTM_NEWROUTE)
>> +				continue;
>>   
>> -	/* nl_foreach() will step through multiple response datagrams,
>> -	 * which we don't want here because we need to have all the
>> -	 * routes in the buffer at once.
>> -	 */
>> -	nh = nl_next(s_src, buf, NULL, &nlmsgs_size);
>> -	for (left = nlmsgs_size;
>> -	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
>> -	     nh = NLMSG_NEXT(nh, left)) {
>> -		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
>> -		bool discard = false;
>> -		struct rtattr *rta;
>> -		size_t na;
>> +			struct rtattr *tb[RTA_MAX+1] = { 0 };
> 
> Coding style: [RTA_MAX + 1]

done

>>   
>> -		if (nh->nlmsg_type != RTM_NEWROUTE)
>> -			continue;
>> +			for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh);
>> +			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
>> +				tb[rta->rta_type] = rta;
> 
> Coding style: single line for statement, no curly brackets.

No! -- extraordinary complex for loop control statements, using commas 
and spread over multiply lines, are always unnecessary hard to read. 
Without curly brackets you can hardly grasp the separation between loop 
control and body on first view.

>> +			}
>>   
>> -		/* nexthop state flags don't apply to freshly created routes,
>> -		 * and the kernel will refuse our route if they are set.
>> -		 */
>> -		rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
>> +			/* Process only matching routes of current priority
>> +			 * (variant of an approach used in iproute2s restore_handler)
> 
> iproute2's (Saxon genitive in English needs an apostrophe),
> restore_handler() (it's a function name). It should all be wrapped at
> 80 columns.

done
>> +			 */
>> +			if (!(prio == 2 && tb[RTA_GATEWAY]) &&
> 
> Can we split this a bit? I think it would be easier to read with
> multiple conditions and multiple continue statements (three of them
> perhaps?) rather than all together.

I don't think it's much more readable now, but it should behave exactly 
the same as before, because only DeMorgens law got applied a few times.

The previews variant looked more similar to the expressions used in 
iproute2, which I personally would still value as more helpful.

> 
>> +			    !(prio == 1 && !tb[RTA_GATEWAY] &&
> 
> If the enum labels I proposed earlier are too long for this to be
> readable, maybe drop "_DUP_" from them.

Already done

>> +			      tb[RTA_PREFSRC] &&
>> +			      nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) &&
>> +			    !(prio == 0 && !tb[RTA_GATEWAY] &&
>> +			      (!tb[RTA_PREFSRC] ||
>> +			       !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST]))))
>> +				continue;
>> +
>> +			/* nexthop state flags don't apply to freshly created routes,
> 
> Same here, wrap at 80 columns.

done

>> +			 * and the kernel will refuse our route if they are set.
>> +			 */
>> +			rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
>>   
>> -		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
>> -		     rta = RTA_NEXT(rta, na)) {
>>   			/* RTA_OIF and RTA_MULTIPATH attributes carry the
>>   			 * identifier of a host interface. If they match the
>>   			 * host interface we're copying from, change them to
>> @@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>>   			 * available), or if any interface index in nexthop
>>   			 * objects differ from the host interface, discard the
>>   			 * route altogether.
>> -			*/
>> -			if (rta->rta_type == RTA_OIF) {
>> -				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
>> -					discard = true;
>> -					break;
>> +			 */
>> +			if (tb[RTA_OIF]) {
>> +				if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=
>> +				    ifi_src) {
> 
> Maybe extract it into a temporary variable ('ifi'?).

IMHO it wouldn't be much shorter or significant easier to grasp.

>> +					continue;
>>   				}
> 
> No curly brackets needed if it's a single statement (syntax) on a single
> line (coding style).

done

>> -
>> -				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
>> -			} else if (rta->rta_type == RTA_MULTIPATH) {
>> -				int nh_len = RTA_PAYLOAD(rta);
>> +				*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =
>> +					ifi_dst;
>> +			} else if (tb[RTA_MULTIPATH]) {
>> +				int nh_len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
>>   				struct rtnexthop *rtnh;
>> -
>> -				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
>> +				bool discard = false;
>> +				for (rtnh = (struct rtnexthop *)
>> +					     RTA_DATA(tb[RTA_MULTIPATH]);
>>   				     RTNH_OK(rtnh, nh_len);
>> -				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
>> +				     rtnh = RTNH_NEXT_AND_DEC(rtnh,
>> +							      nh_len)) {
> 
> This part could remain on a single line (it hurts my eyes but the
> ugliness is pre-existing, and actually my fault).

applied

>>   					int src = (int)ifi_src;
>> -
>>   					if (rtnh->rtnh_ifindex != src) {
>>   						discard = true;
>>   						break;
>>   					}
>> -
>>   					rtnh->rtnh_ifindex = ifi_dst;
>>   				}
>> -
>>   				if (discard)
>> -					break;
>> -			} else if (rta->rta_type == RTA_PREFSRC ||
>> -				   rta->rta_type == RTA_NH_ID) {
>> +					continue;
>> +			} else if (tb[RTA_PREFSRC]) {
>>   				/* Strip RTA_PREFSRC attributes: host routes
>>   				 * might include a preferred source address,
>>   				 * which must be one of the host's addresses.
>>   				 * However, with -a, pasta will use a different
>>   				 * namespace address, making such a route
>>   				 * invalid in the namespace.
>> -				 *
>> -				 * Strip RTA_NH_ID attributes: host routes set
>> +				 */
>> +				tb[RTA_PREFSRC]->rta_type = RTA_UNSPEC;
>> +			} else if (tb[RTA_NH_ID]) {
>> +				/* Strip RTA_NH_ID attributes: host routes set
>>   				 * up via routing protocols (e.g. OSPF) might
>>   				 * contain a nexthop ID (and not nexthop
>>   				 * objects, which are taken care of in the
>>   				 * RTA_MULTIPATH case above) that's not valid
>>   				 * in the target namespace.
>>   				 */
>> -				rta->rta_type = RTA_UNSPEC;
>> +				tb[RTA_NH_ID]->rta_type = RTA_UNSPEC;
>>   			}
>> -		}
>> -
>> -		if (discard)
>> -			nh->nlmsg_type = NLMSG_NOOP;
>> -		else
>> -			dup_routes++;
>> -	}
>> -
>> -	if (!NLMSG_OK(nh, left)) {
>> -		/* Process any remaining datagrams in a different
>> -		 * buffer so we don't overwrite the first one.
>> -		 */
>> -		char tail[NLBUFSIZ];
>> -		unsigned extra = 0;
>> -
>> -		nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE)
>> -			extra++;
>> -
>> -		if (extra) {
>> -			err("netlink: Too many routes to duplicate");
>> -			return -E2BIG;
>> -		}
>> -	}
>> -	if (status < 0)
>> -		return status;
>> -
>> -	/* Routes might have dependencies between each other, and the kernel
>> -	 * processes RTM_NEWROUTE messages sequentially. For n routes, we might
>> -	 * need to send the requests up to n times to get all of them inserted.
>> -	 * Routes that have been already inserted will return -EEXIST, but we
>> -	 * can safely ignore that and repeat the requests. This avoids the need
>> -	 * to calculate dependencies: let the kernel do that.
>> -	 */
>> -	for (i = 0; i < dup_routes; i++) {
>> -		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
>> -		     NLMSG_OK(nh, left);
>> -		     nh = NLMSG_NEXT(nh, left)) {
>> -			uint16_t flags = nh->nlmsg_flags;
>> -			int rc;
>> -
>> -			if (nh->nlmsg_type != RTM_NEWROUTE)
>> -				continue;
>>   
>>   			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
>> -				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
>> +				   (flags & ~NLM_F_DUMP_FILTERED) |
>> +					   NLM_F_CREATE,
> 
> Reasonable to fix this here, but it should be aligned to its logical
> meaning, that is:
> 
> 				   (flags & ~NLM_F_DUMP_FILTERED) |
> 				   NLM_F_CREATE, 

reformated

>>   				   nh->nlmsg_len);
>>   			if (rc < 0 && rc != -EEXIST &&
>>   			    rc != -ENETUNREACH && rc != -EHOSTUNREACH)
> 
> On one hand, EEXIST, ENETUNREACH, and EHOSTUNREACH should all be
> considered actual errors now. On the other hand it's probably more
> robust to carry on as if nothing happened even if we hit them. I'm a
> bit undecided but I'd rather leave this part as it is.
I personally would be biased to add at least some debug logs in case of 
these errors, but to keep passt/pasta based services reliable working, I 
also wouldn't enable more strict error handlers or execution abort.

Right now we don't check for NLM_F_DUMP_INTR 
(https://docs.kernel.org/userspace-api/netlink/intro.html#dump-consistency) 
and we even combine three successive route table dumps with minimal 
temporal gaps between them in our processing. Therefore, we have to 
consider that we always could get slightly inconsistent results causing 
these errors.

I don't see any satisfying simple solution for this problem. Aborting 
the execution or recursive retry attempts in case of errors would just 
open another door for denial-of-service attacks.

>>   				return rc;
>>   		}
>> +		if (status < 0)
>> +			return status;
>>   	}
>> -
> 
> I think you could leave this one, it's a bit easier on eyes.
> 
>>   	return 0;
>>   }
>>   
> 
> Hah, so simple? If I had known. :)

Well -- as far as I can see, all modifications of v5 are only of 
cosmetic nature and should not alter the actual operation.

I'm really glad you're putting so much effort into reviewing this 
patches, but please just go ahead and change this less important 
stylistic aspects on the fly without unnecessary debates. Currently, we 
all have to fight the burden of stupid LLM generated slop, but we also 
still have to feel responsible for handling human collaborators not like 
willless agents and bots. Insisting on overly picky style guides can be 
really frustrating for voluntary contribution.

Martin

Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Stefano Brivio]

On Sat, 7 Nov 2026 12:50:18 +0100
Martin Schitter <ms+git@mur.at> wrote:

> On 10/21/26 01:56, Stefano Brivio wrote:
> > Martin, thanks for the updated patch, and apologies for the
> > embarrassing delay.  
> 
> I would like to answer: "no problem at all!", but since this issue could 
> be easily exploited for denial-of-service attacks it may not be wise to 
> delay applying these fixes any longer than necessary.

Well, on the other hand, what happens in your setup is that pasta simply
doesn't start, and nobody else reported the issue so far.  But yes, I
agree with you, it's not nice, and you already spent quite some time on
this.

> > It all seems to work for me. I thought I could break this with
> > multipath routes but, even with those, I can't actually build
> > interdependent routes, not even under iproute2's category 2.  
> 
> That's nice to hear!
> 
> >> Fixes: #221  
> > That's for Debian bugs. We use tags just like Linux kernel
> > contributions, that is:  
> 
> It's one of the keywords recognized by nearly all current more 
> comfortable CI/CD solutions and experienced developers. It also 
> shouldn't do any harm otherwise:
> 
> https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern
> https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue
> https://forgejo.org/docs/v15.0/user/collaboration/linked-references/#actionable-references-in-pull-requests-and-commit-messages
> ...

Thanks for the references, I thought it was just for Debian and perhaps
GitHub.  Anyway, my point here is mostly about consistency.  If we had
had this discussion five years ago perhaps we would use different tags
now.

There's also the little problem that (same here, this comes from the
Linux kernel process) we use "Fixes" tags to indicate previous commits
that are fixed with a given change, rather than links to bugs, so that
would kind of overlap, and it wouldn't be possible for me (and possibly
others) to use git hooks such as the one I shared here:

  https://lore.kernel.org/all/20190220213729.49deb54f@redhat.com/

> >> Eliminate the route entries capacity limitations of nl_route_dup
> >> by copping them to the destination namespace in three successive  
> > 
> > (copying)  
> 
> corrected
> 
> >> steps to bypass dependency issues. This approach is taken from
> >> the handling used in the restore subcommand of iproute2.
> >>  
> > 
> > Link: https://bugs.passt.top/show_bug.cgi?id=221  
> 
> added
> 
> >> Signed-off-by: Martin Schitter <ms+git@mur.at>
> >> ---
> >>   netlink.c | 207 ++++++++++++++++++++++++++----------------------------
> >>   1 file changed, 99 insertions(+), 108 deletions(-)
> >>
> >> diff --git a/netlink.c b/netlink.c
> >> index 225b666..803cb20 100644
> >> --- a/netlink.c
> >> +++ b/netlink.c
> >> @@ -525,6 +525,20 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
> >>   	return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
> >>   }
> >>   
> >> +/**
> >> + * nl_rtattr_cmp() -- compare two routing attributes  
> > 
> > With one '-' (kerneldoc), and "Compare" (for consistency).  
> 
> done
> 
> > But what about returning a bool (we use C11 now) and turn this into
> > nl_rtrattr_eq(), so that one doesn't have to wonder if it behaves like
> > memcmp() or the other way around? There's a single non-zero value we
> > return anyway.  
> 
> yes -- it definitely could be done this way as well.
> But isn't it more useful to keep this code sequence as close as possible 
> to its origin in iproute2?

Hmm, I see your point.  I'd still tend to say not because it's more
important to make this easier to grasp inside the codebase itself, and
we almost never have to refer to iproute2 implementations, but that was
a very slight preference on my side.

> At the end it is just a memcmp with some additional protection for 
> problematic corner cases.
> 
> >> + * @rta1:	Routing attribute
> >> + * @rta2:	Routing attribute  
> > 
> > Maybe "First" and "Second", just for the day we finally manage to get
> > all this through Sphinx or similar (same name for two arguments results
> > in some warnings in some documentation system, I can't remember which
> > one, but it kind of makes sense).  
> 
> It's already perfectly correct C code and already used/accepted by 
> another prestigious software project.
> (but you are free to modify the labels anytime if you don't like them!)

It's not about C code, or about usage, it's just that some code
documentation tools (I'm fairly sure Sphinx does) warn if arguments
have the same description.  But I haven't really checked, and
https://bugs.passt.top/show_bug.cgi?id=35 didn't go anywhere yet.

> >> + *
> >> + * Return: 0 if equal
> >> +  */
> >> +static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtattr *rta2)
> >> +{
> >> +	if (!rta1 || !rta2 || rta1->rta_len != rta2->rta_len)
> >> +		return 1;  
> > 
> > We would usually insert an extra newline here for readability.  
> 
> done
> 
> >> +	return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1));
> >> +}
> >> +
> >>   /**
> >>    * nl_route_dup() - Copy routes for given interface and address family
> >>    * @s_src:	Netlink socket in source namespace
> >> @@ -535,56 +549,73 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
> >>    *
> >>    * Return: 0 on success, negative error code on failure
> >>    */
> >> -int nl_route_dup(int s_src, unsigned int ifi_src,
> >> -		 int s_dst, unsigned int ifi_dst, sa_family_t af)
> >> +int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst,
> >> +		 unsigned int ifi_dst, sa_family_t af)  
> > 
> > Unrelated change.  
> 
> (modified by autoformatting operation based on passt/.clang-format)
> 
> reverted
> 
> >>   {
> >> -	struct req_t {
> >> -		struct nlmsghdr nlh;
> >> -		struct rtmsg rtm;
> >> -		struct rtattr rta;
> >> -		unsigned int ifi;
> >> -	} req = {
> >> -		.rtm.rtm_family	  = af,
> >> -		.rtm.rtm_table	  = RT_TABLE_MAIN,
> >> -		.rtm.rtm_scope	  = RT_SCOPE_UNIVERSE,
> >> -		.rtm.rtm_type	  = RTN_UNICAST,
> >> +	/* Duplicate routes in correct order using 3 succesive stages:  
> > 
> > "successive"  
> 
> corrected
> 
> >> +	 * 0. ones for local addresses,
> >> +	 * 1. ones for local networks,
> >> +	 * 2. others (remote networks/hosts).
> >> +	 */
> >> +	for (int prio = 0; prio < 3; prio++) {  
> > 
> > What about making this an enum, so that it's self-documenting? Say:
> > 
> > 	enum route_dup_prio {
> > 		ROUTE_DUP_LOCAL_ADDR,
> > 		ROUTE_DUP_LOCAL_NET,
> > 		ROUTE_DUP_OTHER,
> > 		ROUTE_DUP_COUNT,
> > 	};
> > 
> > 	for (enum prio = 0; prio < ROUTE_DUP_COUNT; prio++) {
> > 		...  
> 
> O.k. -- applied in a slightly less verbose variant.
> 
> >> +		struct req_t {
> >> +			struct nlmsghdr nlh;
> >> +			struct rtmsg rtm;
> >> +			struct rtattr rta;
> >> +			unsigned int ifi;
> >> +		} req = {
> >> +			.rtm.rtm_family = af,
> >> +			.rtm.rtm_table = RT_TABLE_MAIN,
> >> +			.rtm.rtm_scope = RT_SCOPE_UNIVERSE,
> >> +			.rtm.rtm_type = RTN_UNICAST,  
> > 
> > I haven't tried, but what happens if you try to filter on rtm_dst_len
> > in struct rtmsg so that you only get the category of routes you want
> > (at least for the ones representing local addresses that might be
> > possible?), so that you have to go through fewer elements in the loop?  
> 
> I searched the rtnetlink(7) man page and a few other documentation 
> sources, but I couldn't figure out any more useful selective 
> netmask/prefix setting for rtm_dst_len than this current implicit value 
> of 0, which returns all entries.

Sorry, I naively assumed that we could filter on rtm_dst_len being 32 or
128 for local addresses, but looking at ip_valid_fib_dump_req() (which
is also used for IPv6) it doesn't seem to be the case.  The request
would be rejected altogether (!).

> >> +
> >> +			.rta.rta_type = RTA_OIF,
> >> +			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
> >> +			.ifi = ifi_src,  
> > 
> > The way this was indented before (with tabs) looked more readable,
> > because it built some kind of "table".  
> 
> It was using an unpleasant mix of tabs and space characters, which will 
> not always look nice and better readable in all possible code views.

If tabs are 8 characters wide (just like the kernel coding style that
we use), then it all works and it's consistent.

> Nevertheless, I'll try to revert the change.
> 
> >> +		};
> >> +		ssize_t status;
> >> +		struct nlmsghdr *nh;
> >> +		char buf[NLBUFSIZ];
> >> +		uint32_t seq;
> >> +
> >> +		seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP,
> >> +			      sizeof(req));
> >> +
> >> +		nl_foreach(nh, status, s_src, buf, seq) {
> >> +			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
> >> +			struct rtattr *rta;
> >> +			size_t na;
> >> +			uint16_t flags = nh->nlmsg_flags;
> >> +			int rc;  
> > 
> > The usual pyramid tree stuff if possible (see CONTRIBUTING.md).  
> 
> done
> 
> >>   
> >> -		.rta.rta_type	  = RTA_OIF,
> >> -		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
> >> -		.ifi		  = ifi_src,
> >> -	};
> >> -	ssize_t nlmsgs_size, left, status;
> >> -	unsigned dup_routes = 0;
> >> -	struct nlmsghdr *nh;
> >> -	char buf[NLBUFSIZ];
> >> -	uint32_t seq;
> >> -	unsigned i;
> >>     
> > 
> > There's an excess newline here now.
> >   
> >> -	seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
> >> +			if (nh->nlmsg_type != RTM_NEWROUTE)
> >> +				continue;
> >>   
> >> -	/* nl_foreach() will step through multiple response datagrams,
> >> -	 * which we don't want here because we need to have all the
> >> -	 * routes in the buffer at once.
> >> -	 */
> >> -	nh = nl_next(s_src, buf, NULL, &nlmsgs_size);
> >> -	for (left = nlmsgs_size;
> >> -	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
> >> -	     nh = NLMSG_NEXT(nh, left)) {
> >> -		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
> >> -		bool discard = false;
> >> -		struct rtattr *rta;
> >> -		size_t na;
> >> +			struct rtattr *tb[RTA_MAX+1] = { 0 };  
> > 
> > Coding style: [RTA_MAX + 1]  
> 
> done
> 
> >>   
> >> -		if (nh->nlmsg_type != RTM_NEWROUTE)
> >> -			continue;
> >> +			for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh);
> >> +			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
> >> +				tb[rta->rta_type] = rta;  
> > 
> > Coding style: single line for statement, no curly brackets.  
> 
> No! -- extraordinary complex for loop control statements, using commas 
> and spread over multiply lines, are always unnecessary hard to read. 
> Without curly brackets you can hardly grasp the separation between loop 
> control and body on first view.

Hmm, okay.  I would rather blame it on the for loop header at that point
(maybe it could use an iterator macro then).  Fine either way.

> >> +			}
> >>   
> >> -		/* nexthop state flags don't apply to freshly created routes,
> >> -		 * and the kernel will refuse our route if they are set.
> >> -		 */
> >> -		rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
> >> +			/* Process only matching routes of current priority
> >> +			 * (variant of an approach used in iproute2s restore_handler)  
> > 
> > iproute2's (Saxon genitive in English needs an apostrophe),
> > restore_handler() (it's a function name). It should all be wrapped at
> > 80 columns.  
> 
> done
> >> +			 */
> >> +			if (!(prio == 2 && tb[RTA_GATEWAY]) &&  
> > 
> > Can we split this a bit? I think it would be easier to read with
> > multiple conditions and multiple continue statements (three of them
> > perhaps?) rather than all together.  
> 
> I don't think it's much more readable now, but it should behave exactly 
> the same as before, because only DeMorgens law got applied a few times.
> 
> The previews variant looked more similar to the expressions used in 
> iproute2, which I personally would still value as more helpful.
> 
> >> +			    !(prio == 1 && !tb[RTA_GATEWAY] &&  
> > 
> > If the enum labels I proposed earlier are too long for this to be
> > readable, maybe drop "_DUP_" from them.  
> 
> Already done
> 
> >> +			      tb[RTA_PREFSRC] &&
> >> +			      nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) &&
> >> +			    !(prio == 0 && !tb[RTA_GATEWAY] &&
> >> +			      (!tb[RTA_PREFSRC] ||
> >> +			       !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST]))))
> >> +				continue;
> >> +
> >> +			/* nexthop state flags don't apply to freshly created routes,  
> > 
> > Same here, wrap at 80 columns.  
> 
> done
> 
> >> +			 * and the kernel will refuse our route if they are set.
> >> +			 */
> >> +			rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
> >>   
> >> -		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
> >> -		     rta = RTA_NEXT(rta, na)) {
> >>   			/* RTA_OIF and RTA_MULTIPATH attributes carry the
> >>   			 * identifier of a host interface. If they match the
> >>   			 * host interface we're copying from, change them to
> >> @@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
> >>   			 * available), or if any interface index in nexthop
> >>   			 * objects differ from the host interface, discard the
> >>   			 * route altogether.
> >> -			*/
> >> -			if (rta->rta_type == RTA_OIF) {
> >> -				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
> >> -					discard = true;
> >> -					break;
> >> +			 */
> >> +			if (tb[RTA_OIF]) {
> >> +				if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=
> >> +				    ifi_src) {  
> > 
> > Maybe extract it into a temporary variable ('ifi'?).  
> 
> IMHO it wouldn't be much shorter or significant easier to grasp.
> 
> >> +					continue;
> >>   				}  
> > 
> > No curly brackets needed if it's a single statement (syntax) on a single
> > line (coding style).  
> 
> done
> 
> >> -
> >> -				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
> >> -			} else if (rta->rta_type == RTA_MULTIPATH) {
> >> -				int nh_len = RTA_PAYLOAD(rta);
> >> +				*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =
> >> +					ifi_dst;
> >> +			} else if (tb[RTA_MULTIPATH]) {
> >> +				int nh_len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
> >>   				struct rtnexthop *rtnh;
> >> -
> >> -				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
> >> +				bool discard = false;
> >> +				for (rtnh = (struct rtnexthop *)
> >> +					     RTA_DATA(tb[RTA_MULTIPATH]);
> >>   				     RTNH_OK(rtnh, nh_len);
> >> -				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
> >> +				     rtnh = RTNH_NEXT_AND_DEC(rtnh,
> >> +							      nh_len)) {  
> > 
> > This part could remain on a single line (it hurts my eyes but the
> > ugliness is pre-existing, and actually my fault).  
> 
> applied
> 
> >>   					int src = (int)ifi_src;
> >> -
> >>   					if (rtnh->rtnh_ifindex != src) {
> >>   						discard = true;
> >>   						break;
> >>   					}
> >> -
> >>   					rtnh->rtnh_ifindex = ifi_dst;
> >>   				}
> >> -
> >>   				if (discard)
> >> -					break;
> >> -			} else if (rta->rta_type == RTA_PREFSRC ||
> >> -				   rta->rta_type == RTA_NH_ID) {
> >> +					continue;
> >> +			} else if (tb[RTA_PREFSRC]) {
> >>   				/* Strip RTA_PREFSRC attributes: host routes
> >>   				 * might include a preferred source address,
> >>   				 * which must be one of the host's addresses.
> >>   				 * However, with -a, pasta will use a different
> >>   				 * namespace address, making such a route
> >>   				 * invalid in the namespace.
> >> -				 *
> >> -				 * Strip RTA_NH_ID attributes: host routes set
> >> +				 */
> >> +				tb[RTA_PREFSRC]->rta_type = RTA_UNSPEC;
> >> +			} else if (tb[RTA_NH_ID]) {
> >> +				/* Strip RTA_NH_ID attributes: host routes set
> >>   				 * up via routing protocols (e.g. OSPF) might
> >>   				 * contain a nexthop ID (and not nexthop
> >>   				 * objects, which are taken care of in the
> >>   				 * RTA_MULTIPATH case above) that's not valid
> >>   				 * in the target namespace.
> >>   				 */
> >> -				rta->rta_type = RTA_UNSPEC;
> >> +				tb[RTA_NH_ID]->rta_type = RTA_UNSPEC;
> >>   			}
> >> -		}
> >> -
> >> -		if (discard)
> >> -			nh->nlmsg_type = NLMSG_NOOP;
> >> -		else
> >> -			dup_routes++;
> >> -	}
> >> -
> >> -	if (!NLMSG_OK(nh, left)) {
> >> -		/* Process any remaining datagrams in a different
> >> -		 * buffer so we don't overwrite the first one.
> >> -		 */
> >> -		char tail[NLBUFSIZ];
> >> -		unsigned extra = 0;
> >> -
> >> -		nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE)
> >> -			extra++;
> >> -
> >> -		if (extra) {
> >> -			err("netlink: Too many routes to duplicate");
> >> -			return -E2BIG;
> >> -		}
> >> -	}
> >> -	if (status < 0)
> >> -		return status;
> >> -
> >> -	/* Routes might have dependencies between each other, and the kernel
> >> -	 * processes RTM_NEWROUTE messages sequentially. For n routes, we might
> >> -	 * need to send the requests up to n times to get all of them inserted.
> >> -	 * Routes that have been already inserted will return -EEXIST, but we
> >> -	 * can safely ignore that and repeat the requests. This avoids the need
> >> -	 * to calculate dependencies: let the kernel do that.
> >> -	 */
> >> -	for (i = 0; i < dup_routes; i++) {
> >> -		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
> >> -		     NLMSG_OK(nh, left);
> >> -		     nh = NLMSG_NEXT(nh, left)) {
> >> -			uint16_t flags = nh->nlmsg_flags;
> >> -			int rc;
> >> -
> >> -			if (nh->nlmsg_type != RTM_NEWROUTE)
> >> -				continue;
> >>   
> >>   			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
> >> -				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
> >> +				   (flags & ~NLM_F_DUMP_FILTERED) |
> >> +					   NLM_F_CREATE,  
> > 
> > Reasonable to fix this here, but it should be aligned to its logical
> > meaning, that is:
> > 
> > 				   (flags & ~NLM_F_DUMP_FILTERED) |
> > 				   NLM_F_CREATE,   
> 
> reformated
> 
> >>   				   nh->nlmsg_len);
> >>   			if (rc < 0 && rc != -EEXIST &&
> >>   			    rc != -ENETUNREACH && rc != -EHOSTUNREACH)  
> > 
> > On one hand, EEXIST, ENETUNREACH, and EHOSTUNREACH should all be
> > considered actual errors now. On the other hand it's probably more
> > robust to carry on as if nothing happened even if we hit them. I'm a
> > bit undecided but I'd rather leave this part as it is.  
> I personally would be biased to add at least some debug logs in case of 
> these errors,

Hmm, yes, that could be a good idea actually, but it can be added later.

Even with a netlink monitor running frequently, they shouldn't happen
routinely, so there should be no risk of making logs unusable.

> but to keep passt/pasta based services reliable working, I 
> also wouldn't enable more strict error handlers or execution abort.
> 
> Right now we don't check for NLM_F_DUMP_INTR 
> (https://docs.kernel.org/userspace-api/netlink/intro.html#dump-consistency) 
> and we even combine three successive route table dumps with minimal 
> temporal gaps between them in our processing. Therefore, we have to 
> consider that we always could get slightly inconsistent results causing 
> these errors.

Ah, right, I didn't consider that.

> I don't see any satisfying simple solution for this problem. Aborting 
> the execution or recursive retry attempts in case of errors would just 
> open another door for denial-of-service attacks.

Yes, definitely, let's just leave it like that.

> >>   				return rc;
> >>   		}
> >> +		if (status < 0)
> >> +			return status;
> >>   	}
> >> -  
> > 
> > I think you could leave this one, it's a bit easier on eyes.
> >   
> >>   	return 0;
> >>   }
> >>     
> > 
> > Hah, so simple? If I had known. :)  
> 
> Well -- as far as I can see, all modifications of v5 are only of 
> cosmetic nature and should not alter the actual operation.

From a very quick look, I think so too.  I need to have another calmer
look but I think it's ready to be merged as it is.

> I'm really glad you're putting so much effort into reviewing this 
> patches, but please just go ahead and change this less important 
> stylistic aspects on the fly without unnecessary debates.

Okay, I will, if "needed".

> Currently, we 
> all have to fight the burden of stupid LLM generated slop, but we also 
> still have to feel responsible for handling human collaborators not like 
> willless agents and bots. Insisting on overly picky style guides can be 
> really frustrating for voluntary contribution.

Oops, my bad, I'm sorry you felt that way.  You already mentioned on an
earlier version that I could just go ahead and fix up coding style, but
I had more substantial comments than that on v4, so I thought I'd just
give you all my comments anyway.  In some spots, I didn't want to tweak
your code without even asking for your opinion.

And given that this fix is a bit more than a fix, and genuinely valuable
development, spending a bit of time on coding style comments and
related explanations makes sense an investment for me, because I hope
you'll contribute further in the future, and in that case that would
save me time.

In general, I'm trying to be picky about style because it helps _all_
contributors in the long term.  If it's frustrating for occasional
contributors, perhaps I should opt more frequently (I already do that
sometimes) to just fix details up on merge.  Feedback taken, thanks.

-- 
Stefano