From: Mattia Meleleo tcp_bpf_ioctl() answers SIOCINQ from psock->msg_tot_len, which only counts bytes in ingress_msg. Without a stream/skb verdict program nothing is diverted there: data stays in sk_receive_queue, so FIONREAD returns 0 even though read() returns data. Add tcp_inq() to the reported value when the psock has no verdict program. The two queues are disjoint, so bytes redirected into ingress_msg from other sockets stay correctly accounted through msg_tot_len. Remove unused sk_psock_msg_inq(). Fixes: 929e30f93125 ("bpf, sockmap: Fix FIONREAD for sockmap") Reviewed-by: Jiayuan Chen Reviewed-by: Emil Tsalapatis Signed-off-by: Mattia Meleleo --- include/linux/skmsg.h | 14 -------------- net/ipv4/tcp_bpf.c | 17 ++++++++++++++++- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index a8553401b..d5e35f247 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h @@ -551,20 +551,6 @@ static inline void psock_progs_drop(struct sk_psock_progs *progs) psock_set_prog(&progs->skb_verdict, NULL); } -/* for tcp only, sk is locked */ -static inline ssize_t sk_psock_msg_inq(struct sock *sk) -{ - struct sk_psock *psock; - ssize_t inq = 0; - - psock = sk_psock_get(sk); - if (likely(psock)) { - inq = sk_psock_get_msg_len_nolock(psock); - sk_psock_put(sk, psock); - } - return inq; -} - /* for udp only, sk is not locked */ static inline ssize_t sk_msg_first_len(struct sock *sk) { diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index cc0bd73f3..8e905b50d 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -334,6 +334,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk, static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg) { + struct sk_psock *psock; bool slow; if (cmd != SIOCINQ) @@ -344,7 +345,21 @@ static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg) return -EINVAL; slow = lock_sock_fast(sk); - *karg = sk_psock_msg_inq(sk); + psock = sk_psock_get(sk); + if (unlikely(!psock)) { + unlock_sock_fast(sk, slow); + return tcp_ioctl(sk, cmd, karg); + } + *karg = sk_psock_get_msg_len_nolock(psock); + /* Without a verdict program, ingress data is never diverted to + * ingress_msg: it stays in sk_receive_queue and is read through + * the fallback to tcp_recvmsg(), so account for it like + * tcp_ioctl() does. + */ + if (!READ_ONCE(psock->progs.stream_verdict) && + !READ_ONCE(psock->progs.skb_verdict)) + *karg += tcp_inq(sk); + sk_psock_put(sk, psock); unlock_sock_fast(sk, slow); return 0; -- Git-155) From: Mattia Meleleo Add a test validating that FIONREAD on a TCP socket in a sockmap without a verdict program reports data pending in sk_receive_queue. Reviewed-by: Jiayuan Chen Reviewed-by: Emil Tsalapatis Signed-off-by: Mattia Meleleo --- .../selftests/bpf/prog_tests/sockmap_basic.c | 39 ++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c index cb3229711..f0f368201 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c @@ -1373,6 +1373,43 @@ static void test_sockmap_multi_channels(int sotype) test_sockmap_pass_prog__destroy(skel); } +/* A socket in a sockmap without a verdict program keeps its ingress data + * in sk_receive_queue: FIONREAD must account for it. + */ +static void test_sockmap_no_verdict_fionread(void) +{ + int err, map, zero = 0, sent, avail; + int c0 = -1, c1 = -1, p0 = -1, p1 = -1; + struct test_sockmap_pass_prog *skel; + char buf[256] = "0123456789"; + + skel = test_sockmap_pass_prog__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + return; + map = bpf_map__fd(skel->maps.sock_map_rx); + + err = create_socket_pairs(AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1); + if (!ASSERT_OK(err, "create_socket_pairs()")) + goto out; + + err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST); + if (!ASSERT_OK(err, "bpf_map_update_elem(c1)")) + goto out_close; + + sent = xsend(p1, &buf, sizeof(buf), 0); + ASSERT_EQ(sent, sizeof(buf), "xsend(p1)"); + avail = wait_for_fionread(c1, sizeof(buf), IO_TIMEOUT_SEC); + ASSERT_EQ(avail, sizeof(buf), "ioctl(FIONREAD)"); + +out_close: + close(c0); + close(p0); + close(c1); + close(p1); +out: + test_sockmap_pass_prog__destroy(skel); +} + void test_sockmap_basic(void) { if (test__start_subtest("sockmap create_update_free")) @@ -1415,6 +1452,8 @@ void test_sockmap_basic(void) test_sockmap_skb_verdict_shutdown(); if (test__start_subtest("sockmap skb_verdict fionread")) test_sockmap_skb_verdict_fionread(true); + if (test__start_subtest("sockmap no_verdict fionread")) + test_sockmap_no_verdict_fionread(); if (test__start_subtest("sockmap skb_verdict fionread on drop")) test_sockmap_skb_verdict_fionread(false); if (test__start_subtest("sockmap skb_verdict change tail")) -- Git-155)