From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net v2] net: bridge: Fix locking in br_fdb_find_port() Date: Fri, 8 Jun 2018 16:39:50 +0300 Message-ID: <3f7a5287-08c7-867d-10c5-e22ff4febfc6@cumulusnetworks.com> References: <38d89430-32c4-8842-efd4-88e7d3912506@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: stephen@networkplumber.org, davem@davemloft.net To: Petr Machata , bridge@lists.linux-foundation.org, netdev@vger.kernel.org Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:36843 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752706AbeFHNjx (ORCPT ); Fri, 8 Jun 2018 09:39:53 -0400 Received: by mail-wm0-f67.google.com with SMTP id v131-v6so3832853wma.1 for ; Fri, 08 Jun 2018 06:39:52 -0700 (PDT) In-Reply-To: <38d89430-32c4-8842-efd4-88e7d3912506@cumulusnetworks.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 08/06/18 16:35, Nikolay Aleksandrov wrote: > On 08/06/18 16:11, Petr Machata wrote: >> Callers of br_fdb_find() need to hold the hash lock, which >> br_fdb_find_port() doesn't do. However, since br_fdb_find_port() is not >> doing any actual FDB manipulation, the hash lock is not really needed at >> all. So convert to br_fdb_find_rcu(), surrounded by rcu_read_lock() / >> _unlock() pair. >> >> The device pointer copied from inside the FDB entry is then kept alive >> by the RTNL lock, which br_fdb_find_port() asserts. >> >> Fixes: 4d4fd36126d6 ("net: bridge: Publish bridge accessor functions") >> Signed-off-by: Petr Machata >> --- >> >> Notes: >> Changes from v1 to v2: >> >> - Instead of taking hash lock, take RCU lock and call br_fdb_find_rcu(). >> >> net/bridge/br_fdb.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >> index b19e310..502f663 100644 >> --- a/net/bridge/br_fdb.c >> +++ b/net/bridge/br_fdb.c >> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, >> return NULL; >> >> br = netdev_priv(br_dev); >> - f = br_fdb_find(br, addr, vid); >> + rcu_read_lock(); >> + f = br_fdb_find_rcu(br, addr, vid); >> if (f && f->dst) >> dev = f->dst->dev; >> + rcu_read_unlock(); >> >> return dev; >> } >> > > Important note: the only reason this will not dereference a NULL pointer > when getting f->dst is because RTNL is held in all of its current > callers. I missed the comments on the previous version, but using RCU > here is dangerous if someone decides to use this without rtnl they will > get a false sense of security, that is why I acked the previous version. > I'd suggest to use READ_ONCE() for f->dst to avoid reading it again. > Nevermind the READ_ONCE part, I missed that there's ASSERT_RTNL() in the beginning of this function, so it'll always be used with RTNL. :-) It's good as it stands, I need to get some coffee. Acked-by: Nikolay Aleksandrov