Re: [PATCH bpf v2 0/2] bpf: fix wrong copied_seq calculation and add tests

[Jakub Sitnicki <jakub@cloudflare.com>]

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?