From patchwork Wed Mar 17 18:53:19 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thadeu Lima de Souza Cascardo X-Patchwork-Id: 12146801 X-Patchwork-Delegate: kuba@kernel.org 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=-16.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT 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 7CFDCC433E0 for ; Wed, 17 Mar 2021 18:55:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3B4E864F57 for ; Wed, 17 Mar 2021 18:55:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233053AbhCQSyv (ORCPT ); Wed, 17 Mar 2021 14:54:51 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:39339 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232979AbhCQSyS (ORCPT ); Wed, 17 Mar 2021 14:54:18 -0400 Received: from 1.general.cascardo.us.vpn ([10.172.70.58] helo=localhost.localdomain) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lMbJA-0007zv-0s; Wed, 17 Mar 2021 18:54:16 +0000 From: Thadeu Lima de Souza Cascardo To: netdev@vger.kernel.org Cc: davem@davemloft.net, kuba@kernel.org, dsahern@kernel.org, Thadeu Lima de Souza Cascardo Subject: [PATCH 1/2] neighbour: allow referenced neighbours to be removed Date: Wed, 17 Mar 2021 15:53:19 -0300 Message-Id: <20210317185320.1561608-1-cascardo@canonical.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org X-Patchwork-Delegate: kuba@kernel.org During forced garbage collection, neighbours with more than a reference are not removed. It's possible to DoS the neighbour table by using ARP spoofing in such a way that there is always a timer pending for all neighbours, preventing any of them from being removed. That will cause any new neighbour creation to fail. Use the same code as used by neigh_flush_dev, which deletes the timer and cleans the queue when there are still references left. With the same ARP spoofing technique, it was still possible to reach a valid destination when this fix was applied, with no more table overflows. Signed-off-by: Thadeu Lima de Souza Cascardo --- net/core/neighbour.c | 117 +++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 66 deletions(-) diff --git a/net/core/neighbour.c b/net/core/neighbour.c index e2982b3970b8..bbc89c7ffdfd 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -173,25 +173,48 @@ static bool neigh_update_ext_learned(struct neighbour *neigh, u32 flags, return rc; } +static int neigh_del_timer(struct neighbour *n) +{ + if ((n->nud_state & NUD_IN_TIMER) && + del_timer(&n->timer)) { + neigh_release(n); + return 1; + } + return 0; +} + static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np, struct neigh_table *tbl) { - bool retval = false; - + rcu_assign_pointer(*np, + rcu_dereference_protected(n->next, + lockdep_is_held(&tbl->lock))); write_lock(&n->lock); - if (refcount_read(&n->refcnt) == 1) { - struct neighbour *neigh; - - neigh = rcu_dereference_protected(n->next, - lockdep_is_held(&tbl->lock)); - rcu_assign_pointer(*np, neigh); - neigh_mark_dead(n); - retval = true; + neigh_del_timer(n); + neigh_mark_dead(n); + if (refcount_read(&n->refcnt) != 1) { + /* The most unpleasant situation. + We must destroy neighbour entry, + but someone still uses it. + + The destroy will be delayed until + the last user releases us, but + we must kill timers etc. and move + it to safe state. + */ + __skb_queue_purge(&n->arp_queue); + n->arp_queue_len_bytes = 0; + n->output = neigh_blackhole; + if (n->nud_state & NUD_VALID) + n->nud_state = NUD_NOARP; + else + n->nud_state = NUD_NONE; + neigh_dbg(2, "neigh %p is stray\n", n); } write_unlock(&n->lock); - if (retval) - neigh_cleanup_and_release(n); - return retval; + neigh_cleanup_and_release(n); + + return true; } bool neigh_remove_one(struct neighbour *ndel, struct neigh_table *tbl) @@ -229,22 +252,20 @@ static int neigh_forced_gc(struct neigh_table *tbl) write_lock_bh(&tbl->lock); list_for_each_entry_safe(n, tmp, &tbl->gc_list, gc_list) { - if (refcount_read(&n->refcnt) == 1) { - bool remove = false; - - write_lock(&n->lock); - if ((n->nud_state == NUD_FAILED) || - (tbl->is_multicast && - tbl->is_multicast(n->primary_key)) || - time_after(tref, n->updated)) - remove = true; - write_unlock(&n->lock); - - if (remove && neigh_remove_one(n, tbl)) - shrunk++; - if (shrunk >= max_clean) - break; - } + bool remove = false; + + write_lock(&n->lock); + if ((n->nud_state == NUD_FAILED) || + (tbl->is_multicast && + tbl->is_multicast(n->primary_key)) || + time_after(tref, n->updated)) + remove = true; + write_unlock(&n->lock); + + if (remove && neigh_remove_one(n, tbl)) + shrunk++; + if (shrunk >= max_clean) + break; } tbl->last_flush = jiffies; @@ -264,16 +285,6 @@ static void neigh_add_timer(struct neighbour *n, unsigned long when) } } -static int neigh_del_timer(struct neighbour *n) -{ - if ((n->nud_state & NUD_IN_TIMER) && - del_timer(&n->timer)) { - neigh_release(n); - return 1; - } - return 0; -} - static void pneigh_queue_purge(struct sk_buff_head *list) { struct sk_buff *skb; @@ -307,33 +318,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev, np = &n->next; continue; } - rcu_assign_pointer(*np, - rcu_dereference_protected(n->next, - lockdep_is_held(&tbl->lock))); - write_lock(&n->lock); - neigh_del_timer(n); - neigh_mark_dead(n); - if (refcount_read(&n->refcnt) != 1) { - /* The most unpleasant situation. - We must destroy neighbour entry, - but someone still uses it. - - The destroy will be delayed until - the last user releases us, but - we must kill timers etc. and move - it to safe state. - */ - __skb_queue_purge(&n->arp_queue); - n->arp_queue_len_bytes = 0; - n->output = neigh_blackhole; - if (n->nud_state & NUD_VALID) - n->nud_state = NUD_NOARP; - else - n->nud_state = NUD_NONE; - neigh_dbg(2, "neigh %p is stray\n", n); - } - write_unlock(&n->lock); - neigh_cleanup_and_release(n); + neigh_del(n, np, tbl); } } } From patchwork Wed Mar 17 18:53:20 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thadeu Lima de Souza Cascardo X-Patchwork-Id: 12146803 X-Patchwork-Delegate: kuba@kernel.org 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=-16.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT 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 DB12BC433E9 for ; Wed, 17 Mar 2021 18:55:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 92A7264F51 for ; Wed, 17 Mar 2021 18:55:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233086AbhCQSyw (ORCPT ); Wed, 17 Mar 2021 14:54:52 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:39348 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233028AbhCQSy1 (ORCPT ); Wed, 17 Mar 2021 14:54:27 -0400 Received: from 1.general.cascardo.us.vpn ([10.172.70.58] helo=localhost.localdomain) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lMbJJ-0007zv-QH; Wed, 17 Mar 2021 18:54:26 +0000 From: Thadeu Lima de Souza Cascardo To: netdev@vger.kernel.org Cc: davem@davemloft.net, kuba@kernel.org, dsahern@kernel.org, Thadeu Lima de Souza Cascardo , Kasper Dupont Subject: [PATCH 2/2] neighbour: allow NUD_NOARP entries to be forced GCed Date: Wed, 17 Mar 2021 15:53:20 -0300 Message-Id: <20210317185320.1561608-2-cascardo@canonical.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20210317185320.1561608-1-cascardo@canonical.com> References: <20210317185320.1561608-1-cascardo@canonical.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org X-Patchwork-Delegate: kuba@kernel.org IFF_POINTOPOINT interfaces use NUD_NOARP entries for IPv6. It's possible to fill up the neighbour table with enough entries that it will overflow for valid connections after that. This behaviour is more prevalent after commit 58956317c8de ("neighbor: Improve garbage collection") is applied, as it prevents removal from entries that are not NUD_FAILED, unless they are more than 5s old. Fixes: 58956317c8de (neighbor: Improve garbage collection) Reported-by: Kasper Dupont Signed-off-by: Thadeu Lima de Souza Cascardo --- net/core/neighbour.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/core/neighbour.c b/net/core/neighbour.c index bbc89c7ffdfd..be5ca411b149 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -256,6 +256,7 @@ static int neigh_forced_gc(struct neigh_table *tbl) write_lock(&n->lock); if ((n->nud_state == NUD_FAILED) || + (n->nud_state == NUD_NOARP) || (tbl->is_multicast && tbl->is_multicast(n->primary_key)) || time_after(tref, n->updated))