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

[Martin Schitter <ms+git@mur.at>]

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