| Message ID | 20210414165256.1837753-3-olteanv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Pass the BR_FDB_LOCAL information to switchdev drivers | expand |
On 14/04/2021 19:52, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify > switchdev for local FDB addresses") as well as in this discussion: > https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/ > > the switchdev notifiers for FDB entries managed to have a zero-day bug, > which was that drivers would not know what to do with local FDB entries, > because they were not told that they are local. The bug fix was to > simply not notify them of those addresses. > > Let us now add the 'is_local' bit to bridge FDB entries, and make all > drivers ignore these entries by their own choice. > > Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c | 4 ++-- > drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +- > drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 5 +++-- > drivers/net/ethernet/rocker/rocker_main.c | 4 ++-- > drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 4 ++-- > drivers/net/ethernet/ti/cpsw_switchdev.c | 4 ++-- > include/net/switchdev.h | 1 + > net/bridge/br_switchdev.c | 3 +-- > net/dsa/slave.c | 2 +- > 9 files changed, 15 insertions(+), 14 deletions(-) For cpsw: Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com> Thank you.
On Wed, Apr 14, 2021 at 07:52:56PM +0300, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify > switchdev for local FDB addresses") as well as in this discussion: > https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/ > > the switchdev notifiers for FDB entries managed to have a zero-day bug, > which was that drivers would not know what to do with local FDB entries, > because they were not told that they are local. The bug fix was to > simply not notify them of those addresses. > > Let us now add the 'is_local' bit to bridge FDB entries, and make all > drivers ignore these entries by their own choice. > > Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com> One comment below > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index c390f84adea2..a5e601e41cb9 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > .addr = fdb->key.addr.addr, > .vid = fdb->key.vlan_id, > .added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags), > + .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), > .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), > }; > > if (!fdb->dst) > return; Do you plan to eventually remove this check so that entries pointing to the bridge device itself will be notified? For example: # bridge fdb add 00:01:02:03:04:05 dev br0 self local > - if (test_bit(BR_FDB_LOCAL, &fdb->flags)) > - return; > > switch (type) { > case RTM_DELNEIGH:
On Fri, Apr 16, 2021 at 06:22:08PM +0300, Ido Schimmel wrote: > On Wed, Apr 14, 2021 at 07:52:56PM +0300, Vladimir Oltean wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify > > switchdev for local FDB addresses") as well as in this discussion: > > https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/ > > > > the switchdev notifiers for FDB entries managed to have a zero-day bug, > > which was that drivers would not know what to do with local FDB entries, > > because they were not told that they are local. The bug fix was to > > simply not notify them of those addresses. > > > > Let us now add the 'is_local' bit to bridge FDB entries, and make all > > drivers ignore these entries by their own choice. > > > > Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com> > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> Thanks! > One comment below > > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > > index c390f84adea2..a5e601e41cb9 100644 > > --- a/net/bridge/br_switchdev.c > > +++ b/net/bridge/br_switchdev.c > > @@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > > .addr = fdb->key.addr.addr, > > .vid = fdb->key.vlan_id, > > .added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags), > > + .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), > > .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), > > }; > > > > if (!fdb->dst) > > return; > > Do you plan to eventually remove this check so that entries pointing to > the bridge device itself will be notified? For example: > > # bridge fdb add 00:01:02:03:04:05 dev br0 self local > > > - if (test_bit(BR_FDB_LOCAL, &fdb->flags)) > > - return; > > > > switch (type) { > > case RTM_DELNEIGH: Yes I do, it's this patch over here: https://patchwork.kernel.org/project/netdevbpf/patch/20210224114350.2791260-10-olteanv@gmail.com/ One at a time though.
On 14/04/2021 19:52, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify > switchdev for local FDB addresses") as well as in this discussion: > https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/ > > the switchdev notifiers for FDB entries managed to have a zero-day bug, > which was that drivers would not know what to do with local FDB entries, > because they were not told that they are local. The bug fix was to > simply not notify them of those addresses. > > Let us now add the 'is_local' bit to bridge FDB entries, and make all > drivers ignore these entries by their own choice. > > Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c | 4 ++-- > drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +- > drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 5 +++-- > drivers/net/ethernet/rocker/rocker_main.c | 4 ++-- > drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 4 ++-- > drivers/net/ethernet/ti/cpsw_switchdev.c | 4 ++-- > include/net/switchdev.h | 1 + > net/bridge/br_switchdev.c | 3 +-- > net/dsa/slave.c | 2 +- > 9 files changed, 15 insertions(+), 14 deletions(-) > For the bridge change: Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c index 80efc8116963..d7c0e11b16f7 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c @@ -1832,7 +1832,7 @@ static void dpaa2_switch_event_work(struct work_struct *work) switch (switchdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; if (is_unicast_ether_addr(fdb_info->addr)) err = dpaa2_switch_port_fdb_add_uc(netdev_priv(dev), @@ -1847,7 +1847,7 @@ static void dpaa2_switch_event_work(struct work_struct *work) &fdb_info->info, NULL); break; case SWITCHDEV_FDB_DEL_TO_DEVICE: - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; if (is_unicast_ether_addr(fdb_info->addr)) dpaa2_switch_port_fdb_del_uc(netdev_priv(dev), fdb_info->addr); diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c index 49e052273f30..cb564890a3dc 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c @@ -798,7 +798,7 @@ static void prestera_fdb_event_work(struct work_struct *work) switch (swdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: fdb_info = &swdev_work->fdb_info; - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; err = prestera_port_fdb_set(port, fdb_info, true); diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c index c1f05c17557d..eeccd586e781 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c @@ -2916,7 +2916,8 @@ mlxsw_sp_switchdev_bridge_nve_fdb_event(struct mlxsw_sp_switchdev_event_work * return; if (switchdev_work->event == SWITCHDEV_FDB_ADD_TO_DEVICE && - !switchdev_work->fdb_info.added_by_user) + (!switchdev_work->fdb_info.added_by_user || + switchdev_work->fdb_info.is_local)) return; if (!netif_running(dev)) @@ -2971,7 +2972,7 @@ static void mlxsw_sp_switchdev_bridge_fdb_event_work(struct work_struct *work) switch (switchdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: fdb_info = &switchdev_work->fdb_info; - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; err = mlxsw_sp_port_fdb_set(mlxsw_sp_port, fdb_info, true); if (err) diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c index 3473d296b2e2..a46633606cae 100644 --- a/drivers/net/ethernet/rocker/rocker_main.c +++ b/drivers/net/ethernet/rocker/rocker_main.c @@ -2736,7 +2736,7 @@ static void rocker_switchdev_event_work(struct work_struct *work) switch (switchdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: fdb_info = &switchdev_work->fdb_info; - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; err = rocker_world_port_fdb_add(rocker_port, fdb_info); if (err) { @@ -2747,7 +2747,7 @@ static void rocker_switchdev_event_work(struct work_struct *work) break; case SWITCHDEV_FDB_DEL_TO_DEVICE: fdb_info = &switchdev_work->fdb_info; - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) break; err = rocker_world_port_fdb_del(rocker_port, fdb_info); if (err) diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c index d93ffd8a08b0..23cfb91e9c4d 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c @@ -385,7 +385,7 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work) fdb->addr, fdb->vid, fdb->added_by_user, fdb->offloaded, port_id); - if (!fdb->added_by_user) + if (!fdb->added_by_user || fdb->is_local) break; if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0) port_id = HOST_PORT_NUM; @@ -401,7 +401,7 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work) fdb->addr, fdb->vid, fdb->added_by_user, fdb->offloaded, port_id); - if (!fdb->added_by_user) + if (!fdb->added_by_user || fdb->is_local) break; if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0) port_id = HOST_PORT_NUM; diff --git a/drivers/net/ethernet/ti/cpsw_switchdev.c b/drivers/net/ethernet/ti/cpsw_switchdev.c index a72bb570756f..05a64fb7a04f 100644 --- a/drivers/net/ethernet/ti/cpsw_switchdev.c +++ b/drivers/net/ethernet/ti/cpsw_switchdev.c @@ -395,7 +395,7 @@ static void cpsw_switchdev_event_work(struct work_struct *work) fdb->addr, fdb->vid, fdb->added_by_user, fdb->offloaded, port); - if (!fdb->added_by_user) + if (!fdb->added_by_user || fdb->is_local) break; if (memcmp(priv->mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0) port = HOST_PORT_NUM; @@ -411,7 +411,7 @@ static void cpsw_switchdev_event_work(struct work_struct *work) fdb->addr, fdb->vid, fdb->added_by_user, fdb->offloaded, port); - if (!fdb->added_by_user) + if (!fdb->added_by_user || fdb->is_local) break; if (memcmp(priv->mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0) port = HOST_PORT_NUM; diff --git a/include/net/switchdev.h b/include/net/switchdev.h index 8c3218177136..f1a5a9a3634d 100644 --- a/include/net/switchdev.h +++ b/include/net/switchdev.h @@ -209,6 +209,7 @@ struct switchdev_notifier_fdb_info { const unsigned char *addr; u16 vid; u8 added_by_user:1, + is_local:1, offloaded:1; }; diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index c390f84adea2..a5e601e41cb9 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) .addr = fdb->key.addr.addr, .vid = fdb->key.vlan_id, .added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags), + .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), }; if (!fdb->dst) return; - if (test_bit(BR_FDB_LOCAL, &fdb->flags)) - return; switch (type) { case RTM_DELNEIGH: diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 995e0e16f295..6e348d2222a9 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2329,7 +2329,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused, fdb_info = ptr; if (dsa_slave_dev_check(dev)) { - if (!fdb_info->added_by_user) + if (!fdb_info->added_by_user || fdb_info->is_local) return NOTIFY_OK; dp = dsa_slave_to_port(dev);