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 137245A0269 for ; Wed, 21 Oct 2026 01:57:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1792540623; 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=nor9C0v3ERup1WAps6HT/oRqVrKk1+pWorDZenkcUtE=; b=fVl19SqUEEG/otrPRnS2I4KzISBry5++pW70hIayEKCR5on/tVZarg1qHHN+ZHZBtDt+Ni Mg6MZJP3j3m+IQpvarVAydr9k1ieQEhA5l5Qsf6kb4dpESwWnGoIrUyV3xtfQZ48K1Jtxt wZrI+TzD7l5KaEOd8pym0DrBobYwDMM= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-465-_GcH6cuiO6adBym4ou-P4Q-1; Tue, 20 Oct 2026 23:56:55 +0000 X-MC-Unique: _GcH6cuiO6adBym4ou-P4Q-1 X-Mimecast-MFC-AGG-ID: _GcH6cuiO6adBym4ou-P4Q_1792540615 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4a1d7663a4dso909565e9.2 for ; Tue, 20 Oct 2026 16:56:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1792540614; x=1793145414; 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=nor9C0v3ERup1WAps6HT/oRqVrKk1+pWorDZenkcUtE=; b=BGnUjB1B22PfsFuamcsxhb+5M3oWQWs2YsWq5VtactYfZlAzurj5voGH1xiPuq44KW nCjuPKi+kLVDYEdHi8LAVVf6W2AtITJOFxXm5UymBCRsrdFNdU2VqoqFIcqqifx81cdx 1jJqgmcQbI3Ima96jEEOofEFQQIEZJVMhzLwWQwmIQNKPEWiQ2QF4MuqCG3ndd1FAq3s LIpRERkP6kg9y/VJaRdVNseNu25ZRduVFWxqx+4M9rGXrkP7gFx2cGNvLAW8ZyvdZ62X 8tFpkJV31zuy0OnlXZjC4RvoZMIJXENWd+tmZv2O7+GIEuFx88VQ5QQUa+31bF8Fv8v4 /S7g== X-Gm-Message-State: AFq9FYI2mF/N7javJSlHTFB8S+nF3VasS4sD9Jg3sD6HQ2XiBwu6hneY S5VBtgGEbOHd+tLW/ufsul7p89I1T+w7FNBQpff/efmdHIX5jD0B0frcYKY11RpFAbugFsWz0O6 rNjAlAnwQxHlty8vIIbuUz1BImyWoDrNJKkq/c/wbqo0iuomDwXRLCA== X-Gm-Gg: AYBFou0gDAih+sYCBGHmdWVARnFDkmuqH8Vsjpce5i5688Tho5lR+SEi2DZ6niwAXON 3zxY4ZDZSuuXWPrWavylZyDDLF2NGOwP53etDpymGptctfspZe7/uBsVZ1EjzY0v5JWFkLJy/3w ckjGWmv4dkRZ/w21OWjVrPYRr4n5O57NWnunYKAJ6BIq6vcuAtPg+hmUBSjHAkFUO5Lhi5heBXB JVULhZXrSMUDiZU4HNiQ5Xx7Xud2FNsEjIe+Pw/84ZxmCtas8RxzJFTotLZ00673c/aB6JAlvQ4 dO8EwyXH/spP775XRmPZh2XFiL2ryMf0z84a9nnB4V6f/Aqi0pVbwyPfnZ303h7Q8mE9NF4H62w +7yB525cFC3YZl4S5l71YwvxEm7U= X-Received: by 2002:a05:600c:a00d:b0:4a1:cd83:f027 with SMTP id 5b1f17b1804b1-4a1fdd09bd5mr4806705e9.27.1792540614369; Tue, 20 Oct 2026 16:56:54 -0700 (PDT) X-Received: by 2002:a05:600c:a00d:b0:4a1:cd83:f027 with SMTP id 5b1f17b1804b1-4a1fdd09bd5mr4806315e9.27.1792540613760; Tue, 20 Oct 2026 16:56:53 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4a1f2d43242sm102001385e9.9.2026.10.20.16.56.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Oct 2026 16:56:52 -0700 (PDT) From: Stefano Brivio To: Martin Schitter Subject: Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing Message-ID: <20261021015651.582339bd@elisabeth> In-Reply-To: <20260926120520.809653-2-ms+git@mur.at> References: <20260926120520.809653-1-ms+git@mur.at> <20260926120520.809653-2-ms+git@mur.at> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 21 Oct 2026 01:56:52 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: jqeWk6ZDKHsWChgg02Vt9tLNM6Ymwup5C85_qjNu0Ak_1792540615 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: O6PJANZX2JI4VKSZOTIYULLYX4U7NJG7 X-Message-ID-Hash: O6PJANZX2JI4VKSZOTIYULLYX4U7NJG7 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: 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