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=-7.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS 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 11D18C43381 for ; Fri, 8 Mar 2019 21:33:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D32F720652 for ; Fri, 8 Mar 2019 21:33:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="owm/820J" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726671AbfCHVdG (ORCPT ); Fri, 8 Mar 2019 16:33:06 -0500 Received: from mail-pg1-f193.google.com ([209.85.215.193]:34293 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726414AbfCHVdG (ORCPT ); Fri, 8 Mar 2019 16:33:06 -0500 Received: by mail-pg1-f193.google.com with SMTP id i130so15127636pgd.1 for ; Fri, 08 Mar 2019 13:33:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=KzN91Iry08cYdHlfm0TX6Hd+s4SJH3HhxRZl6SFYJww=; b=owm/820JyGPW9c9ICstbB5KXm7GlyfLUq/ClZuC/xMw8RkSN36bXKA7HC7k7yQ3OE1 7nA4nuauBKxhPjMcmPcMR6Py34ZYh818I3ZXWDdgybxs66IC8GodhpOLIB+dBtTO3gTp fb5MxR7U5ryVnyGVwcPhh6mkMVPQoZdGrKqQLWL7hi2NKPdjo7te9Hr/7THGKAibBUSG 19xfF6xltVYH6WO0qLMeny/Ua0qqL5bbza4NEpussFOy6Bql8FagdYuTObG8LHFcvbkz lcIptmmjKxjhEvCB7SCiT1/CpOoutN/JdWfCW4ETzudj41xYZqgtbURllE2uhPhk3wAg 4GrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KzN91Iry08cYdHlfm0TX6Hd+s4SJH3HhxRZl6SFYJww=; b=cAFyrr51hOO9buRRhjgCuMdrdPspE0LdBY94gMfjHs7/UvMxPfg0FfxTlshT/PAt5V GnzHu51ogfBgcvMYbtXqGrIo3x8EeUCwEoQ1c5t/paWlx89qh3uWevd5yNf8stqH7es+ SFu2V7TSHdp3FOGByoxT41EB+FIzDP3nuTRKohC0uqRhriZ6juB4VWxhqBqyKOlSlI6t K+bRdCtQ1UUX3eEfTxuHBhH+NV7N+6woc9SxK6kEkl7CpXVFYeQTRFJDPAb7ARtyvd8Z LHAUF7kZ0QD+fQ2Ej5waY6lPEOsmC2n32zXC4hyARBrEsci7d+yY+/HqIp5z0BJmTv9L 5nFA== X-Gm-Message-State: APjAAAW8w9bT/UQuTQBerhDl1UnxYPZSdmNSGDBabU61jI7kuKJXOLYE tcOw/AtUiwT0yYtJ/che19N9tz/W X-Google-Smtp-Source: APXvYqzOPa+XpUCW7/aNh5u3p77bzU6+h3Hmu8Balqbc7HzngYOz6fCERQ5jCLQwtjkd/IfPZYUYKA== X-Received: by 2002:a63:694a:: with SMTP id e71mr1541364pgc.129.1552080785024; Fri, 08 Mar 2019 13:33:05 -0800 (PST) Received: from ?IPv6:2620:15c:2c1:200:55c7:81e6:c7d8:94b? ([2620:15c:2c1:200:55c7:81e6:c7d8:94b]) by smtp.gmail.com with ESMTPSA id a184sm14413049pge.68.2019.03.08.13.33.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Mar 2019 13:33:03 -0800 (PST) Subject: Re: [PATCH net] tcp: handle inet_csk_reqsk_queue_add() failures To: Guillaume Nault , netdev@vger.kernel.org References: <9b8502f9cec31c971e480ee2281f5cd7088b50df.1552077823.git.gnault@redhat.com> From: Eric Dumazet Message-ID: Date: Fri, 8 Mar 2019 13:33:02 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <9b8502f9cec31c971e480ee2281f5cd7088b50df.1552077823.git.gnault@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 03/08/2019 01:09 PM, Guillaume Nault wrote: > Commit 7716682cc58e ("tcp/dccp: fix another race at listener > dismantle") let inet_csk_reqsk_queue_add() fail, and adjusted > {tcp,dccp}_check_req() accordingly. However, TFO and syncookies > weren't modified, thus leaking allocated resources on error. > > Contrary to tcp_check_req(), in both syncookies and TFO cases, > we need to drop the request socket. Also, since the child socket is > created with inet_csk_clone_lock(), we have to unlock it and drop an > extra reference (->sk_refcount is initially set to 2 and > inet_csk_reqsk_queue_add() drops only one ref). > > For TFO, we also need to revert the work done by tcp_try_fastopen() > (with reqsk_fastopen_remove()). > > Fixes: 7716682cc58e ("tcp/dccp: fix another race at listener dismantle") > Signed-off-by: Guillaume Nault > --- > > Note for stable backports: this patch relies on da8ab57863ed > ("tcp/dccp: remove reqsk_put() from inet_child_forget()"), to prevent > inet_child_forget() from dropping a reference from the request socket. > > Therefore, for trees older than 4.14, commit da8ab57863ed has to be > backported before this patch. > Thanks for working on this issue (it was on my radar as well) > > net/ipv4/syncookies.c | 7 ++++++- > net/ipv4/tcp_input.c | 8 +++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > index 606f868d9f3f..e531344611a0 100644 > --- a/net/ipv4/syncookies.c > +++ b/net/ipv4/syncookies.c > @@ -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 ? 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); > + } > } else { > reqsk_free(req); > } > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 4eb0c8ca3c60..5def3c48870e 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -6498,7 +6498,13 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, > af_ops->send_synack(fastopen_sk, dst, &fl, req, > &foc, TCP_SYNACK_FASTOPEN); > /* Add the child socket directly into the accept queue */ > - inet_csk_reqsk_queue_add(sk, req, fastopen_sk); > + if (!inet_csk_reqsk_queue_add(sk, req, fastopen_sk)) { > + reqsk_fastopen_remove(fastopen_sk, req, false); > + bh_unlock_sock(fastopen_sk); > + sock_put(fastopen_sk); > + reqsk_put(req); > + goto drop; These two lines can be replaced by : goto drop_and_free; > + } > sk->sk_data_ready(sk); > bh_unlock_sock(fastopen_sk); > sock_put(fastopen_sk); >