* [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device
@ 2021-08-02 11:36 Vladimir Oltean
2021-08-02 11:40 ` Nikolay Aleksandrov
2021-08-03 21:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Vladimir Oltean @ 2021-08-02 11:36 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Jiri Pirko, Ido Schimmel, Roopa Prabhu, Nikolay Aleksandrov, bridge
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 <nikolay@nvidia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
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:
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device
2021-08-02 11:36 [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device Vladimir Oltean
@ 2021-08-02 11:40 ` Nikolay Aleksandrov
2021-08-03 21:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Nikolay Aleksandrov @ 2021-08-02 11:40 UTC (permalink / raw)
To: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller
Cc: Jiri Pirko, Ido Schimmel, Roopa Prabhu, bridge
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 <nikolay@nvidia.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> 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 <nikolay@nvidia.com>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device
2021-08-02 11:36 [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device Vladimir Oltean
2021-08-02 11:40 ` Nikolay Aleksandrov
@ 2021-08-03 21:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-03 21:40 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: netdev, kuba, davem, jiri, idosch, roopa, nikolay, bridge
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Mon, 2 Aug 2021 14:36:33 +0300 you 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.
>
> [...]
Here is the summary with links:
- [net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device
https://git.kernel.org/netdev/net-next/c/2e19bb35ce15
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-03 21:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02 11:36 [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device Vladimir Oltean
2021-08-02 11:40 ` Nikolay Aleksandrov
2021-08-03 21:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).