On Mon, 12 Oct 2026 19:26:31 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The consumed-SRH tests in
> host_srv6_end_flv_consumed_srh_tests()…
> - [Low] setup_rt_consumed_srh() runs `ip -netns ... -6 route add "${sid}"
> table 90 encap seg6 mode inline segs ... dev dum0` and does not check…
>
> --
>
> [snip]
>
> [Severity: Medium]
> If rt-2 removes the wrong SRH, or no SRH at all, would these tests notice?
>
> [snip]
>
> So two possible kernel regressions on the multi-SRH path would still pass:
>
> - end_flv8986_core() takes srhoff from the first routing header instead of
> the one found with IP6_FH_F_SKIP_RH. seg6_pop_srh() then removes the
> consumed SRH at offset 40 and leaves
> "IPv6 | Policy's SRH, SL=0 | ICMPv6".
>
> - PSP falls back to plain End without calling seg6_pop_srh(), which leaves
> "IPv6 | extra SRH, SL=0 | Policy's SRH, SL=0 | ICMPv6".
>
> In both cases ipv6_srh_rcv() skips each SL=0 SRH, the echo request is
> delivered, and the ping succeeds.
>
> The comment and the commit message say "PSP itself is checked by the PSP
> flavor and NEXT-C-SID,PSP tests". Those tests only carry a single SRH, so
> srhoff is always sizeof(struct ipv6hdr). They never pick an SRH at
> srhoff > 40, and they never run the positional nexthdr fix-up in the else
> branch of seg6_pop_srh():
>
> if (srhoff == sizeof(*iph)) {
> iph->nexthdr = srh_nexthdr;
> } else {
> ...
>
> Could a host-side check of what is left in the packet close this gap? One
> option is an ip6tables "-m srh" match on the host's veth0. It would accept
> only a first SRH with --srh-last-entry-eq 1 and --srh-next-hdr 58.
>
> [snip]
The consumed-SRH tests in this patch would not notice these cases.
However, the suggested match is not enough for the first case. In (i.b)
and (i.d), the Policy's SRH also has Last Entry 1 and is followed by
ICMPv6, and the packet is still accepted. I checked this on a kernel
where I added this regression on purpose.
A check on Segment List[0] of the first SRH would detect this case. In
the extra SRH it is the SID that triggered the insertion, while in the
Policy's SRH it is an address of the host.
I will address this finding in v2.
> [snip]
>
> > +setup_rt_consumed_srh()
> > +{
> > + local rt="$1"
> > + local sid="$2"
> > + local end_rt="$3"
> > + local nsname
> > +
> > + nsname="$(get_rtname "${rt}")"
> > +
> > + ip -netns "${nsname}" -6 route \
> > + add "${sid}" table "${LOCALSID_TABLE_ID}" \
> > + encap seg6 mode inline \
> > + segs "${LOCATOR_SERVICE}:${end_rt}::${END_FUNC}" \
> > + dev "${DUMMY_DEVNAME}"
> > +}
>
> [Severity: Low]
> Should setup_rt_consumed_srh() check the exit status of this ip route add?
>
> The test body runs with set +e, so a failure here is ignored. Without the
> route, the extra SRH is never inserted.
>
> The packet would then follow the plain Policy path that the earlier PSP and
> NEXT-C-SID tests already cover. The ping would succeed, since the hosts have
> seg6_enabled=1. log_test would then record a pass for the consumed SRH case
> without running that path.
>
> I could not find a way for this route add to fail in the current topology.
> setup() already uses the same features under set -e, so this is a
> robustness question more than a confirmed failure.
>
> [snip]
I will also have a look at this for v2.
Ciao,
Andrea
pw-bot: cr
