Re: [PATCH bpf v9 0/5] bpf: fix wrong copied_seq calculation and add tests

John Fastabend
2025-01-27T16:04:45Z

On 2025-01-26 15:16:47, Jakub Sitnicki wrote:
> On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote:
> > A previous commit described in this topic
> > http://lore.kernel.org/bpf/20230523025618.113937-9-john.fastabend@gmail.com
> > directly updated 'sk->copied_seq' in the tcp_eat_skb() function when the
> > action of a BPF program was SK_REDIRECT. For other actions, like SK_PASS,
> > the update logic for 'sk->copied_seq' was moved to
> > tcp_bpf_recvmsg_parser() to ensure the accuracy of the 'fionread' feature.
> >
> > That commit works for a single stream_verdict scenario, as it also
> > modified 'sk_data_ready->sk_psock_verdict_data_ready->tcp_read_skb'
> > to remove updating 'sk->copied_seq'.
> >
> > However, for programs where both stream_parser and stream_verdict are
> > active (strparser purpose), tcp_read_sock() was used instead of
> > tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock).
> > tcp_read_sock() now still updates 'sk->copied_seq', leading to duplicated
> > updates.
> >
> > In summary, for strparser + SK_PASS, copied_seq is redundantly calculated
> > in both tcp_read_sock() and tcp_bpf_recvmsg_parser().
> >
> > The issue causes incorrect copied_seq calculations, which prevent
> > correct data reads from the recv() interface in user-land.
> >
> > Also we added test cases for bpf + strparser and separated them from
> > sockmap_basic, as strparser has more encapsulation and parsing
> > capabilities compared to sockmap.
> >
> > ---
> > V8 -> v9
> > https://lore.kernel.org/bpf/20250121050707.55523-1-mrpre@163.com/
> > Fixed some issues suggested by Jakub Sitnicki.
> >
> > V7 -> V8
> > https://lore.kernel.org/bpf/20250116140531.108636-1-mrpre@163.com/
> > Avoid using add read_sock to psock. (Jakub Sitnicki)
> > Avoid using warpper function to check whether strparser is supported.
> >
> > V3 -> V7:
> > https://lore.kernel.org/bpf/20250109094402.50838-1-mrpre@163.com/
> > https://lore.kernel.org/bpf/20241218053408.437295-1-mrpre@163.com/
> > Avoid introducing new proto_ops. (Jakub Sitnicki).
> > Add more edge test cases for strparser + bpf.
> > Fix patchwork fail of test cases code.
> > Fix psock fetch without rcu lock.
> > Move code of modifying to tcp_bpf.c.
> >
> > V1 -> V3:
> > https://lore.kernel.org/bpf/20241209152740.281125-1-mrpre@163.com/
> > Fix patchwork fail by adding Fixes tag.
> > Save skb data offset for ENOMEM. (John Fastabend)
> > ---
>
> Thanks for addressing all feedback, Jiayuan. Series LGTM.
> Feel free to carry my tags if there is another iteration.

+1 Thanks Jiayuan for sticking with this.

I've reviewed this a couple times. I had one nit on the if/else branch
for a read call, but I haven't come up with anything better on my end
and this fixes a real bug. So lets take it.

For the series.

Acked-by: John Fastabend <john.fastabend@gmail.com>

>
> -jkbs