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
