linux-kernel - Re: [PATCH bpf v2 0/2] 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: <87o71bx1l4.fsf@cloudflare.com>
Date: Mon, 16 Dec 2024 13:19:03 +0100
From: Jakub Sitnicki <jakub@...udflare.com>
To: Cong Wang <xiyou.wangcong@...il.com>, Jiayuan Chen <mrpre@....com>
Cc: bpf@...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,  john.fastabend@...il.com,  andrii@...nel.org,
  mhal@...x.co,  yonghong.song@...ux.dev,  daniel@...earbox.net,
  horms@...nel.org
Subject: Re: [PATCH bpf v2 0/2] bpf: fix wrong copied_seq calculation and
 add tests

On Sat, Dec 14, 2024 at 05:18 PM -08, Cong Wang wrote:
> On Sat, Dec 14, 2024 at 07:50:37PM +0100, Jakub Sitnicki wrote:
>> On Mon, Dec 09, 2024 at 11:27 PM +08, Jiayuan Chen wrote:
>> 
>> [...]
>> 
>> > We added test cases for bpf + strparser and separated them from
>> > sockmap_basic. This is because we need to add more test cases for
>> > strparser in the future.
>> >
>> > Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
>> >
>> > ---
>> 
>> I have a question unrelated to the fix itself -
>> 
>> Are you an active strparser+verdict sockmap user?
>> 
>> I was wondering if we can deprecate strparser if/when KCM time comes
>
> I am afraid not.
>
> strparser is very different from skb verdict, upper layer (e.g. HTTP)
> protocol messages may be splitted accross sendmsg() call's, strparser
> is the only place where we can assemble the messages and parse them as a
> whole.
>
> And I don't think we have to use KCM together with strparser. Therefore,
> even _if_ KCM can be deprecated, strparse still can't.

Thanks for the context. Good to know we have strparser users.

I also wanna ask - did you guys consider migrating
strp_data_ready->strp_read_sock->...->strp_recv to read_skb /
tcp_read_skb to prevent the duplicate copied_seq update?

tcp_bpf_read_sock looks awfully lot like tcp_read_skb.

I realize it is easier said than done because there is an interface
mismatch - desc.count used to stop reading, and desc.error to signal OOM
/ need to requeue is missing. And then there is the SW kTLS read_sock
callback that would need adapting as well.

Definitely more work, but maybe less code duplication in the long run?