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: <642b3f94f13df_e67b72086@john.notmuch>
Date:   Mon, 03 Apr 2023 14:05:24 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>,
        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

Jakub Sitnicki wrote:
> 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?

Yes, I had lumped it in with follow up fixes needed for the
stream parser case but your right its not related.

Mind if I do it in a follow up? Or if I need to do a v4 I'll
roll it in there.

Thanks!
John