On Tue, May 19, 2026 at 10:32 AM Andrea Mayer <[email protected]> wrote:
> The commit message lists three drop reasons including > SEG6_MOBILE_HOP_LIMIT_EXCEEDED, but the code does not add that one. > The likely third reason (SEG6_MOBILE_MTU_EXCEEDED) appears in > patch 2. (Flagged by Sashiko, the Patchwork AI reviewer.) You and Sashiko are right. The new submission will only list the drop reasons that End.MAP actually adds. > Nit: As far as I can see, Link: tags in this tree usually point to mailing > list messages (patch.msgid.link, lore.kernel.org). Other commits that > reference RFCs typically cite them in the commit body instead. Same for > patches 2-7. Will move the RFC references into the commit body and drop the Link: tags in the upcoming submissions. > This single reason covers several distinct failure modes across the > patchset: wrong Segments Left, SRH absent, SRH structurally malformed, and > HMAC validation failure. An operator seeing this drop cannot tell which > check failed. Using separate SRv6-level drop reasons seems reasonable. See > my cover letter reply for the broader discussion on drop reasons. Agreed. As discussed on the cover letter thread, the SRv6-level drop reasons (SEG6_INVALID_SRH, SEG6_HMAC, ...) would be introduced in a prep patchset that also converts the existing seg6_local and seg6_iptunnel call sites. Pending the resolution on the cover letter thread of who prepares the prep series, the End.MAP repost will depend on it and consume the new reasons. > This overlaps with the existing generic SKB_DROP_REASON_NOMEM in > dropreason-core.h. Why not use the generic one? I'll drop SEG6_MOBILE_NOMEM and use the generic SKB_DROP_REASON_NOMEM. > seg6_get_srh() returns NULL both when the SRH is absent and when it is > malformed (seg6_validate_srh fails, e.g. type != 4) or truncated. > seg6_mobile_get_validated_srh() sets *missing = true in all these cases, > so input_action_end_map() treats a malformed SRH the same as an absent > one and continues processing. HMAC validation is also bypassed because > seg6_get_srh() returns NULL before HMAC is reached. > > seg6_mobile_get_validated_srh() needs to distinguish "absent" from > "malformed/truncated" so callers can drop on malformed instead of silently > accepting the packet. The natural fix is to change seg6_get_srh() to expose the reason (absent / malformed / HMAC failure) to the caller, which fits into the drop-reason prep series and lets the dedicated helper go away. API shape deferred to whoever prepares the prep series. > Nit: the function comment says "decrement the Hop Limit" but the code does not > do it explicitly. The forwarding path handles it (ip6_forward). Maybe > remove that part from the comment or add a note that the forwarding path > handles it? Will reword the comment to make it clear that Hop Limit handling is left to the forwarding path. > Because of the bug described above, the only path that reaches the > drop label with this reason is HMAC validation failure (when HMAC is > enabled). Same drop reason granularity point as above. Right, that's a symptom of the helper bug; both will be fixed together by the seg6_get_srh() refactor in the prep series. > See above: this only catches HMAC failure. A malformed SRH falls through as > if the SRH were absent. This call site folds away with the helper rework above. > Sashiko flagged that for SRH-less packets this breaks the ICMPv6 checksum, > because the pseudo-header includes the DA. The AI bot was right, but when I > ran the selftest it passed. Digging a bit further, I noticed why: > 2001:db8:f::1 and 2001:db8:2::e have the same 16-bit word sum > (0x000f+0x0001 = 0x0002+0x000e), so the checksum stays valid by > coincidence. Changing nh6 to 2001:db8:2::2 makes the ping fail with > Icmp6InCsumErrors. Thanks for reproducing this. I'll fix the L4 checksum on DA rewrite with an incremental update via inet_proto_csum_replace16(). > seg6_lookup_nexthop() calls seg6_lookup_any_nexthop() which already calls > skb_dst_drop() internally. The explicit skb_dst_drop(skb) above is > redundant. Will drop the redundant call. > End.MAP reuses SEG6_LOCAL_NH6 to mean "replacement SID", not "next-hop" > as in End.X/End.DX6. This overloads the existing UAPI semantics of > the attribute. The cover letter reply discusses this attribute-semantics > question across the patchset. As discussed on the cover letter thread, End.MAP will move into the new seg6_mobile module under LWTUNNEL_ENCAP_SEG6_MOBILE and use a new SEG6_MOBILE_* attribute (SEG6_MOBILE_NH6 or similar) for the replacement SID, so SEG6_LOCAL_NH6 keeps its existing next-hop semantics. No SEG6_LOCAL_* attribute will be reused. > The selftest only covers the SRH-less path. End.MAP also accepts packets > with an SRH, and that case should be covered as well. > > To expose the ICMPv6 checksum issue noted above, the address pair should be > chosen so that the daddr rewrite changes the checksum. Will cover SRH and no-SRH paths per the cover letter discussion, and pick addresses whose 16-bit word sums differ so the ICMPv6 checksum path is actually exercised. The next round will be a fresh standalone "[PATCH] seg6_mobile: add End.MAP behavior" series in the new seg6_mobile module, posted after (or as a dependency on) the seg6 drop-reason prep series, per the cover letter discussion. Thanks again for the careful review. Yuya
