All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Kuniyuki Iwashima" <kuniyu@google.com>
Cc: bpf@vger.kernel.org
Subject: Re: [PATCH v1 bpf-next 2/8] bpf: tcp: Introduce BPF_SOCK_OPS_RCVLOWAT_CB.
Date: Fri, 08 May 2026 19:17:36 +0000	[thread overview]
Message-ID: <20260508191737.01190C2BCB0@smtp.kernel.org> (raw)
In-Reply-To: <20260508073355.3916746-3-kuniyu@google.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The deliberate exclusion of BPF_SOCK_OPS_RCVLOWAT_CB from is_locked_tcp_sock_ops() disables critical socket helpers, notably bpf_sock_ops_cb_flags_set().
--

commit 2ab2e31caf792b1e7312616215a88ad7ae1ef4e1
Author: Kuniyuki Iwashima <kuniyu@google.com>

bpf: tcp: Introduce BPF_SOCK_OPS_RCVLOWAT_CB.

This commit introduces a new hook, BPF_SOCK_OPS_RCVLOWAT_CB, which allows
BPF programs to dynamically adjust sk->sk_rcvlowat upon recvmsg completion
or when packets are enqueued. The commit leaves is_locked_tcp_sock_ops
unchanged to prevent unnecessary enablement of bpf_setsockopt.

>     This will allow the BPF prog to dynamically adjust sk->sk_rcvlowat,
>     suppressing unnecessary EPOLLIN wakeups until sufficient data
>     (e.g., a full RPC frame) is available in the receive queue.
>     
>     Note that is_locked_tcp_sock_ops() is left unchanged not to enable
>     bpf_setsockopt() unnecessarily.

Does leaving is_locked_tcp_sock_ops() unchanged also prevent the use of
bpf_sock_ops_cb_flags_set() from within the BPF_SOCK_OPS_RCVLOWAT_CB hook?

Because BPF_SOCK_OPS_RCVLOWAT_CB is appended to the enum after
BPF_SOCK_OPS_WRITE_HDR_OPT_CB, is_locked_tcp_sock_ops() returns false
for this new hook.

Both bpf_sock_ops_cb_flags_set() and bpf_setsockopt() require
is_locked_tcp_sock_ops() to return true. If it returns false, they fail
with -EOPNOTSUPP.

Since direct modification of bpf_sock_ops_cb_flags is disallowed by the
verifier, wouldn't a BPF program running in this context be entirely unable
to clear the BPF_SOCK_OPS_RCVLOWAT_CB_FLAG once it finishes parsing?

If the flag cannot be cleared, does this permanently trap the socket in the
fast-path hook for every enqueued packet and recvmsg() call, leading to
performance degradation?

While the potential UAF concern regarding bpf_skb_load_bytes() is a false
positive addressed later in the series by "bpf: tcp: Support
bpf_skb_load_bytes() for BPF_SOCK_OPS_RCVLOWAT_CB", and the hook omission
is fixed by "bpf: tcp: Add SOCK_OPS rcvlowat hook", this limitation
regarding is_locked_tcp_sock_ops() does not appear to be addressed later
in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508073355.3916746-1-kuniyu@google.com?part=2

  reply	other threads:[~2026-05-08 19:17 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-08  7:33 [PATCH v1 bpf-next 0/8] bpf: Add SOCK_OPS hooks for TCP AutoLOWAT Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 1/8] selftest: bpf: Use BPF_SOCK_OPS_ALL_CB_FLAGS + 1 for bad_cb_test_rv Kuniyuki Iwashima
2026-05-08 19:02   ` sashiko-bot
2026-05-08 20:21     ` Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 2/8] bpf: tcp: Introduce BPF_SOCK_OPS_RCVLOWAT_CB Kuniyuki Iwashima
2026-05-08 19:17   ` sashiko-bot [this message]
2026-05-08 20:26     ` Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 3/8] bpf: tcp: Support bpf_skb_load_bytes() for BPF_SOCK_OPS_RCVLOWAT_CB Kuniyuki Iwashima
2026-05-08 15:15   ` Stanislav Fomichev
2026-05-08 19:45     ` Kuniyuki Iwashima
2026-05-11 14:56       ` Stanislav Fomichev
2026-05-08  7:33 ` [PATCH v1 bpf-next 4/8] tcp: Split out __tcp_set_rcvlowat() Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 5/8] bpf: tcp: Add kfunc to adjust sk->sk_rcvlowat Kuniyuki Iwashima
2026-05-11 12:34   ` Björn Töpel
2026-05-17 23:28     ` Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 6/8] bpf: tcp: Factorise bpf_skops_established() Kuniyuki Iwashima
2026-05-08  7:33 ` [PATCH v1 bpf-next 7/8] bpf: tcp: Add SOCK_OPS rcvlowat hook Kuniyuki Iwashima
2026-05-08 10:37   ` Jiayuan Chen
2026-05-08 11:30     ` Kuniyuki Iwashima
2026-05-08 12:19       ` Jiayuan Chen
2026-05-08 15:28   ` Stanislav Fomichev
2026-05-08 20:05     ` Kuniyuki Iwashima
2026-05-11 14:55       ` Stanislav Fomichev
2026-05-08 21:46   ` sashiko-bot
2026-05-08  7:33 ` [PATCH v1 bpf-next 8/8] selftest: bpf: Add test for BPF_SOCK_OPS_RCVLOWAT_CB Kuniyuki Iwashima
2026-05-08 15:35   ` Stanislav Fomichev
2026-05-08 20:19     ` Kuniyuki Iwashima
2026-05-08 21:47       ` Stanislav Fomichev
2026-05-08 21:58         ` Kuniyuki Iwashima
2026-05-08 22:17   ` sashiko-bot
2026-05-08 22:47     ` Kuniyuki Iwashima

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260508191737.01190C2BCB0@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=kuniyu@google.com \
    --cc=sashiko@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.