netdev - RE: [PATCH bpf-next 2/3] tcp: Add the data length in skmsg to SIOCINQ ioctl

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: <6554713028d5b_3733620856@john.notmuch>
Date: Tue, 14 Nov 2023 23:20:16 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Pengcheng Yang <yangpc@...gsu.com>, 
 John Fastabend <john.fastabend@...il.com>, 
 Jakub Sitnicki <jakub@...udflare.com>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Cc: Pengcheng Yang <yangpc@...gsu.com>
Subject: RE: [PATCH bpf-next 2/3] tcp: Add the data length in skmsg to SIOCINQ
 ioctl

Pengcheng Yang wrote:
> SIOCINQ ioctl returns the number unread bytes of the receive
> queue but does not include the ingress_msg queue. With the
> sk_msg redirect, an application may get a value 0 if it calls
> SIOCINQ ioctl before recv() to determine the readable size.
> 
> Signed-off-by: Pengcheng Yang <yangpc@...gsu.com>
> ---
>  net/ipv4/tcp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 3d3a24f79573..04da0684c397 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -267,6 +267,7 @@
>  #include <linux/errqueue.h>
>  #include <linux/static_key.h>
>  #include <linux/btf.h>
> +#include <linux/skmsg.h>
>  
>  #include <net/icmp.h>
>  #include <net/inet_common.h>
> @@ -613,7 +614,7 @@ int tcp_ioctl(struct sock *sk, int cmd, int *karg)
>  			return -EINVAL;
>  
>  		slow = lock_sock_fast(sk);
> -		answ = tcp_inq(sk);
> +		answ = tcp_inq(sk) + sk_msg_queue_len(sk);

This will break the SK_PASS case I believe. Here we do
not update copied_seq until data is actually copied into user
space. This also ensures tcp_epollin_ready works correctly and
tcp_inq. The fix is relatively recent.

 commit e5c6de5fa025882babf89cecbed80acf49b987fa
 Author: John Fastabend <john.fastabend@...il.com>
 Date:   Mon May 22 19:56:12 2023 -0700

    bpf, sockmap: Incorrectly handling copied_seq

The previous patch increments the msg_len for all cases even
the SK_PASS case so you will get double counting.

I was starting to poke around at how to fix the other cases e.g.
stream parser is in use and redirects but haven't got to it  yet.
By the way I think even with this patch epollin_ready is likely
not correct still. We observe this as either failing to wake up
or waking up an application to early when using stream parser.

The other thing to consider is redirected skb into another socket
and then read off the list increment the copied_seq even though
they shouldn't if they came from another sock?  The result would
be tcp_inq would be incorrect even negative perhaps?

What does your test setup look like? Simple redirect between
two TCP sockets? With or without stream parser? My guess is we
need to fix underlying copied_seq issues related to the redirect
and stream parser case. I believe the fix is, only increment
copied_seq for data that was put on the ingress_queue from SK_PASS.
Then update previous patch to only incrmeent sk_msg_queue_len()
for redirect paths. And this patch plus fix to tcp_epollin_ready
would resolve most the issues. Its a bit unfortunate to leak the
sk_sg_queue_len() into tcp_ioctl and tcp_epollin but I don't have
a cleaner idea right now.

>  		unlock_sock_fast(sk, slow);
>  		break;
>  	case SIOCATMARK:
> -- 
> 2.38.1
>