From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:58836 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725770AbeKTSQ1 (ORCPT ); Tue, 20 Nov 2018 13:16:27 -0500 Date: Tue, 20 Nov 2018 08:48:39 +0100 From: Greg KH To: Alakesh Haloi Cc: stable@vger.kernel.org, Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" Subject: Re: [PATCH] netfilter: xt_connlimit: fix race in connection counting Message-ID: <20181120074839.GC15276@kroah.com> References: <20181119221732.GA82454@dev-dsk-alakeshh-2c-f8a3e6e0.us-west-2.amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181119221732.GA82454@dev-dsk-alakeshh-2c-f8a3e6e0.us-west-2.amazon.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Nov 19, 2018 at 10:17:38PM +0000, Alakesh Haloi wrote: > An iptable rule like the following on a multicore systems will result in > accepting more connections than set in the rule. > > iptables -A INPUT -p tcp -m tcp --syn --dport 7777 -m connlimit \ > --connlimit-above 2000 --connlimit-mask 0 -j DROP > > In check_hlist function, connections that are found in saved connections > but not in netfilter conntrack are deleted, assuming that those > connections do not exist anymore. But for multi core systems, there exists > a small time window, when a connection has been added to the xt_connlimit > maintained rb-tree but has not yet made to netfilter conntrack table. This > causes concurrent connections to return incorrect counts and go over limit > set in iptable rule. > > Connection 1 on Core 1 Connection 2 on Core 2 > > list_length = N > conntrack_table_len = N > spin_lock_bh() > In check_hlist() function > a. loop over saved connections > 1. call nf_conntrack_find_get() > 2. If not found in 1, > i. call hlist_del() > b. return total count to caller > c. connection gets added to list > of saved connections. > spin_unlock_bh() > list_length = N + 1 > spin_lock_bh() on core 2 > In check_hlist() function > a. loop over saved connection. > 1. call nf_conntrack_find_get() > 2. If not found in 1. > i. call hlist_del() > [Connection 1 was in the > but not in nf_conntrack yet] > ii. connection 1 gets deleted > > list_length = N > conntrack_table_len = N > b. return total count to caller > c. connection 2 gets added to list > of saved connections > spin_unlock_bh() > > d. connection 1 gets added to > nf_conntrack > list_length = N + 1 > conntrack_table_len = N + 1 > e. connection 2 gets added to > nf_conntrack > list_length = N + 1 > conntrack_table_len = N + 2 > > So we end up with N + 1 connections in the list but N + 2 in nf_conntrack, > allowing more number of connections eventually than set in the rule. > > This fix adds an additional field to track such pending connections > and prevent them from being deleted by another execution thread on > a different core and returns correct count. > > Signed-off-by: Alakesh Haloi > Cc: Pablo Neira Ayuso > Cc: Jozsef Kadlecsik > Cc: Florian Westphal > Cc: "David S. Miller" > Cc: stable@vger.kernel.org # v4.15 and before > --- > net/netfilter/xt_connlimit.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) What is the git commit id of this patch in Linus's tree? thanks, greg k-h