Re: [PATCH net-next v4 12/13] net: renesas: rswitch: add handler for FDB notification

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: renesas: rswitch: add handler for FDB notification

The SWITCHDEV_FDB_ADD_TO_DEVICE notification writes the bridge MAC address
to the GWCA and adds a static rule to the MAC table.
The SWITCHDEV_FDB_DEL_TO_DEVICE removes the static rule from the MAC table.

> diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h
> @@ -1213,11 +1213,20 @@ struct rswitch_private {
>  	bool etha_no_runtime_change;
>  	bool gwca_halt;
>  	struct net_device *offload_brdev;
> +	netdevice_tracker dev_tracker;

Is it safe to use a single shared dev_tracker for dynamically allocated
asynchronous work items? 

Because system_long_wq queues works asynchronously, multiple FDB events might
overwrite this single tracker state before they execute, leaking the previous
ref_tracker allocations and potentially causing a double-free on netdev_put().

[ ... ]
> diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c b/drivers/net/ethernet/renesas/rswitch_l2.c
> --- a/drivers/net/ethernet/renesas/rswitch_l2.c
> +++ b/drivers/net/ethernet/renesas/rswitch_l2.c
> @@ -82,8 +83,9 @@ static void rswitch_update_l2_hw_forwarding(struct rswitch_private *priv)
>  			 *
>  			 * Do not allow L2 forwarding to self for hw port.
>  			 */
> -			iowrite32(FIELD_PREP(FWPC2_LTWFW, fwd_mask | BIT(rdev->port)),
> -				  priv->addr + FWPC2(rdev->port));
> +			rswitch_modify(priv->addr, FWPC2(rdev->port),
> +				       FIELD_PREP(FWPC2_LTWFW, ~(fwd_mask | BIT(rdev->port))),
> +				       0);

Since rswitch_modify() performs (val & ~clear) | set, and set is passed as
0, this operation can only clear bits. Can it ever set bits back to 1 to
block a port again?

[ ... ]
> +static int rswitch_gwca_write_mac_address(struct rswitch_private *priv, const u8 *mac)
> +{
> +	int err;
> +
> +	err = rswitch_gwca_change_mode(priv, GWMC_OPC_CONFIG);

Since rswitch_gwca_change_mode() transitions to GWMC_OPC_DISABLE
asynchronously from a workqueue without stopping transmit queues or NAPI,
can this race with the active datapath and corrupt the DMA descriptor rings?

[ ... ]
> +static void rswitch_switchdev_bridge_fdb_event_work(struct work_struct *work)
> +{
> +	struct rswitch_switchdev_event_work *switchdev_work;
> +	const unsigned char addr[] = {0, 0, 0, 0, 0, 0};
> +	struct rswitch_device *rdev;
> +	struct net_device *ndev;
> +
> +	switchdev_work = container_of(work, struct rswitch_switchdev_event_work, work);
> +	ndev = switchdev_work->ndev;
> +
> +	rtnl_lock();
> +
> +	/* Unfortunately all net_device members point to br0, there is no simple way to check
> +	 * if the event was triggered by a port device setting.
> +	 */
> +	rswitch_for_all_ports(switchdev_work->priv, rdev) {
> +		if (ether_addr_equal(rdev->ndev->dev_addr, switchdev_work->fdb_info.addr))
> +			goto out;
> +	}
> +
> +	/* Handle only bridge device */
> +	if (is_rdev(ndev))
> +		goto out;

If the event originates from a completely unrelated device in the system
(for example, a different ethernet or wifi interface), will is_rdev()
evaluate to false and cause this unrelated MAC to be erroneously programmed
into the rswitch hardware?

> +
> +	switch (switchdev_work->event) {
> +	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> +		rswitch_gwca_write_mac_address(switchdev_work->priv, switchdev_work->fdb_info.addr);
> +		rswitch_add_addr_to_mactable(switchdev_work->priv, switchdev_work->fdb_info.addr);
> +		break;
> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> +		rswitch_del_addr_from_mactable(switchdev_work->priv, switchdev_work->fdb_info.addr);
> +		rswitch_gwca_write_mac_address(switchdev_work->priv, addr);

Does deleting any single FDB entry on the bridge wipe out the switch's
CPU MAC address since rswitch_gwca_write_mac_address() is called with a
zeroed MAC?

[ ... ]
> +		INIT_WORK(&switchdev_work->work, rswitch_switchdev_bridge_fdb_event_work);
> +
> +		memcpy(&switchdev_work->fdb_info, ptr, sizeof(switchdev_work->fdb_info));
> +
> +		switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> +		if (!switchdev_work->fdb_info.addr)
> +			goto err_addr_alloc;
> +
> +		ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> +				fdb_info->addr);
> +		netdev_hold(ndev, &priv->dev_tracker, GFP_ATOMIC);
> +		queue_work(system_long_wq, &switchdev_work->work);

Is there a mechanism to flush or cancel these untracked work items on module
unload? If the driver unloads while work is pending on system_long_wq, could
it cause a use-after-free when the worker eventually executes and accesses
switchdev_work->priv?