Re: [PATCH net-next v5 14/14] net: renesas: rswitch: add vlan aware switching

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: renesas: rswitch: add vlan aware switching

This patch adds VLAN support to the L2 HW bridge for the Renesas switch.
It implements the standard switchdev callbacks for configuring STP state,
aging time, and VLAN filtering. The implementation works around hardware
limitations by keeping all ports in a default VLAN.

> 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
[ ... ]
> +static void rswitch_update_vlan_filtering(struct rswitch_private *priv,
> +					  bool vlan_filtering)
> +{
> +	if (vlan_filtering)
> +		rswitch_modify(priv->addr, FWPC0(AGENT_INDEX_GWCA),
> +			       0, FWPC0_VLANSA | FWPC0_VLANRU);
> +	else
> +		rswitch_modify(priv->addr, FWPC0(AGENT_INDEX_GWCA),
> +			       FWPC0_VLANSA | FWPC0_VLANRU, 0);
> +}

[Severity: Medium]
Does this properly synchronize the VLAN filtering state to the physical
switch ports? This seems to update the FWPC0 registers only for the CPU
port (AGENT_INDEX_GWCA) without iterating over the external physical
ports, which might leave the hardware in an inconsistent state.

[ ... ]
> +static int rswitch_handle_port_attr_set(struct net_device *ndev,
> +					struct notifier_block *nb,
> +					struct switchdev_notifier_port_attr_info *info)
> +{
[ ... ]
> +	switch (attr->id) {
> +	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
> +		err = rswitch_port_update_stp_state(ndev, attr->u.stp_state);
> +
> +		break;
[ ... ]
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (err < 0)
> +		return err;
> +
> +	info->handled = true;
> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
Could this block switchdev configurations for other network devices on the
system? 

The switchdev notifiers are broadcast globally, and without a check
verifying the target ndev belongs to this driver, the default case here
returns -EOPNOTSUPP for unrelated devices. When passed to
notifier_from_errno(), this stops the global notifier chain.

[ ... ]
> +static int rswitch_gwca_set_vlan_tag(struct rswitch_private *priv,
> +				     struct switchdev_obj_port_vlan *p_vlan,
> +				     bool delete)
> +{
> +	u32 vem_val;
> +	int err;
> +
> +	err = rswitch_gwca_change_mode(priv, GWMC_OPC_CONFIG);

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a self-
deadlock in rswitch_gwca_change_mode() when trying to enable the agent
clock:

drivers/net/ethernet/renesas/rswitch_main.c:rswitch_gwca_change_mode() {
        if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index)) {
                spin_lock(&priv->agent_lock);
                rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 1);
                spin_lock(&priv->agent_lock);
        }
...
}

Will the second spin_lock() cause a deadlock on the same CPU?

> +	if (err < 0)
> +		return err;

[ ... ]
> +static int rswitch_port_obj_do_add_gwca(struct net_device *ndev,
> +					struct rswitch_private *priv,
> +					struct switchdev_obj_port_vlan *p_vlan)
> +{
> +	int err;
> +
> +	if (!(p_vlan->flags & BRIDGE_VLAN_INFO_BRENTRY))
> +		return NOTIFY_DONE;
> +
> +	/* Set Rswitch VLAN mode */
> +	iowrite32(br_vlan_enabled(ndev) ? FIELD_PREP(FWGC_SVM, C_TAG) : 0, priv->addr + FWGC);

[Severity: High]
Is it safe to pass ndev directly to br_vlan_enabled() here? 

Because the notifier runs globally and routes non-rdev devices into this
function, ndev might be an unrelated physical port rather than a bridge
master. This seems like it could lead to out-of-bounds memory reads when
the function dereferences the foreign driver's private data.

[ ... ]
> +static int rswitch_port_obj_do_del(struct net_device *ndev,
> +				   struct switchdev_obj_port_vlan *p_vlan)
> +{
> +	struct rswitch_device *rdev = netdev_priv(ndev);
> +	struct rswitch_private *priv = rdev->priv;
> +	struct rswitch_etha *etha = rdev->etha;
> +	int err;
> +
> +	err = rswitch_port_set_vlan_tag(etha, p_vlan, true);
> +	if (err < 0)
> +		return err;
> +
> +	rswitch_modify(priv->addr, FWPC0(etha->index), FWPC0_VLANSA | FWPC0_VLANRU, 0);
> +	rswitch_modify(priv->addr, FWPC2(AGENT_INDEX_GWCA), 0,
> +		       FIELD_PREP(FWPC2_LTWFW, BIT(etha->index)));
> +	rswitch_modify(priv->addr, FWPC2(rdev->port),
> +		       0, FIELD_PREP(FWPC2_LTWFW, GENMASK(RSWITCH_NUM_AGENTS - 1, 0)));

[Severity: High]
Does this unconditionally break connectivity for all remaining active VLANs
on this port? 

When handling a VLAN deletion event, clearing these forwarding masks happens
regardless of whether the port is still a member of other VLANs.

> +
> +	return rswitch_erase_vlan_table(priv, p_vlan->vid, etha->index);
> +}

[ ... ]
>  static int rswitch_switchdev_blocking_event(struct notifier_block *nb,
> -					    unsigned long event, void *ptr)
> +					    unsigned long event,
> +					    void *ptr)
>  {
>  	struct net_device *ndev = switchdev_notifier_info_to_dev(ptr);
> -	int ret;
>  
>  	switch (event) {
>  	case SWITCHDEV_PORT_OBJ_ADD:
> -		return -EOPNOTSUPP;
> +		err = rswitch_handle_port_obj_add(ndev, nb, ptr);
> +
> +		return notifier_from_errno(err);
>  	case SWITCHDEV_PORT_OBJ_DEL:
> -		return -EOPNOTSUPP;
> +		err = rswitch_handle_port_obj_del(ndev, nb, ptr);
> +
> +		return notifier_from_errno(err);
>  	case SWITCHDEV_PORT_ATTR_SET:
> -		ret = switchdev_handle_port_attr_set(ndev, ptr,
> -						     is_rdev,
> -						     rswitch_port_attr_set);
> -		break;
> -	default:
> -		if (is_rdev(ndev))
> -			return -EOPNOTSUPP;
> -		return NOTIFY_DONE;
> +		err = rswitch_handle_port_attr_set(ndev, nb, ptr);
> +
> +		return notifier_from_errno(err);
>  	}

[Severity: High]
Could removing the is_rdev check from the attribute and object handler
paths cause the driver to wrongly modify its own MMIO registers based on
events meant for other hardware? 

Since switchdev_handle_port_attr_set previously guarded against unrelated
devices, removing it exposes the handlers to global broadcast events.