From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1D7B9C43381 for ; Fri, 8 Mar 2019 22:22:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E9942206DF for ; Fri, 8 Mar 2019 22:22:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726363AbfCHWWg (ORCPT ); Fri, 8 Mar 2019 17:22:36 -0500 Received: from mail-wm1-f67.google.com ([209.85.128.67]:53988 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726286AbfCHWWg (ORCPT ); Fri, 8 Mar 2019 17:22:36 -0500 Received: by mail-wm1-f67.google.com with SMTP id e74so13869491wmg.3 for ; Fri, 08 Mar 2019 14:22:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=vaDVzbcNnkWA+MI9+FVD8LNDnYqPg3pVuYjZu1a7aAM=; b=Y9Dj1HREsnA5J4vcgOU9nsV4nNMqVu66hCUzs/x7dWt9NfWwvyW+iKe5/Qz2RKxsco yZIEEc/rlsAbyQwBS7655aANCdyKaAPc9wVLaOeobd1ZPqYDzrjPHg04Zn9vIioagRty 9y9KJ8UIPc7F/doyZsHqY8CzJHYWkWivX+u9K8d5DxNHZ1j2+wwIguhZl6TPu8bL9oB5 TXqmww6JhCZYTnl8rUTIaDK3L0eDwvtoRTv8MxAyJ4tpZ8evvCdhM9khxTdWV6Q/ukXJ 9m+WA5fn9pnikjK1VbeSEyAM3SKeqkt4ZPMyPSCmZtREudtg/GSK70iM3dRX6Lfxq+k/ rz/A== X-Gm-Message-State: APjAAAUafgGxUW/CsjmKfjcro/zT/WyLJoFVKyKuc4C+JqFKzWajcX+L 0GKzwokSsDRLG0nxstUrvzA0IBgGHkU= X-Google-Smtp-Source: APXvYqybWGn3wjY2XPBVfl1WRtsm/WXfi4bMFr0nfAgL96tAEpvPWGjFvgCF1xgJMCNfYu+OipvN1A== X-Received: by 2002:a1c:e084:: with SMTP id x126mr10788651wmg.39.1552083754741; Fri, 08 Mar 2019 14:22:34 -0800 (PST) Received: from pc-2.home (2a01cb05850ddf00045dd60e6368f84b.ipv6.abo.wanadoo.fr. [2a01:cb05:850d:df00:45d:d60e:6368:f84b]) by smtp.gmail.com with ESMTPSA id r63sm14901803wmr.32.2019.03.08.14.22.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 08 Mar 2019 14:22:34 -0800 (PST) Date: Fri, 8 Mar 2019 23:22:32 +0100 From: Guillaume Nault To: Eric Dumazet Cc: netdev@vger.kernel.org Subject: Re: [PATCH net] tcp: handle inet_csk_reqsk_queue_add() failures Message-ID: <20190308222231.GA26006@pc-2.home> References: <9b8502f9cec31c971e480ee2281f5cd7088b50df.1552077823.git.gnault@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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