From: Yuya Kusakabe <[email protected]> Hi Andrea,
Thank you for the review. The points shared with patch 2 (NF_HOOK split removal, drop reasons via your prep series, reverse christmas tree, the missing frag_off check, BAD_INNER scoping, the repeated size-selection ternary, iptunnel_handle_offloads(), the fixed source port, and the RFC 6040 wording) will be addressed as described in my patch 2 reply and apply here the same way. Below are the End.M.GTP6.E-specific points. > SEG6_LOCAL_MOBILE_SRC_ADDR (the "src" attribute) is copied verbatim into > the outer IPv6 source address. In patch 2 (End.M.GTP4.E) the same > attribute is used as a template from which bits are extracted to form > the IPv4 source address, and may be entirely unused depending on > v4_mask_len. > This UAPI overload needs revision. Agreed. With v4_mask_len gone, End.M.GTP4.E will not take src at all (the IPv4 SA will be recovered purely from the inbound IPv6 SA, see the patch 2 reply), which removes the verbatim-vs-template overload. In the new SEG6_MOBILE_* namespace I plan to give SEG6_MOBILE_SRC_ADDR a single meaning for the IPv6-emitting behaviors (End.M.GTP6.E/D/D.Di): the outer IPv6 source address, used verbatim. The one remaining non-verbatim consumer would be H.M.GTP4.D, where the configured address acts as the RFC 9433 Figure 12 "Source UPF Prefix" template with exactly the 32 IPv4 SA bits overlaid at v6_src_prefix_len. H.M.GTP4.D posts last in the per-behavior order, so if you prefer the two semantics not to share one attribute name, I can give the template a distinctly named attribute in that series. > udp6_set_csum() already handles the CHECKSUM_PARTIAL + pseudo-header seed > setup and also covers the GSO case. Using it would avoid open-coding this > sequence. Will switch to udp6_set_csum(), thanks. It is also more correct than the open-coded sequence: for a non-GSO inner that arrives CHECKSUM_PARTIAL it resolves the inner checksum via local checksum offload instead of clobbering csum_start. > seg6_lookup_any_nexthop() already calls skb_dst_drop() internally. The > explicit call above is redundant. Will remove. > Nit: fc_dst_len is int in struct fib6_config (IPv6 prefix length, range > 0..128); the (unsigned int) cast is not needed. This check will move into the attribute parser of the new explicit locator-length attribute (see the patch 2 reply), so the fib6_config peek and the cast both go away. Thanks, Yuya
