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

[Michael Dege]

Hello Jakub,

Thank you for your review.

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Wednesday, May 13, 2026 3:36 AM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; niklas.soderlund
> <niklas.soderlund@xxxxxxxxxxxx>; paul@xxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-renesas-
> soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v4 12/13] net: renesas: rswitch: add handler for FDB notification
> 
> 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().
> 

No, it was not safe and it actually threw warnings. Fortunately, I was able to find an example how
this was solved in a different driver. Now it is fixed and working.

> [ ... ]
> > 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?

Since bridge ports are not overwritten, but need to be removed and rewritten, the bits are set, when 
the port is removed from the bridge.

> 
> [ ... ]
> > +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?

I changed the logic, now this should not happen anymore.
> 
> > +
> > +	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?

No, this does not happen. I tested this and the MAC is not deleted, when a single IF is removed 
>From the bridge.

> 
> [ ... ]
> > +		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?

I added flushing of the work item from the WQ when the driver is unloaded.

Best regards,

Michael