Re: [PATCH net] tcp: handle inet_csk_reqsk_queue_add() failures

[Guillaume Nault <gnault@redhat.com>]

On Fri, Mar 08, 2019 at 01:33:02PM -0800, Eric Dumazet wrote:
> 
> 
> On 03/08/2019 01:09 PM, Guillaume Nault wrote:
> > @@ -216,7 +216,12 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
> >  		refcount_set(&req->rsk_refcnt, 1);
> >  		tcp_sk(child)->tsoffset = tsoff;
> >  		sock_rps_save_rxhash(child, skb);
> > -		inet_csk_reqsk_queue_add(sk, req, child);
> > +		if (!inet_csk_reqsk_queue_add(sk, req, child)) {
> > +			bh_unlock_sock(child);
> > +			sock_put(child);
> > +			child = NULL;
> > +			reqsk_put(req);
> 
> Since we use reqsk_free(req) in the same function, we can use reqsk_free(req)
> here as well ?
> 
That was my first approach, but reqsk_free() doesn't like it:

static inline void reqsk_free(struct request_sock *req)
{
        /* temporary debugging */
	WARN_ON_ONCE(refcount_read(&req->rsk_refcnt) != 0);
...
}

> I suggest the following maybe :
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 606f868d9f3fde1c3140aa7eecde87d2ec32b5f2..8b28fb66a8fcefba27a2f5e371e9469d4d7e3650 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -216,11 +216,14 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
>                 refcount_set(&req->rsk_refcnt, 1);
>                 tcp_sk(child)->tsoffset = tsoff;
>                 sock_rps_save_rxhash(child, skb);
> -               inet_csk_reqsk_queue_add(sk, req, child);
> -       } else {
> -               reqsk_free(req);
> +               if (likely(inet_csk_reqsk_queue_add(sk, req, child)))
> +                       return child;
> +               bh_unlock_sock(child);
> +               sock_put(child);
>         }
> -       return child;
> +
> +       reqsk_free(req);
> +       return NULL;
>  }
>  EXPORT_SYMBOL(tcp_get_cookie_sock);
>  
> 
I prefer this form as well, but I'm not sure if removing the
"temporary" WARN() is appropriate for -net. If it is, I'll resubmit.
Otherwise I can refactor it after net-next reopens. Any opinion?

Guillaume