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

From: John Fastabend <[email-masked]>
Date: Mon, 03 Apr 2023 14:05:24 -0700

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 <[email-masked]>
> > Signed-off-by: John Fastabend <[email-masked]>
> > ---
> >  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