From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=fail (p=quarantine dis=none) header.from=redhat.com Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 017115A0265 for ; Sat, 07 Nov 2026 21:02:52 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1794081771; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xNh36oMKpwxDDq2nUlosV4oj+q0vW/MFQowttjeA6Uo=; b=Oh2z/g49YLIRljQvQqgH7zWMq3nQYbl0heg8DJWBxRnSwIHxrGb5XRGZVKEsfnqGglZ943 63LL+WF/qtyPoFAm+EcNAA1Im46Way7U3ZUGwAZyXYbdlgC9umajxdabX4wcatCmN1xEND 51Bi4AnfUJqK+Rz5zgbsRpDnpE8fS+k= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-695-AeVeOkWTN9qPEsLoDdmC8A-1; Sat, 07 Nov 2026 15:02:47 -0500 X-MC-Unique: AeVeOkWTN9qPEsLoDdmC8A-1 X-Mimecast-MFC-AGG-ID: AeVeOkWTN9qPEsLoDdmC8A_1794081766 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4b64722b5e6so19418465e9.2 for ; Sat, 07 Nov 2026 12:02:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1794081766; x=1794686566; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=6xdbPBSycczCU1OPwqBTruND2JZjOVuPvg3k6pzQGqU=; b=PWGxKx7/ljvdOl6UARZHpzCcE24r1C8wx6ac22G03a63UauM57WPjcMuV6mlMOtOeb k49A1VMhiFKtZkwRoUAbKOO6inyHQzNwt18tucgHq+MJVk9lnf6YTO2s2tdNckV0kZey Q3wB/7h1ZHP6etR9uIFW6cnQM7wFDR4BEYJ2y4+HBtlGviL2QFD68FVqj88xaeBH910g AY3lds0uJq4LD2b1NGHf6o6739yT8sBsOeSCvh2eFLiEiqtNyT74aABN6yY4LqeisIce rNHBRVpgegP0rWFExLVHKVA1bNHtyd3+COiXxdpko+hMJBuuO/ktX9V0JUVhe5AF6Wbm 9NBw== X-Gm-Message-State: APuU4Z9BhW+Sn0TGo5AMBmrE9+NkLp6V7rCAGYDBhD/9w0oLk1ttvkDA vYejjBkEJNALjql6eMRfO4lPO6Fg5wESsAdfBTLgsk0K9W1CRcwmVlHrggNaE4RicX9QRVGJuzj YBi4mWv4fImq0Ic+Kw0c39NC3Xs9hRGtwMC/0BqGJYZXb8b2GHsn8IX1ogmlbUw== X-Gm-Gg: AYBFou14mBrOwFV+dxrNrndK6O7atsPnygcUeeHiLR4qp6RpvdhQSpuTRE376lNWmUV MJnNBs4UZo2/HdFNTBHknqRVyK7ZVvejMqzpZo/cJvVmfqgG04i2f8hjxlCkMsBIhZmvhY1oYkH HyzqjLxbNPXA6bDWI3Nz9FJhamIEmrbemo3a8AD3C3GOmrAsBxrQbTgLdBchCwC5exqD9ucZV2y X2htzooaXMID1fkwEeVxUDq5mfuhEwor4hhfpo08e3afc1zygZ38j8Bx8nrq1sSOkG9Dd4NSSrO X8YT96/aT08PFvmXFFfz4aXxxYQQhr09ItYqsBFaflxyBuY882+BaY+1I6JxnnRE7lCXC9EQCRr uwavj3aJxyoWl/qc2FFFWcCF4sOmjxzODWX8cDL84dIZM3hNYO/A9JG2kqWygXAky3cxPdYX2My Z6J1osFln3 X-Received: by 2002:a05:600d:82e3:b0:4a1:9661:14b5 with SMTP id 5b1f17b1804b1-4b6dff87583mr126216015e9.29.1794081765694; Sat, 07 Nov 2026 12:02:45 -0800 (PST) X-Received: by 2002:a05:600d:82e3:b0:4a1:9661:14b5 with SMTP id 5b1f17b1804b1-4b6dff87583mr126215475e9.29.1794081764815; Sat, 07 Nov 2026 12:02:44 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-492fc80e017sm10656037f8f.34.2026.11.07.12.02.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 07 Nov 2026 12:02:40 -0800 (PST) From: Stefano Brivio To: Martin Schitter Subject: Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing Message-ID: <20261107210237.59bcb1cf@elisabeth> In-Reply-To: References: <20260926120520.809653-1-ms+git@mur.at> <20260926120520.809653-2-ms+git@mur.at> <20261021015651.582339bd@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Sat, 07 Nov 2026 21:02:39 +0100 (CET) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: YMaFCNV2-lprromWobHGOK5TxG-OmoJ7jAHeoF5YmRY_1794081766 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Message-ID-Hash: KKT4M4F47ZJ2LSSMDR3VSLBBZRRRTG27 X-Message-ID-Hash: KKT4M4F47ZJ2LSSMDR3VSLBBZRRRTG27 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. =20 >=20 > I would like to answer: "no problem at all!", but since this issue could= =20 > be easily exploited for denial-of-service attacks it may not be wise to= =20 > 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. =20 >=20 > That's nice to hear! >=20 > >> Fixes: #221 =20 > > That's for Debian bugs. We use tags just like Linux kernel > > contributions, that is: =20 >=20 > It's one of the keywords recognized by nearly all current more=20 > comfortable CI/CD solutions and experienced developers. It also=20 > shouldn't do any harm otherwise: >=20 > https://docs.gitlab.com/user/project/issues/managing_issues/#default-clos= ing-pattern > https://docs.github.com/en/get-started/writing-on-github/working-with-adv= anced-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/#acti= onable-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 =20 > >=20 > > (copying) =20 >=20 > corrected >=20 > >> steps to bypass dependency issues. This approach is taken from > >> the handling used in the restore subcommand of iproute2. > >> =20 > >=20 > > Link: https://bugs.passt.top/show_bug.cgi?id=3D221 =20 >=20 > added >=20 > >> 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) > >> =09return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, le= n); > >> } > >> =20 > >> +/** > >> + * nl_rtattr_cmp() -- compare two routing attributes =20 > >=20 > > With one '-' (kerneldoc), and "Compare" (for consistency). =20 >=20 > done >=20 > > 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. =20 >=20 > 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= =20 > 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=20 > problematic corner cases. >=20 > >> + * @rta1:=09Routing attribute > >> + * @rta2:=09Routing attribute =20 > >=20 > > 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). =20 >=20 > It's already perfectly correct C code and already used/accepted by=20 > 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=3D35 didn't go anywhere yet. > >> + * > >> + * Return: 0 if equal > >> + */ > >> +static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtat= tr *rta2) > >> +{ > >> +=09if (!rta1 || !rta2 || rta1->rta_len !=3D rta2->rta_len) > >> +=09=09return 1; =20 > >=20 > > We would usually insert an extra newline here for readability. =20 >=20 > done >=20 > >> +=09return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1)); > >> +} > >> + > >> /** > >> * nl_route_dup() - Copy routes for given interface and address fami= ly > >> * @s_src:=09Netlink 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, > >> -=09=09 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, > >> +=09=09 unsigned int ifi_dst, sa_family_t af) =20 > >=20 > > Unrelated change. =20 >=20 > (modified by autoformatting operation based on passt/.clang-format) >=20 > reverted >=20 > >> { > >> -=09struct req_t { > >> -=09=09struct nlmsghdr nlh; > >> -=09=09struct rtmsg rtm; > >> -=09=09struct rtattr rta; > >> -=09=09unsigned int ifi; > >> -=09} req =3D { > >> -=09=09.rtm.rtm_family=09 =3D af, > >> -=09=09.rtm.rtm_table=09 =3D RT_TABLE_MAIN, > >> -=09=09.rtm.rtm_scope=09 =3D RT_SCOPE_UNIVERSE, > >> -=09=09.rtm.rtm_type=09 =3D RTN_UNICAST, > >> +=09/* Duplicate routes in correct order using 3 succesive stages: =20 > >=20 > > "successive" =20 >=20 > corrected >=20 > >> +=09 * 0. ones for local addresses, > >> +=09 * 1. ones for local networks, > >> +=09 * 2. others (remote networks/hosts). > >> +=09 */ > >> +=09for (int prio =3D 0; prio < 3; prio++) { =20 > >=20 > > What about making this an enum, so that it's self-documenting? Say: > >=20 > > =09enum route_dup_prio { > > =09=09ROUTE_DUP_LOCAL_ADDR, > > =09=09ROUTE_DUP_LOCAL_NET, > > =09=09ROUTE_DUP_OTHER, > > =09=09ROUTE_DUP_COUNT, > > =09}; > >=20 > > =09for (enum prio =3D 0; prio < ROUTE_DUP_COUNT; prio++) { > > =09=09... =20 >=20 > O.k. -- applied in a slightly less verbose variant. >=20 > >> +=09=09struct req_t { > >> +=09=09=09struct nlmsghdr nlh; > >> +=09=09=09struct rtmsg rtm; > >> +=09=09=09struct rtattr rta; > >> +=09=09=09unsigned int ifi; > >> +=09=09} req =3D { > >> +=09=09=09.rtm.rtm_family =3D af, > >> +=09=09=09.rtm.rtm_table =3D RT_TABLE_MAIN, > >> +=09=09=09.rtm.rtm_scope =3D RT_SCOPE_UNIVERSE, > >> +=09=09=09.rtm.rtm_type =3D RTN_UNICAST, =20 > >=20 > > 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? = =20 >=20 > I searched the rtnetlink(7) man page and a few other documentation=20 > sources, but I couldn't figure out any more useful selective=20 > netmask/prefix setting for rtm_dst_len than this current implicit value= =20 > 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 (!). > >> + > >> +=09=09=09.rta.rta_type =3D RTA_OIF, > >> +=09=09=09.rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)), > >> +=09=09=09.ifi =3D ifi_src, =20 > >=20 > > The way this was indented before (with tabs) looked more readable, > > because it built some kind of "table". =20 >=20 > It was using an unpleasant mix of tabs and space characters, which will= =20 > 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. >=20 > >> +=09=09}; > >> +=09=09ssize_t status; > >> +=09=09struct nlmsghdr *nh; > >> +=09=09char buf[NLBUFSIZ]; > >> +=09=09uint32_t seq; > >> + > >> +=09=09seq =3D nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, > >> +=09=09=09 sizeof(req)); > >> + > >> +=09=09nl_foreach(nh, status, s_src, buf, seq) { > >> +=09=09=09struct rtmsg *rtm =3D (struct rtmsg *)NLMSG_DATA(nh); > >> +=09=09=09struct rtattr *rta; > >> +=09=09=09size_t na; > >> +=09=09=09uint16_t flags =3D nh->nlmsg_flags; > >> +=09=09=09int rc; =20 > >=20 > > The usual pyramid tree stuff if possible (see CONTRIBUTING.md). =20 >=20 > done >=20 > >> =20 > >> -=09=09.rta.rta_type=09 =3D RTA_OIF, > >> -=09=09.rta.rta_len=09 =3D RTA_LENGTH(sizeof(unsigned int)), > >> -=09=09.ifi=09=09 =3D ifi_src, > >> -=09}; > >> -=09ssize_t nlmsgs_size, left, status; > >> -=09unsigned dup_routes =3D 0; > >> -=09struct nlmsghdr *nh; > >> -=09char buf[NLBUFSIZ]; > >> -=09uint32_t seq; > >> -=09unsigned i; > >> =20 > >=20 > > There's an excess newline here now. > > =20 > >> -=09seq =3D nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req)= ); > >> +=09=09=09if (nh->nlmsg_type !=3D RTM_NEWROUTE) > >> +=09=09=09=09continue; > >> =20 > >> -=09/* nl_foreach() will step through multiple response datagrams, > >> -=09 * which we don't want here because we need to have all the > >> -=09 * routes in the buffer at once. > >> -=09 */ > >> -=09nh =3D nl_next(s_src, buf, NULL, &nlmsgs_size); > >> -=09for (left =3D nlmsgs_size; > >> -=09 NLMSG_OK(nh, left) && (status =3D nl_status(nh, left, seq)) >= 0; > >> -=09 nh =3D NLMSG_NEXT(nh, left)) { > >> -=09=09struct rtmsg *rtm =3D (struct rtmsg *)NLMSG_DATA(nh); > >> -=09=09bool discard =3D false; > >> -=09=09struct rtattr *rta; > >> -=09=09size_t na; > >> +=09=09=09struct rtattr *tb[RTA_MAX+1] =3D { 0 }; =20 > >=20 > > Coding style: [RTA_MAX + 1] =20 >=20 > done >=20 > >> =20 > >> -=09=09if (nh->nlmsg_type !=3D RTM_NEWROUTE) > >> -=09=09=09continue; > >> +=09=09=09for (rta =3D RTM_RTA(rtm), na =3D RTM_PAYLOAD(nh); > >> +=09=09=09 RTA_OK(rta, na); rta =3D RTA_NEXT(rta, na)) { > >> +=09=09=09=09tb[rta->rta_type] =3D rta; =20 > >=20 > > Coding style: single line for statement, no curly brackets. =20 >=20 > No! -- extraordinary complex for loop control statements, using commas=20 > and spread over multiply lines, are always unnecessary hard to read.=20 > Without curly brackets you can hardly grasp the separation between loop= =20 > 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. > >> +=09=09=09} > >> =20 > >> -=09=09/* nexthop state flags don't apply to freshly created routes, > >> -=09=09 * and the kernel will refuse our route if they are set. > >> -=09=09 */ > >> -=09=09rtm->rtm_flags &=3D ~RTNH_COMPARE_MASK; > >> +=09=09=09/* Process only matching routes of current priority > >> +=09=09=09 * (variant of an approach used in iproute2s restore_handler= ) =20 > >=20 > > iproute2's (Saxon genitive in English needs an apostrophe), > > restore_handler() (it's a function name). It should all be wrapped at > > 80 columns. =20 >=20 > done > >> +=09=09=09 */ > >> +=09=09=09if (!(prio =3D=3D 2 && tb[RTA_GATEWAY]) && =20 > >=20 > > 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. =20 >=20 > I don't think it's much more readable now, but it should behave exactly= =20 > the same as before, because only DeMorgens law got applied a few times. >=20 > The previews variant looked more similar to the expressions used in=20 > iproute2, which I personally would still value as more helpful. >=20 > >> +=09=09=09 !(prio =3D=3D 1 && !tb[RTA_GATEWAY] && =20 > >=20 > > If the enum labels I proposed earlier are too long for this to be > > readable, maybe drop "_DUP_" from them. =20 >=20 > Already done >=20 > >> +=09=09=09 tb[RTA_PREFSRC] && > >> +=09=09=09 nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) && > >> +=09=09=09 !(prio =3D=3D 0 && !tb[RTA_GATEWAY] && > >> +=09=09=09 (!tb[RTA_PREFSRC] || > >> +=09=09=09 !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])))) > >> +=09=09=09=09continue; > >> + > >> +=09=09=09/* nexthop state flags don't apply to freshly created routes= , =20 > >=20 > > Same here, wrap at 80 columns. =20 >=20 > done >=20 > >> +=09=09=09 * and the kernel will refuse our route if they are set. > >> +=09=09=09 */ > >> +=09=09=09rtm->rtm_flags &=3D ~RTNH_COMPARE_MASK; > >> =20 > >> -=09=09for (rta =3D RTM_RTA(rtm), na =3D RTM_PAYLOAD(nh); RTA_OK(rta, = na); > >> -=09=09 rta =3D RTA_NEXT(rta, na)) { > >> =09=09=09/* RTA_OIF and RTA_MULTIPATH attributes carry the > >> =09=09=09 * identifier of a host interface. If they match the > >> =09=09=09 * host interface we're copying from, change them to > >> @@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_sr= c, > >> =09=09=09 * available), or if any interface index in nexthop > >> =09=09=09 * objects differ from the host interface, discard the > >> =09=09=09 * route altogether. > >> -=09=09=09*/ > >> -=09=09=09if (rta->rta_type =3D=3D RTA_OIF) { > >> -=09=09=09=09if (*(unsigned int *)RTA_DATA(rta) !=3D ifi_src) { > >> -=09=09=09=09=09discard =3D true; > >> -=09=09=09=09=09break; > >> +=09=09=09 */ > >> +=09=09=09if (tb[RTA_OIF]) { > >> +=09=09=09=09if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=3D > >> +=09=09=09=09 ifi_src) { =20 > >=20 > > Maybe extract it into a temporary variable ('ifi'?). =20 >=20 > IMHO it wouldn't be much shorter or significant easier to grasp. >=20 > >> +=09=09=09=09=09continue; > >> =09=09=09=09} =20 > >=20 > > No curly brackets needed if it's a single statement (syntax) on a singl= e > > line (coding style). =20 >=20 > done >=20 > >> - > >> -=09=09=09=09*(unsigned int *)RTA_DATA(rta) =3D ifi_dst; > >> -=09=09=09} else if (rta->rta_type =3D=3D RTA_MULTIPATH) { > >> -=09=09=09=09int nh_len =3D RTA_PAYLOAD(rta); > >> +=09=09=09=09*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =3D > >> +=09=09=09=09=09ifi_dst; > >> +=09=09=09} else if (tb[RTA_MULTIPATH]) { > >> +=09=09=09=09int nh_len =3D RTA_PAYLOAD(tb[RTA_MULTIPATH]); > >> =09=09=09=09struct rtnexthop *rtnh; > >> - > >> -=09=09=09=09for (rtnh =3D (struct rtnexthop *)RTA_DATA(rta); > >> +=09=09=09=09bool discard =3D false; > >> +=09=09=09=09for (rtnh =3D (struct rtnexthop *) > >> +=09=09=09=09=09 RTA_DATA(tb[RTA_MULTIPATH]); > >> =09=09=09=09 RTNH_OK(rtnh, nh_len); > >> -=09=09=09=09 rtnh =3D RTNH_NEXT_AND_DEC(rtnh, nh_len)) { > >> +=09=09=09=09 rtnh =3D RTNH_NEXT_AND_DEC(rtnh, > >> +=09=09=09=09=09=09=09 nh_len)) { =20 > >=20 > > This part could remain on a single line (it hurts my eyes but the > > ugliness is pre-existing, and actually my fault). =20 >=20 > applied >=20 > >> =09=09=09=09=09int src =3D (int)ifi_src; > >> - > >> =09=09=09=09=09if (rtnh->rtnh_ifindex !=3D src) { > >> =09=09=09=09=09=09discard =3D true; > >> =09=09=09=09=09=09break; > >> =09=09=09=09=09} > >> - > >> =09=09=09=09=09rtnh->rtnh_ifindex =3D ifi_dst; > >> =09=09=09=09} > >> - > >> =09=09=09=09if (discard) > >> -=09=09=09=09=09break; > >> -=09=09=09} else if (rta->rta_type =3D=3D RTA_PREFSRC || > >> -=09=09=09=09 rta->rta_type =3D=3D RTA_NH_ID) { > >> +=09=09=09=09=09continue; > >> +=09=09=09} else if (tb[RTA_PREFSRC]) { > >> =09=09=09=09/* Strip RTA_PREFSRC attributes: host routes > >> =09=09=09=09 * might include a preferred source address, > >> =09=09=09=09 * which must be one of the host's addresses. > >> =09=09=09=09 * However, with -a, pasta will use a different > >> =09=09=09=09 * namespace address, making such a route > >> =09=09=09=09 * invalid in the namespace. > >> -=09=09=09=09 * > >> -=09=09=09=09 * Strip RTA_NH_ID attributes: host routes set > >> +=09=09=09=09 */ > >> +=09=09=09=09tb[RTA_PREFSRC]->rta_type =3D RTA_UNSPEC; > >> +=09=09=09} else if (tb[RTA_NH_ID]) { > >> +=09=09=09=09/* Strip RTA_NH_ID attributes: host routes set > >> =09=09=09=09 * up via routing protocols (e.g. OSPF) might > >> =09=09=09=09 * contain a nexthop ID (and not nexthop > >> =09=09=09=09 * objects, which are taken care of in the > >> =09=09=09=09 * RTA_MULTIPATH case above) that's not valid > >> =09=09=09=09 * in the target namespace. > >> =09=09=09=09 */ > >> -=09=09=09=09rta->rta_type =3D RTA_UNSPEC; > >> +=09=09=09=09tb[RTA_NH_ID]->rta_type =3D RTA_UNSPEC; > >> =09=09=09} > >> -=09=09} > >> - > >> -=09=09if (discard) > >> -=09=09=09nh->nlmsg_type =3D NLMSG_NOOP; > >> -=09=09else > >> -=09=09=09dup_routes++; > >> -=09} > >> - > >> -=09if (!NLMSG_OK(nh, left)) { > >> -=09=09/* Process any remaining datagrams in a different > >> -=09=09 * buffer so we don't overwrite the first one. > >> -=09=09 */ > >> -=09=09char tail[NLBUFSIZ]; > >> -=09=09unsigned extra =3D 0; > >> - > >> -=09=09nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE) > >> -=09=09=09extra++; > >> - > >> -=09=09if (extra) { > >> -=09=09=09err("netlink: Too many routes to duplicate"); > >> -=09=09=09return -E2BIG; > >> -=09=09} > >> -=09} > >> -=09if (status < 0) > >> -=09=09return status; > >> - > >> -=09/* Routes might have dependencies between each other, and the kern= el > >> -=09 * processes RTM_NEWROUTE messages sequentially. For n routes, we = might > >> -=09 * need to send the requests up to n times to get all of them inse= rted. > >> -=09 * Routes that have been already inserted will return -EEXIST, but= we > >> -=09 * can safely ignore that and repeat the requests. This avoids the= need > >> -=09 * to calculate dependencies: let the kernel do that. > >> -=09 */ > >> -=09for (i =3D 0; i < dup_routes; i++) { > >> -=09=09for (nh =3D (struct nlmsghdr *)buf, left =3D nlmsgs_size; > >> -=09=09 NLMSG_OK(nh, left); > >> -=09=09 nh =3D NLMSG_NEXT(nh, left)) { > >> -=09=09=09uint16_t flags =3D nh->nlmsg_flags; > >> -=09=09=09int rc; > >> - > >> -=09=09=09if (nh->nlmsg_type !=3D RTM_NEWROUTE) > >> -=09=09=09=09continue; > >> =20 > >> =09=09=09rc =3D nl_do(s_dst, nh, RTM_NEWROUTE, > >> -=09=09=09=09 (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE, > >> +=09=09=09=09 (flags & ~NLM_F_DUMP_FILTERED) | > >> +=09=09=09=09=09 NLM_F_CREATE, =20 > >=20 > > Reasonable to fix this here, but it should be aligned to its logical > > meaning, that is: > >=20 > > =09=09=09=09 (flags & ~NLM_F_DUMP_FILTERED) | > > =09=09=09=09 NLM_F_CREATE, =20 >=20 > reformated >=20 > >> =09=09=09=09 nh->nlmsg_len); > >> =09=09=09if (rc < 0 && rc !=3D -EEXIST && > >> =09=09=09 rc !=3D -ENETUNREACH && rc !=3D -EHOSTUNREACH) =20 > >=20 > > 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. =20 > I personally would be biased to add at least some debug logs in case of= =20 > 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=20 > also wouldn't enable more strict error handlers or execution abort. >=20 > Right now we don't check for NLM_F_DUMP_INTR=20 > (https://docs.kernel.org/userspace-api/netlink/intro.html#dump-consistenc= y)=20 > and we even combine three successive route table dumps with minimal=20 > temporal gaps between them in our processing. Therefore, we have to=20 > consider that we always could get slightly inconsistent results causing= =20 > these errors. Ah, right, I didn't consider that. > I don't see any satisfying simple solution for this problem. Aborting=20 > the execution or recursive retry attempts in case of errors would just=20 > open another door for denial-of-service attacks. Yes, definitely, let's just leave it like that. > >> =09=09=09=09return rc; > >> =09=09} > >> +=09=09if (status < 0) > >> +=09=09=09return status; > >> =09} > >> - =20 > >=20 > > I think you could leave this one, it's a bit easier on eyes. > > =20 > >> =09return 0; > >> } > >> =20 > >=20 > > Hah, so simple? If I had known. :) =20 >=20 > Well -- as far as I can see, all modifications of v5 are only of=20 > 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=20 > patches, but please just go ahead and change this less important=20 > stylistic aspects on the fly without unnecessary debates. Okay, I will, if "needed". > Currently, we=20 > all have to fight the burden of stupid LLM generated slop, but we also=20 > still have to feel responsible for handling human collaborators not like= =20 > willless agents and bots. Insisting on overly picky style guides can be= =20 > 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. --=20 Stefano