Dan Carpenter <dan.carpenter@xxxxxxxxxx> writes:
> Hello Petr Machata,
>
> The patch ec4643ca3d98: "mlxsw: spectrum_switchdev: Replay switchdev
> objects on port join" from Jul 19, 2023 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:2679 mlxsw_sp_bridge_8021d_port_join()
> error: we previously assumed 'mlxsw_sp_port_vlan->fid' could be null (see line 2664)
Thanks for the report!
> drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> 2642 static int
> 2643 mlxsw_sp_bridge_8021d_port_join(struct mlxsw_sp_bridge_device *bridge_device,
> 2644 struct mlxsw_sp_bridge_port *bridge_port,
> 2645 struct mlxsw_sp_port *mlxsw_sp_port,
> 2646 struct netlink_ext_ack *extack)
> 2647 {
> 2648 struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan;
> 2649 struct net_device *dev = bridge_port->dev;
> 2650 u16 vid;
> 2651 int err;
> 2652
> 2653 vid = is_vlan_dev(dev) ? vlan_dev_vlan_id(dev) : MLXSW_SP_DEFAULT_VID;
> 2654 mlxsw_sp_port_vlan = mlxsw_sp_port_vlan_find_by_vid(mlxsw_sp_port, vid);
> 2655 if (WARN_ON(!mlxsw_sp_port_vlan))
> 2656 return -EINVAL;
> 2657
> 2658 if (mlxsw_sp_port_is_br_member(mlxsw_sp_port, bridge_device->dev)) {
> 2659 NL_SET_ERR_MSG_MOD(extack, "Can not bridge VLAN uppers of the same port");
> 2660 return -EINVAL;
> 2661 }
> 2662
> 2663 /* Port is no longer usable as a router interface */
> 2664 if (mlxsw_sp_port_vlan->fid)
>
> This has a check for NULL.
>
> 2665 mlxsw_sp_port_vlan_router_leave(mlxsw_sp_port_vlan);
The function mlxsw_sp_bridge_8021d_port_join() deals only with
VLAN-unaware bridges. It is invoked in response to netlink changeupper
notifications.
If there is a FID in that situation, it cannot be a VLAN FID, it must be
a router FID. So leave the router. So as of line 2666,
mlxsw_sp_port_vlan->fid is NULL.
> 2666
> 2667 err = mlxsw_sp_port_vlan_bridge_join(mlxsw_sp_port_vlan, bridge_port,
> 2668 extack);
>
> Most of the time this would set ->fid but if mlxsw_sp_port_vlan->bridge_port
> then it wouldn't
mlxsw_sp_port_vlan->bridge_port is only ever set in one place, later in
mlxsw_sp_port_vlan_bridge_join(). So if it is set, we have been through
that function once already. In that case, mlxsw_sp_port_vlan->fid is
already non-NULL and nothing needs to be done.
This is only relevant for 802.1q bridges, when PVID / egress_untagged
flags change on a VLAN. Here we deal with 802.1d bridges, where it ought
not to happen.
mlxsw_sp_port_vlan_bridge_join() then constructs the proper 802.1d FID.
At line 2671, mlxsw_sp_port_vlan->fid is non-NULL.
>
> 2669 if (err)
> 2670 return err;
> 2671
> 2672 err = mlxsw_sp_bridge_port_replay(bridge_port, mlxsw_sp_port, extack);
> 2673 if (err)
> 2674 goto err_replay;
> 2675
> 2676 return 0;
> 2677
> 2678 err_replay:
> --> 2679 mlxsw_sp_port_vlan_bridge_leave(mlxsw_sp_port_vlan);
>
> ->fid dereferenced without being checked.
And we rely on that here.
>
> 2680 return err;
> 2681 }
>
> regards,
> dan carpenter
