Re: [PATCH bpf v4 1/2] bpf: tcp: Reject non-TCP skb in bpf_sk_assign_tcp_reqsk()

[Kuniyuki Iwashima]

On Mon, Mar 30, 2026 at 8:19 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 3/30/26 6:35 PM, Kuniyuki Iwashima wrote:
> > On Mon, Mar 30, 2026 at 2:26 AM Jiayuan Chen <jiayuan.chen@xxxxxxxxx> wrote:
> >>
> >>
> >> On 3/30/26 5:00 PM, bot+bpf-ci@xxxxxxxxxx wrote:
> >>>> #if IS_BUILTIN(CONFIG_IPV6)
> >>>>       case htons(ETH_P_IPV6):
> >>>> +            if (ipv6_hdr(skb)->nexthdr != IPPROTO_TCP)
> >>>> +                    return -EINVAL;
> >>>> +
> >>>>               ops = &tcp6_request_sock_ops;
> >>> For IPv6, ipv6_hdr(skb)->nexthdr gives the type of the header
> >>> immediately following the base IPv6 header. If extension headers
> >>> are present (hop-by-hop options, routing, etc.), nexthdr would be
> >>> the extension header type rather than IPPROTO_TCP, even when the
> >>> packet is a valid TCP segment.
> >>>
> >>> Would it be worth using ipv6_find_hdr() here, similar to
> >>> bpf_update_srh_state() in the same file, to walk past any extension
> >>> headers? The IPv4 check above is fine since ip_hdr(skb)->protocol
> >>> always identifies the transport protocol directly.
> >>>
> >>> In practice this is very unlikely to matter since TCP SYN packets
> >>> with IPv6 extension headers are essentially non-existent, but the
> >>> check as written would reject them.
> >>>
> >>>
> >>> ---
> >>> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> >>> See:https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> >>>
> >>> CI run summary:https://github.com/kernel-patches/bpf/actions/runs/23735111188
> >>
> >>
> >> Thanks for the analysis.
> >>
> >> There are many places in the kernel that check nexthdr directly without
> >> walking extension headers.
> >>
> >> I'd prefer to keep it simple for now and leave ipv6_find_hdr() as a
> >> potential future improvement if needed.
> >
> > +1.
> >
> > Given this feature is to create a reqsk to process on this running
> > host, it does not make sense to support such ext options.
>
> While at header reading, does it need a pskb_may_pull() before reading?

Ah good point, now that we read skb->data, pskb_may_pull() is needed indeed.