linux-kernel - Re: [PATCH bpf v9 0/5] bpf: fix wrong copied

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <i2pmhcfge4my5rl4sy5uvu3lhnbtov5rhcjdrqbwunicnefrzy@uhs35blc47lv>
Date: Mon, 27 Jan 2025 08:04:29 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: Jiayuan Chen <mrpre@....com>, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, martin.lau@...ux.dev, ast@...nel.org, edumazet@...gle.com, 
	davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, 
	linux-kernel@...r.kernel.org, song@...nel.org, andrii@...nel.org, mhal@...x.co, 
	yonghong.song@...ux.dev, daniel@...earbox.net, xiyou.wangcong@...il.com, horms@...nel.org, 
	corbet@....net, eddyz87@...il.com, cong.wang@...edance.com, shuah@...nel.org, 
	mykolal@...com, jolsa@...nel.org, haoluo@...gle.com, sdf@...ichev.me, 
	kpsingh@...nel.org, linux-doc@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf v9 0/5] bpf: fix wrong copied_seq calculation and add
 tests

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@...il.com>

> 
> -jkbs