Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0 (2) 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). - [High] The fix is incomplete for non-first fragments. Pre-existing issues: - [Medium] This problem wasn't introduced by this patch, but OFPIEH12_ESP is never set. - [Low] This problem wasn't introduced by this patch, but a single Destination Options header in the final-destination position is… - [Low] This problem wasn't introduced by this patch, but repeated Hop- by-Hop headers are not reported as OFPIEH12_UNREP when the first one… -- openvswitch: advance by the real header length in get_ipv6_ext_hdrs() get_ipv6_ext_hdrs() used to move past every extension header with ipv6_optlen(). That length is wrong for AH and Fragment headers. This patch advances by sizeof(struct frag_hdr) for Fragment headers and by ipv6_authlen() for AH, so the OFPIEH12_* bits come from the headers that are actually present. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c > index 1c4f3a0790443..a1c60d2f552cf 100644 > --- a/net/openvswitch/flow.c > +++ b/net/openvswitch/flow.c > @@ -372,8 +372,13 @@ static void get_ipv6_ext_hdrs(struct sk_buff *skb, > struct ipv6hdr *nh, > hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr); > if (!hp) > break; > + if (next_type == IPPROTO_FRAGMENT) > + start += sizeof(struct frag_hdr); > + else if (next_type == IPPROTO_AH) > + start += ipv6_authlen(hp); > + else > + start += ipv6_optlen(hp); > + next_type = hp->nexthdr; [Severity: High] Can payload bytes still set the exthdrs bits when the packet is a non-first fragment? The IPPROTO_FRAGMENT case in get_ipv6_ext_hdrs() sets OFPIEH12_FRAG and never reads frag_off. This branch then always advances 8 bytes and follows hp->nexthdr. RFC 8200 says every fragment carries the Next Header value of the first header in the Fragmentable Part. Take IPsec AH transport mode with fragmentation as an example. A later fragment shows up as FRAG(offset != 0, nexthdr = AH). The bytes after that Fragment header are payload from the middle of the packet. The next loop iteration still parses them as an ipv6_opt_hdr: payload[0] is taken as the next type and payload[1] as the length. Payload bytes such as 0, 43, 44, 51 or 59 would then set HOP/UNSEQ, ROUTER, FRAG/UNREP, AUTH/UNREP or NONEXT for headers that are not there. The commit message says the walk now advances "as ipv6_find_hdr() does". However, ipv6_find_hdr() stops at non-first fragments: net/ipv6/exthdrs_core.c:ipv6_find_hdr() { ... _frag_off = ntohs(*fp) & ~0x7; if (_frag_off) { if (target < 0 && ((!ipv6_ext_hdr(hp->nexthdr)) || hp->nexthdr == NEXTHDR_NONE)) { if (fragoff) *fragoff = _frag_off; return hp->nexthdr; } if (!found) return -ENOENT; ... } The wrong bits also seem to reach the flow key. parse_ipv6hdr() calls get_ipv6_ext_hdrs() before ipv6_find_hdr(). In the FRAG(offset != 0, nexthdr = extension header) case, ipv6_find_hdr() returns -ENOENT and leaves frag_off at 0. parse_ipv6hdr() therefore sets OVS_FRAG_TYPE_FIRST and returns -EPROTO. key_extract_l3l4() then treats that as success: net/openvswitch/flow.c:key_extract_l3l4() { ... case -EPROTO: skb->transport_header = skb->network_header; error = 0; break; ... } key->ipv6.exthdrs is never cleared on this path. Flows that match on OVS_KEY_ATTR_IPV6_EXTHDRS would still misclassify these packets. The tests in the changelog only cover Fragment headers with offset 0. Would it make sense for the IPPROTO_FRAGMENT case to read frag_off with skb_header_pointer() and stop the walk when ntohs(frag_off) & ~0x7 is non-zero? > } > } > [Severity: Medium] This is a pre-existing issue and the patch doesn't introduce it, but is OFPIEH12_ESP ever set? The loop in get_ipv6_ext_hdrs() is guarded by: while (ipv6_ext_hdr(next_type)) { ipv6_ext_hdr() returns true only for HOP, ROUTING, FRAGMENT, AUTH, NONE and DEST: net/ipv6/exthdrs_core.c:ipv6_ext_hdr() { return (nexthdr == NEXTHDR_HOP) || (nexthdr == NEXTHDR_ROUTING) || (nexthdr == NEXTHDR_FRAGMENT) || (nexthdr == NEXTHDR_AUTH) || (nexthdr == NEXTHDR_NONE) || (nexthdr == NEXTHDR_DEST); } NEXTHDR_ESP is not in that list, so the loop exits before case IPPROTO_ESP can run. flow.h documents OFPIEH12_ESP as "Encrypted Sec Payload header present", and the bit is exported through OVS_KEY_ATTR_IPV6_EXTHDRS. A flow matching ESP=1 can never match, and a flow matching ESP=0 matches every ESP packet. The OFPIEH12_ESP check in the second DSTOPTS mask is dead code for the same reason. If the loop guard is ever changed to include ESP, the new length dispatch in this patch would send ESP to ipv6_optlen(hp). That reads SPI bytes as a length, so ESP would need an explicit stop there. This patch does not change ESP behaviour. Before the patch, an AH followed by ESP also reached next_type == ESP, because next_type comes from the AH header's own nexthdr field. [Severity: Low] This isn't a bug introduced by this patch, but should a single Destination Options header in the final-destination position really get OFPIEH12_UNSEQ? The first DSTOPTS seen in get_ipv6_ext_hdrs() is only accepted if it comes after HOP: case IPPROTO_DSTOPTS: if (dest_options_header_count == 0) { if (*ext_hdrs & ~(OFPIEH12_HOP | OFPIEH12_UNREP)) *ext_hdrs |= OFPIEH12_UNSEQ; Take IPv6 -> Routing -> DSTOPTS -> TCP, or AH -> DSTOPTS. ROUTER or AUTH is already set by then, so UNSEQ is set. The changelog's own test result shows this: "AUTH DEST UNSEQ". The comment above the function lists the preferred order as Hop, Dest, Routing, Fragment, AH, ESP, Dest, upper-layer. Routing -> Dest and AH -> Dest both follow that order. The count of DSTOPTS headers seen so far is being used as if it were the header's position. [Severity: Low] This is a pre-existing issue, but are repeated Hop-by-Hop headers missed when the first one is out of sequence? In get_ipv6_ext_hdrs(): case IPPROTO_HOPOPTS: if (*ext_hdrs & OFPIEH12_HOP) *ext_hdrs |= OFPIEH12_UNREP; ... if (*ext_hdrs == 0) *ext_hdrs |= OFPIEH12_HOP; else *ext_hdrs |= OFPIEH12_UNSEQ; OFPIEH12_HOP is only set when HOP is the first header. Take IPv6 -> Routing -> HOP -> HOP -> TCP. The first HOP sees ROUTER already set, so it sets UNSEQ but not HOP. The second HOP finds HOP clear and also sets only UNSEQ. UNREP stays clear even though two HOP headers are present. ipv6_find_hdr() checks neither order nor repetition, so a short chain like this passes and the missing bit stays in the flow key. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179328922134.76862.10417963652853801970%40gmail.com _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
