* [bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join
@ 2023-07-25 5:24 Dan Carpenter
2023-07-25 12:43 ` Petr Machata
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-07-25 5:24 UTC (permalink / raw)
To: petrm; +Cc: kernel-janitors
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
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join
2023-07-25 5:24 [bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join Dan Carpenter
@ 2023-07-25 12:43 ` Petr Machata
0 siblings, 0 replies; 2+ messages in thread
From: Petr Machata @ 2023-07-25 12:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: petrm, kernel-janitors, NBU-mlxsw
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-07-25 15:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-25 5:24 [bug report] mlxsw: spectrum_switchdev: Replay switchdev objects on port join Dan Carpenter
2023-07-25 12:43 ` Petr Machata
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.