From patchwork Wed Jan 2 20:42:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauricio Faria de Oliveira X-Patchwork-Id: 1020070 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming-netdev@ozlabs.org Delivered-To: patchwork-incoming-netdev@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 43VNKl5ygNz9s4s for ; Thu, 3 Jan 2019 07:43:07 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727696AbfABUnH (ORCPT ); Wed, 2 Jan 2019 15:43:07 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:43827 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727193AbfABUnF (ORCPT ); Wed, 2 Jan 2019 15:43:05 -0500 Received: from mail-qk1-f199.google.com ([209.85.222.199]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1genLz-0002dM-MK for netdev@vger.kernel.org; Wed, 02 Jan 2019 20:43:03 +0000 Received: by mail-qk1-f199.google.com with SMTP id b185so38394870qkc.3 for ; Wed, 02 Jan 2019 12:43:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=E2Gs61w0fpdX78U7pMEDisXwaDylJYVJC2KFhQ7sJJ8=; b=WMyhmr+rUw1alKtY4rjhMeXuufslT9PTBZeHwkdwZpFxzWAJrn/7f6qLq5h+jbJn9X wGGh29Ok3D3vQD9z/dPPst1SDVA8Xa6qFPooDgw4DJxLYf5H3mil14hIeeTFo0nzo1Ug eJec+DczAFQLDXWWP38a9UlEZxDfqmad6nfK720+m0bwQXCsA44oxBWpJudrUICwF5NE Xdb7ClbSwyZ6MhLPLotGsmKIlrG685/HMiCw9/58GV1pTN1TOCC7VgBs3s7r9EfBaB+k PdTjrKQjWvi6MIXVGPQjtZis2/Vqr0aqUeR94ABmrcbLtK9BUAXR9i6SIkVKtENM+Zyq vIAA== X-Gm-Message-State: AJcUukdgIHywXqQOhxTh7xik6SuFDy4xBSAfThbQX7vYNCzypgGzlz5g 8xSKmn3JxJq9LB/imr3I4HJ/pXK3mv095mGsPXf/wam7k4ju/CH/EZiZkwgm406jaH9pRgLmMJP 88MVOWnieVRAHs6aFZ0sePegP+h+L1MQ5kw== X-Received: by 2002:a0c:9257:: with SMTP id 23mr43527270qvz.142.1546461782895; Wed, 02 Jan 2019 12:43:02 -0800 (PST) X-Google-Smtp-Source: ALg8bN7tMHCT13KtEPAIYPok4U2j7SrpFlB1UsNPFMIwZQ1XliYZ0dpasTb71LV3H1eqZa+ChNIZJg== X-Received: by 2002:a0c:9257:: with SMTP id 23mr43527261qvz.142.1546461782707; Wed, 02 Jan 2019 12:43:02 -0800 (PST) Received: from localhost.localdomain ([179.159.56.118]) by smtp.gmail.com with ESMTPSA id e17sm26679381qte.12.2019.01.02.12.42.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Jan 2019 12:43:02 -0800 (PST) From: Mauricio Faria de Oliveira To: stable@vger.kernel.org, netdev@vger.kernel.org, Florian Westphal Cc: Alakesh Haloi , nivedita.singhvi@canonical.com, Pablo Neira Ayuso , Jozsef Kadlecsik , "David S. Miller" , Yi-Hung Wei Subject: [PATCH v2 4.14 4/5] netfilter: nf_conncount: fix garbage collection confirm race Date: Wed, 2 Jan 2019 18:42:03 -0200 Message-Id: <20190102204204.12389-5-mfo@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20190102204204.12389-1-mfo@canonical.com> References: <20190102204204.12389-1-mfo@canonical.com> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Florian Westphal commit b36e4523d4d56e2595e28f16f6ccf1cd6a9fc452 upstream. Yi-Hung Wei and Justin Pettit found a race in the garbage collection scheme used by nf_conncount. When doing list walk, we lookup the tuple in the conntrack table. If the lookup fails we remove this tuple from our list because the conntrack entry is gone. This is the common cause, but turns out its not the only one. The list entry could have been created just before by another cpu, i.e. the conntrack entry might not yet have been inserted into the global hash. The avoid this, we introduce a timestamp and the owning cpu. If the entry appears to be stale, evict only if: 1. The current cpu is the one that added the entry, or, 2. The timestamp is older than two jiffies The second constraint allows GC to be taken over by other cpu too (e.g. because a cpu was offlined or napi got moved to another cpu). We can't pretend the 'doubtful' entry wasn't in our list. Instead, when we don't find an entry indicate via IS_ERR that entry was removed ('did not exist' or withheld ('might-be-unconfirmed'). This most likely also fixes a xt_connlimit imbalance earlier reported by Dmitry Andrianov. Cc: Dmitry Andrianov Reported-by: Justin Pettit Reported-by: Yi-Hung Wei Signed-off-by: Florian Westphal Acked-by: Yi-Hung Wei Signed-off-by: Pablo Neira Ayuso [mfo: backport: refresh context lines and use older symbol/file names: - nf_conncount.c -> xt_connlimit.c. - nf_conncount_rb -> xt_connlimit_rb - nf_conncount_tuple -> xt_connlimit_conn - conncount_conn_cachep -> connlimit_conn_cachep] Signed-off-by: Mauricio Faria de Oliveira --- net/netfilter/xt_connlimit.c | 52 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c index ab1f849..913b86ef 100644 --- a/net/netfilter/xt_connlimit.c +++ b/net/netfilter/xt_connlimit.c @@ -47,6 +47,8 @@ struct xt_connlimit_conn { struct hlist_node node; struct nf_conntrack_tuple tuple; struct nf_conntrack_zone zone; + int cpu; + u32 jiffies32; }; struct xt_connlimit_rb { @@ -126,11 +128,42 @@ bool nf_conncount_add(struct hlist_head *head, return false; conn->tuple = *tuple; conn->zone = *zone; + conn->cpu = raw_smp_processor_id(); + conn->jiffies32 = (u32)jiffies; hlist_add_head(&conn->node, head); return true; } EXPORT_SYMBOL_GPL(nf_conncount_add); +static const struct nf_conntrack_tuple_hash * +find_or_evict(struct net *net, struct xt_connlimit_conn *conn) +{ + const struct nf_conntrack_tuple_hash *found; + unsigned long a, b; + int cpu = raw_smp_processor_id(); + __s32 age; + + found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple); + if (found) + return found; + b = conn->jiffies32; + a = (u32)jiffies; + + /* conn might have been added just before by another cpu and + * might still be unconfirmed. In this case, nf_conntrack_find() + * returns no result. Thus only evict if this cpu added the + * stale entry or if the entry is older than two jiffies. + */ + age = a - b; + if (conn->cpu == cpu || age >= 2) { + hlist_del(&conn->node); + kmem_cache_free(connlimit_conn_cachep, conn); + return ERR_PTR(-ENOENT); + } + + return ERR_PTR(-EAGAIN); +} + unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head, const struct nf_conntrack_tuple *tuple, const struct nf_conntrack_zone *zone, @@ -138,18 +171,27 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head, { const struct nf_conntrack_tuple_hash *found; struct xt_connlimit_conn *conn; - struct hlist_node *n; struct nf_conn *found_ct; + struct hlist_node *n; unsigned int length = 0; *addit = true; /* check the saved connections */ hlist_for_each_entry_safe(conn, n, head, node) { - found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple); - if (found == NULL) { - hlist_del(&conn->node); - kmem_cache_free(connlimit_conn_cachep, conn); + found = find_or_evict(net, conn); + if (IS_ERR(found)) { + /* Not found, but might be about to be confirmed */ + if (PTR_ERR(found) == -EAGAIN) { + length++; + if (!tuple) + continue; + + if (nf_ct_tuple_equal(&conn->tuple, tuple) && + nf_ct_zone_id(&conn->zone, conn->zone.dir) == + nf_ct_zone_id(zone, zone->dir)) + *addit = false; + } continue; }