linux-kernel - Re: [PATCH bpf v3 1/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: <87h66ujex9.fsf@cloudflare.com>
Date: Mon, 23 Dec 2024 21:57:22 +0100
From: Jakub Sitnicki <jakub@...udflare.com>
To: Jiayuan Chen <mrpre@....com>, John Fastabend <john.fastabend@...il.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,  andrii@...nel.org,  mhal@...x.co,
  yonghong.song@...ux.dev,  daniel@...earbox.net,
  xiyou.wangcong@...il.com,  horms@...nel.org
Subject: Re: [PATCH bpf v3 1/2] bpf: fix wrong copied_seq calculation

On Thu, Dec 19, 2024 at 05:30 PM +08, Jiayuan Chen wrote:
> Currently, not all modules using strparser have issues with
> copied_seq miscalculation. The issue exists mainly with
> bpf::sockmap + strparser because bpf::sockmap implements a
> proprietary read interface for user-land: tcp_bpf_recvmsg_parser().
>
> Both this and strp_recv->tcp_read_sock update copied_seq, leading
> to errors.
>
> This is why I rewrote the tcp_read_sock() interface specifically for
> bpf::sockmap.

All right. Looks like reusing read_skb is not going to pan out.

But I think we should not give up just yet. It's easy to add new code.

We can try to break up and parametrize tcp_read_sock - if other
maintainers are not against it. Does something like this work for you?

  https://github.com/jsitnicki/linux/commits/review/stp-copied_seq/idea-2/

Other minor feedback I have:

- The newly added code is optional and should depend on
  CONFIG_BPF_STREAM_PARSER being enabled. Please check that it builds
  with CONFIG_BPF_STREAM_PARSER=n as well.

- Let's not add complexity until it's really needed, and today we don't
  need seprate tcp_bpf_proto_ops for IPv4 and IPv6.

- There are style issues with the added test. Please run checkpatch.pl.