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 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 --- 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

On Sat, 7 Nov 2026 12:50:18 +0100 Martin Schitter 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... https://docs.github.com/en/get-started/writing-on-github/working-with-advanc... https://forgejo.org/docs/v15.0/user/collaboration/linked-references/#actiona... ...

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 --- 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