netdev - Re: [PATCH bpf v2 04/12] bpf: sockmap, handle fin correctly

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: <87a5zpdxu7.fsf@cloudflare.com>
Date:   Mon, 03 Apr 2023 13:11:07 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     cong.wang@...edance.com, daniel@...earbox.net, lmb@...valent.com,
        edumazet@...gle.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
        ast@...nel.org, andrii@...nel.org, will@...valent.com
Subject: Re: [PATCH bpf v2 04/12] bpf: sockmap, handle fin correctly

On Mon, Mar 27, 2023 at 10:54 AM -07, John Fastabend wrote:
> The sockmap code is returning EAGAIN after a FIN packet is received and no
> more data is on the receive queue. Correct behavior is to return 0 to the
> user and the user can then close the socket. The EAGAIN causes many apps
> to retry which masks the problem. Eventually the socket is evicted from
> the sockmap because its released from sockmap sock free handling. The
> issue creates a delay and can cause some errors on application side.
>
> To fix this check on sk_msg_recvmsg side if length is zero and FIN flag
> is set then set return to zero. A selftest will be added to check this
> condition.
>
> Fixes: 04919bed948dc ("tcp: Introduce tcp_read_skb()")
> Tested-by: William Findlay <will@...valent.com>
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>  net/ipv4/tcp_bpf.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cf26d65ca389..3a0f43f3afd8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c

[...]

> @@ -193,6 +211,19 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	lock_sock(sk);
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
> +	/* The typical case for EFAULT is the socket was gracefully
> +	 * shutdown with a FIN pkt. So check here the other case is
> +	 * some error on copy_page_to_iter which would be unexpected.
> +	 * On fin return correct return code to zero.
> +	 */
> +	if (copied == -EFAULT) {
> +		bool is_fin = is_next_msg_fin(psock);
> +
> +		if (is_fin) {
> +			copied = 0;
> +			goto out;
> +		}
> +	}
>  	if (!copied) {
>  		long timeo;
>  		int data;

tcp_bpf_recvmsg needs a similar fix, no?