Re: [PATCH v1 bpf-next 8/8] selftest: bpf: Add test for BPF_SOCK_OPS_RCVLOWAT_CB.

On Fri, May 8, 2026 at 3:17 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Flaky test design due to race condition between send() and epoll_wait()/getsockopt().
> - [Medium] Pipelined data causes a TOCTOU race that permanently breaks and disables the BPF autolowat program.
> --
>
> commit 7be0b88b0617982ff63886ecdcf680fd3a6f0db8
> Author: Kuniyuki Iwashima <[email protected]>
>
> selftest: bpf: Add test for BPF_SOCK_OPS_RCVLOWAT_CB.
>
> This commit adds a selftest simulating a production scenario where a BPF
> program dynamically adjusts a socket's SO_RCVLOWAT based on the parsing
> of RPC descriptors. The test exchanges partial and full RPC frames over
> a local connection and verifies that epoll wakes up the application at
> the correct boundaries.
>
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c b/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c
> > --- a/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcp_autolowat.c
>
> [ ... ]
>
> > @@ -0,0 +1,350 @@
> > +static void tcp_autolowat_run_rpc_test(struct tcp_autolowat_test_cb *cb,
> > +                                    struct rpc_test_case *test_case)
> > +{
>
> [ ... ]
>
> > +             switch (event->type) {
> > +             case RPC_EVENT_END:
> > +                     return;
> > +             case RPC_EVENT_AUTOLOWAT:
> > +                     err = setsockopt(cb->child, SOL_BPF, BPF_TCP_AUTOLOWAT,
> > +                                      &event->val, sizeof(event->val));
> > +                     if (!ASSERT_OK(err, "setsockopt"))
> > +                             return;
> > +                     break;
> > +             case RPC_EVENT_SEND:
> > +                     err = send(cb->client, ptr, event->len, 0);
> > +                     if (!ASSERT_EQ(err, event->len, "send"))
> > +                             return;
> > +
> > +                     ptr += event->len;
> > +                     break;
>
> [ ... ]
>
> > +             case RPC_EVENT_EPOLL:
> > +                     err = epoll_wait(cb->epoll, &ev, 1, 0);
> > +                     if (!ASSERT_EQ(err, event->nfds, "epoll_wait"))
> > +                             return;
> > +                     break;
> > +             case RPC_EVENT_RCVLOWAT:
> > +                     optval = 0;
> > +                     optlen = sizeof(optval);
> > +
> > +                     err = getsockopt(cb->child, SOL_SOCKET, SO_RCVLOWAT, &optval, &optlen);
> > +                     if (!ASSERT_OK(err, "getsockopt") ||
> > +                         !ASSERT_EQ(optval, event->rcvlowat, "rcvlowat"))
> > +                             return;
> > +                     break;
> > +             }
>
> Are the epoll and getsockopt checks vulnerable to flaky failures
> under load?
>
> Since TCP loopback delivery can be deferred to softirq context, could
> epoll_wait with a 0-millisecond timeout return 0 if the packet hasn't
> been queued yet?

No, it's over "lo", and TX/RX is in the same context.

>
> Similarly, could getsockopt read a stale SO_RCVLOWAT value before the
> BPF program gets a chance to update it?
>
> Would using a polling loop or a positive epoll timeout prevent
> intermittent test failures?
>
> > diff --git a/tools/testing/selftests/bpf/progs/tcp_autolowat.c b/tools/testing/selftests/bpf/progs/tcp_autolowat.c
> > --- a/tools/testing/selftests/bpf/progs/tcp_autolowat.c
> > +++ b/tools/testing/selftests/bpf/progs/tcp_autolowat.c
>
> [ ... ]
>
> > @@ -0,0 +1,316 @@
> > +static int tcp_parse_descriptor(struct bpf_sock_ops *skops,
> > +                             struct tcp_autolowat_cb *cb,
> > +                             u32 seq, u32 end_seq)
> > +{
> > +     struct rpc_descriptor *rpc_desc;
> > +     u32 rpc_copied_seq;
> > +     u32 copy_len;
> > +     u64 rpc_len;
> > +     int err;
> > +
> > +     rpc_copied_seq = cb->rpc_desc_seq + cb->rpc_desc_buff_len;
> > +
> > +     if (before(cb->rpc_desc_seq + RPC_DESC_SIZE, end_seq))
> > +             copy_len = RPC_DESC_SIZE - cb->rpc_desc_buff_len;
> > +     else
> > +             copy_len = end_seq - rpc_copied_seq;
> > +
> > +     /* Since LLVM commit 324e27e8bad83ca23a3cd276d7e2e729b1b0b8c7,
> > +      * clang omits the "copy_len == 0" check below, which is necessary
> > +      * to satisfy the BPF verifier's range check for bpf_skb_load_bytes().
> > +      */
> > +     barrier_var(copy_len);
> > +
> > +     if (copy_len == 0)
> > +             goto disable; /* FIN. */
> > +     if (copy_len > RPC_DESC_SIZE)
> > +             goto disable; /* always false, only for verifier. */
> > +     if (cb->rpc_desc_buf + cb->rpc_desc_buff_len >= &cb->rpc_desc_buf[RPC_DESC_SIZE])
> > +             goto disable; /* always false, only for verifier. */
> > +
> > +     err = bpf_skb_load_bytes(skops, rpc_copied_seq - seq,
> > +                              cb->rpc_desc_buf + cb->rpc_desc_buff_len, copy_len);
> > +     if (err)
> > +             goto disable;
>
> Is there a race condition if pipelined data is already in the receive
> queue when BPF_TCP_AUTOLOWAT is enabled?
>
> Looking at tcp_init_autolowat_cb(), cb->rpc_desc_seq is initialized to
> tp->copied_seq. If there is already unread data in the queue, the next
> packet arrival triggers BPF_SOCK_OPS_RCVLOWAT_CB.
>
> Because seq (which comes from tcb->seq) corresponds to the new packet
> and is greater than tp->copied_seq, will rpc_copied_seq - seq underflow?
>
> If it underflows to a large positive value, won't bpf_skb_load_bytes()
> fail with -EFAULT, causing the code to jump to the disable label and
> permanently disable the autolowat functionality for this socket?

This assumes the client will stop at a certain point; it will not
send the next request speculatively before receiving the previous
response.

In such an irregular case, we want to fall back to no autolowat,
so this is fine.