[bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join

[bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join

[Dan Carpenter]

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)

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);
    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

    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.

    2680         return err;
    2681 }

regards,
dan carpenter

Re: [bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join

[Petr Machata]

Dan Carpenter <dan.carpenter@linaro.org> 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