From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1o15jN/Zdxp4cCBMGAsqjBQdsQnQ2oQJcN8GzuXt6+g=; b=LDHiGcvkeVB/pOMRU4SN9DQPLhQa74ZuJYN/Kld6UV8Qe1G9pFKZ7lrRNJlkACsOkN0zHe7NcsCEmnAg3MOKEGSPp5M01oIRclLm/Rb+dMoUtnpIhexTGmRPO7QaQ50OGlGPSSqR3dIRWeNRXuhmXxcFMi35x8wfT7abou9uDCBA1A/F2BhOzU0LB9BGqORQTbyiTbKl1IJ3ZJcDXlRoW5gYr1jaN52D3dJJy2/XPR+/MbwgOydnoXnVtDM8CFxAkuKFNVfrwqpqlHjH10ULs8+koY+fRT8LMMvwqtews1zhNitlncUqpNQVlJPaYVdw0TQwYU8uXR2bRzglhU43Vg== References: <20210802113633.189831-1-vladimir.oltean@nxp.com> From: Nikolay Aleksandrov Message-ID: Date: Mon, 2 Aug 2021 14:40:05 +0300 In-Reply-To: <20210802113633.189831-1-vladimir.oltean@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [Bridge] [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Oltean , netdev@vger.kernel.org, Jakub Kicinski , "David S. Miller" Cc: Ido Schimmel , bridge@lists.linux-foundation.org, Jiri Pirko , Roopa Prabhu On 02/08/2021 14:36, Vladimir Oltean wrote: > Nikolay points out that it is incorrect to assume that it is impossible > to have an fdb entry with fdb->dst == NULL and the BR_FDB_LOCAL bit in > fdb->flags not set. This is because there are reader-side places that > test_bit(BR_FDB_LOCAL, &fdb->flags) without the br->hash_lock, and if > the updating of the FDB entry happens on another CPU, there are no > memory barriers at writer or reader side which would ensure that the > reader sees the updates to both fdb->flags and fdb->dst in the same > order, i.e. the reader will not see an inconsistent FDB entry. > > So we must be prepared to deal with FDB entries where fdb->dst and > fdb->flags are in a potentially inconsistent state, and that means that > fdb->dst == NULL should remain a condition to pick the net_device that > we report to switchdev as being the bridge device, which is what the > code did prior to the blamed patch. > > Fixes: 52e4bec15546 ("net: bridge: switchdev: treat local FDBs the same as entries towards the bridge") > Suggested-by: Nikolay Aleksandrov > Signed-off-by: Vladimir Oltean > --- > net/bridge/br_fdb.c | 2 +- > net/bridge/br_switchdev.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index 4ff8c67ac88f..af31cebfda94 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -745,7 +745,7 @@ static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb, > item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); > item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags); > item.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags); > - item.info.dev = item.is_local ? br->dev : p->dev; > + item.info.dev = (!p || item.is_local) ? br->dev : p->dev; > item.info.ctx = ctx; > > err = nb->notifier_call(nb, action, &item); > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index 023de0e958f1..36d75fd4a80c 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -134,7 +134,7 @@ br_switchdev_fdb_notify(struct net_bridge *br, > .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), > .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), > }; > - struct net_device *dev = info.is_local ? br->dev : dst->dev; > + struct net_device *dev = (!dst || info.is_local) ? br->dev : dst->dev; > > switch (type) { > case RTM_DELNEIGH: > Thanks, Acked-by: Nikolay Aleksandrov