| Message ID | 20210809131152.509092-1-vladimir.oltean@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: switchdev: zero-initialize struct switchdev_notifier_fdb_info emitted by drivers towards the bridge | expand |
On Mon, Aug 09, 2021 at 04:11:52PM +0300, Vladimir Oltean wrote: > The blamed commit a new field to struct switchdev_notifier_fdb_info, but > did not make sure that all call paths set it to something valid. For > example, a switchdev driver may emit a SWITCHDEV_FDB_ADD_TO_BRIDGE > notifier, and since the 'is_local' flag is not set, it contains junk > from the stack, so the bridge might interpret those notifications as > being for local FDB entries when that was not intended. > > To avoid that now and in the future, zero-initialize all > switchdev_notifier_fdb_info structures created by drivers such that all > newly added fields to not need to touch drivers again. > > Fixes: 2c4eca3ef716 ("net: bridge: switchdev: include local flag in FDB notifications") > Reported-by: Ido Schimmel <idosch@idosch.org> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 ++ > drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c | 1 + > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 ++ > drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 1 + > drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c | 1 + > drivers/net/ethernet/rocker/rocker_main.c | 1 + > drivers/net/ethernet/rocker/rocker_ofdpa.c | 1 + > drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 1 + > drivers/net/ethernet/ti/cpsw_switchdev.c | 1 + > drivers/s390/net/qeth_l2_main.c | 2 ++ > net/dsa/slave.c | 1 + > 11 files changed, 14 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > index 0b3e8f2db294..cf60e80dd3ba 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > @@ -750,6 +750,7 @@ prestera_fdb_offload_notify(struct prestera_port *port, > { > struct switchdev_notifier_fdb_info send_info; > > + memset(&send_info, 0, sizeof(send_info)); This can be written simpler. struct switchdev_notifier_fdb_info send_info = {}; In all places. Thanks > send_info.addr = info->addr; > send_info.vid = info->vid; > send_info.offloaded = true; > @@ -1146,6 +1147,7 @@ static void prestera_fdb_event(struct prestera_switch *sw, > if (!dev) > return; > > + memset(&info, 0, sizeof(info)); > info.addr = evt->fdb_evt.data.mac; > info.vid = evt->fdb_evt.vid; > info.offloaded = true; > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c > index a6e1d4f78268..77e09397a062 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c > @@ -71,6 +71,7 @@ mlx5_esw_bridge_fdb_offload_notify(struct net_device *dev, const unsigned char * > { > struct switchdev_notifier_fdb_info send_info; > > + memset(&send_info, 0, sizeof(send_info)); > send_info.addr = addr; > send_info.vid = vid; > send_info.offloaded = true; > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > index 7e221ef01437..8a7660f2d048 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > @@ -9086,6 +9086,7 @@ static void mlxsw_sp_rif_fid_fdb_del(struct mlxsw_sp_rif *rif, const char *mac) > if (!dev) > return; > > + memset(&info, 0, sizeof(info)); > info.addr = mac; > info.vid = 0; > call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE, dev, &info.info, > @@ -9137,6 +9138,7 @@ static void mlxsw_sp_rif_vlan_fdb_del(struct mlxsw_sp_rif *rif, const char *mac) > if (!dev) > return; > > + memset(&info, 0, sizeof(info)); > info.addr = mac; > info.vid = vid; > call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE, dev, &info.info, > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c > index c5ef9aa64efe..f016d909bead 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c > @@ -2510,6 +2510,7 @@ mlxsw_sp_fdb_call_notifiers(enum switchdev_notifier_type type, > { > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = mac; > info.vid = vid; > info.offloaded = offloaded; > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c > index 0443f66b5550..fbc3f5e65882 100644 > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c > @@ -279,6 +279,7 @@ static void sparx5_fdb_call_notifiers(enum switchdev_notifier_type type, > { > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = mac; > info.vid = vid; > info.offloaded = offloaded; > diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c > index a46633606cae..49d548be9fe4 100644 > --- a/drivers/net/ethernet/rocker/rocker_main.c > +++ b/drivers/net/ethernet/rocker/rocker_main.c > @@ -2717,6 +2717,7 @@ rocker_fdb_offload_notify(struct rocker_port *rocker_port, > { > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = recv_info->addr; > info.vid = recv_info->vid; > info.offloaded = true; > diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c > index 967a634ee9ac..7d954fd24134 100644 > --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c > +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c > @@ -1824,6 +1824,7 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work) > bool learned = (lw->flags & OFDPA_OP_FLAG_LEARNED); > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = lw->addr; > info.vid = lw->vid; > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c > index 9c29b363e9ae..81d2b1765a66 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c > @@ -360,6 +360,7 @@ static void am65_cpsw_fdb_offload_notify(struct net_device *ndev, > { > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = rcv->addr; > info.vid = rcv->vid; > info.offloaded = true; > diff --git a/drivers/net/ethernet/ti/cpsw_switchdev.c b/drivers/net/ethernet/ti/cpsw_switchdev.c > index f7fb6e17dadd..446bdab06bdd 100644 > --- a/drivers/net/ethernet/ti/cpsw_switchdev.c > +++ b/drivers/net/ethernet/ti/cpsw_switchdev.c > @@ -370,6 +370,7 @@ static void cpsw_fdb_offload_notify(struct net_device *ndev, > { > struct switchdev_notifier_fdb_info info; > > + memset(&info, 0, sizeof(info)); > info.addr = rcv->addr; > info.vid = rcv->vid; > info.offloaded = true; > diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c > index 2abf86c104d5..843dd4f4d8d7 100644 > --- a/drivers/s390/net/qeth_l2_main.c > +++ b/drivers/s390/net/qeth_l2_main.c > @@ -283,6 +283,7 @@ static void qeth_l2_dev2br_fdb_flush(struct qeth_card *card) > > QETH_CARD_TEXT(card, 2, "fdbflush"); > > + memset(&info, 0, sizeof(info)); > info.addr = NULL; > /* flush all VLANs: */ > info.vid = 0; > @@ -693,6 +694,7 @@ static void qeth_l2_dev2br_fdb_notify(struct qeth_card *card, u8 code, > if (qeth_is_my_net_if_token(card, token)) > return; > > + memset(&info, 0, sizeof(info)); > info.addr = ntfy_mac; > /* don't report VLAN IDs */ > info.vid = 0; > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 532085da8d8f..1cb7f7e56784 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -2298,6 +2298,7 @@ dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work) > if (!dsa_is_user_port(ds, switchdev_work->port)) > return; > > + memset(&info, 0, sizeof(info)); > info.addr = switchdev_work->addr; > info.vid = switchdev_work->vid; > info.offloaded = true; > -- > 2.25.1 >
Hi Leon, On Tue, Aug 10, 2021 at 09:50:41AM +0300, Leon Romanovsky wrote: > > + memset(&send_info, 0, sizeof(send_info)); > > This can be written simpler. > struct switchdev_notifier_fdb_info send_info = {}; > > In all places. Because the structure contains a sub-structure, I believe that a compound literal initializer would require additional braces for the initialization of its sub-objects too. At least I know that expressions like that have attracted the attention of clang people in the past: https://patchwork.ozlabs.org/project/netdev/patch/20190506202447.30907-1-natechancellor@gmail.com/ So I went for the 'unambiguous' path.
On Tue, Aug 10, 2021 at 08:16:17AM +0000, Vladimir Oltean wrote: > Hi Leon, > > On Tue, Aug 10, 2021 at 09:50:41AM +0300, Leon Romanovsky wrote: > > > + memset(&send_info, 0, sizeof(send_info)); > > > > This can be written simpler. > > struct switchdev_notifier_fdb_info send_info = {}; > > > > In all places. > > Because the structure contains a sub-structure, I believe that a > compound literal initializer would require additional braces for the > initialization of its sub-objects too. At least I know that expressions > like that have attracted the attention of clang people in the past: > https://patchwork.ozlabs.org/project/netdev/patch/20190506202447.30907-1-natechancellor@gmail.com/ > So I went for the 'unambiguous' path. There's a difference between: struct foo bar = { 0 }; and struct foo bar = { }; The former tells the compiler that you wish to set the first member of struct foo, which will be an integer type, to zero. The latter is an empty initialiser where all members and sub-members of the structure default to a zero value. You should have no problem with the latter. You will encounter problems with the former if the first member of struct foo is not an integer type. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Aug 10, 2021 at 10:12:38AM +0100, Russell King - ARM Linux admin wrote: > There's a difference between: > > struct foo bar = { 0 }; > > and > > struct foo bar = { }; > > The former tells the compiler that you wish to set the first member of > struct foo, which will be an integer type, to zero. The latter is an > empty initialiser where all members and sub-members of the structure > default to a zero value. > > You should have no problem with the latter. You will encounter problems > with the former if the first member of struct foo is not an integer > type. Ok, that's good to know. Seeing that this patch has not been applied yet I'll go for a v2.
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c index 0b3e8f2db294..cf60e80dd3ba 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c @@ -750,6 +750,7 @@ prestera_fdb_offload_notify(struct prestera_port *port, { struct switchdev_notifier_fdb_info send_info; + memset(&send_info, 0, sizeof(send_info)); send_info.addr = info->addr; send_info.vid = info->vid; send_info.offloaded = true; @@ -1146,6 +1147,7 @@ static void prestera_fdb_event(struct prestera_switch *sw, if (!dev) return; + memset(&info, 0, sizeof(info)); info.addr = evt->fdb_evt.data.mac; info.vid = evt->fdb_evt.vid; info.offloaded = true; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c index a6e1d4f78268..77e09397a062 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c @@ -71,6 +71,7 @@ mlx5_esw_bridge_fdb_offload_notify(struct net_device *dev, const unsigned char * { struct switchdev_notifier_fdb_info send_info; + memset(&send_info, 0, sizeof(send_info)); send_info.addr = addr; send_info.vid = vid; send_info.offloaded = true; diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c index 7e221ef01437..8a7660f2d048 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c @@ -9086,6 +9086,7 @@ static void mlxsw_sp_rif_fid_fdb_del(struct mlxsw_sp_rif *rif, const char *mac) if (!dev) return; + memset(&info, 0, sizeof(info)); info.addr = mac; info.vid = 0; call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE, dev, &info.info, @@ -9137,6 +9138,7 @@ static void mlxsw_sp_rif_vlan_fdb_del(struct mlxsw_sp_rif *rif, const char *mac) if (!dev) return; + memset(&info, 0, sizeof(info)); info.addr = mac; info.vid = vid; call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE, dev, &info.info, diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c index c5ef9aa64efe..f016d909bead 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c @@ -2510,6 +2510,7 @@ mlxsw_sp_fdb_call_notifiers(enum switchdev_notifier_type type, { struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = mac; info.vid = vid; info.offloaded = offloaded; diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c index 0443f66b5550..fbc3f5e65882 100644 --- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c @@ -279,6 +279,7 @@ static void sparx5_fdb_call_notifiers(enum switchdev_notifier_type type, { struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = mac; info.vid = vid; info.offloaded = offloaded; diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c index a46633606cae..49d548be9fe4 100644 --- a/drivers/net/ethernet/rocker/rocker_main.c +++ b/drivers/net/ethernet/rocker/rocker_main.c @@ -2717,6 +2717,7 @@ rocker_fdb_offload_notify(struct rocker_port *rocker_port, { struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = recv_info->addr; info.vid = recv_info->vid; info.offloaded = true; diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c index 967a634ee9ac..7d954fd24134 100644 --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c @@ -1824,6 +1824,7 @@ static void ofdpa_port_fdb_learn_work(struct work_struct *work) bool learned = (lw->flags & OFDPA_OP_FLAG_LEARNED); struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = lw->addr; info.vid = lw->vid; diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c index 9c29b363e9ae..81d2b1765a66 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c @@ -360,6 +360,7 @@ static void am65_cpsw_fdb_offload_notify(struct net_device *ndev, { struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = rcv->addr; info.vid = rcv->vid; info.offloaded = true; diff --git a/drivers/net/ethernet/ti/cpsw_switchdev.c b/drivers/net/ethernet/ti/cpsw_switchdev.c index f7fb6e17dadd..446bdab06bdd 100644 --- a/drivers/net/ethernet/ti/cpsw_switchdev.c +++ b/drivers/net/ethernet/ti/cpsw_switchdev.c @@ -370,6 +370,7 @@ static void cpsw_fdb_offload_notify(struct net_device *ndev, { struct switchdev_notifier_fdb_info info; + memset(&info, 0, sizeof(info)); info.addr = rcv->addr; info.vid = rcv->vid; info.offloaded = true; diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c index 2abf86c104d5..843dd4f4d8d7 100644 --- a/drivers/s390/net/qeth_l2_main.c +++ b/drivers/s390/net/qeth_l2_main.c @@ -283,6 +283,7 @@ static void qeth_l2_dev2br_fdb_flush(struct qeth_card *card) QETH_CARD_TEXT(card, 2, "fdbflush"); + memset(&info, 0, sizeof(info)); info.addr = NULL; /* flush all VLANs: */ info.vid = 0; @@ -693,6 +694,7 @@ static void qeth_l2_dev2br_fdb_notify(struct qeth_card *card, u8 code, if (qeth_is_my_net_if_token(card, token)) return; + memset(&info, 0, sizeof(info)); info.addr = ntfy_mac; /* don't report VLAN IDs */ info.vid = 0; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 532085da8d8f..1cb7f7e56784 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2298,6 +2298,7 @@ dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work) if (!dsa_is_user_port(ds, switchdev_work->port)) return; + memset(&info, 0, sizeof(info)); info.addr = switchdev_work->addr; info.vid = switchdev_work->vid; info.offloaded = true;
The blamed commit a new field to struct switchdev_notifier_fdb_info, but did not make sure that all call paths set it to something valid. For example, a switchdev driver may emit a SWITCHDEV_FDB_ADD_TO_BRIDGE notifier, and since the 'is_local' flag is not set, it contains junk from the stack, so the bridge might interpret those notifications as being for local FDB entries when that was not intended. To avoid that now and in the future, zero-initialize all switchdev_notifier_fdb_info structures created by drivers such that all newly added fields to not need to touch drivers again. Fixes: 2c4eca3ef716 ("net: bridge: switchdev: include local flag in FDB notifications") Reported-by: Ido Schimmel <idosch@idosch.org> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 ++ drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c | 1 + drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 ++ drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 1 + drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c | 1 + drivers/net/ethernet/rocker/rocker_main.c | 1 + drivers/net/ethernet/rocker/rocker_ofdpa.c | 1 + drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 1 + drivers/net/ethernet/ti/cpsw_switchdev.c | 1 + drivers/s390/net/qeth_l2_main.c | 2 ++ net/dsa/slave.c | 1 + 11 files changed, 14 insertions(+)