Re: [PATCH net-next v6 12/12] net: airoha: add phylink support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `phylink_destroy()` before `unregister_netdev()` causes a Use-After-Free during device teardown.
- [High] Unconditional PCS parsing in `airoha_setup_phylink()` causes probe failure for the GDM1 port.
- [Medium] The RX fragment size is incorrectly configured using the TX fragment size variable due to a copy-paste error.
- [Medium] The `phylink` instance is leaked in the `airoha_probe()` error path for successfully allocated but unregistered devices.
--

commit 7158f1ddaf486067bb751bd3d43f1a8bfc87794f
Author: Christian Marangi <ansuelsmth@gmail.com>

    net: airoha: add phylink support
    
    Add phylink support for each GDM port. For GDM1 add the internal interface
    mode as the only supported mode. For GDM2/3/4 add the required
    configuration of the PCS to make the external PHY or attached SFP cage
    work.
    
    These needs to be defined in the GDM port node using the pcs-handle
    property.

[ ... ]

> @@ -3148,6 +3161,153 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
>  	return false;
>  }
>  
> +/* Nothing to do in MAC, everything is handled in PCS */
> +static void airoha_mac_config(struct phylink_config *config, unsigned int mode,
> +			      const struct phylink_link_state *state)
> +{
> +}
> +
> +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex, bool tx_pause, bool rx_pause)
> +{

[ ... ]

> +	if (dev->nbq == 1) {
> +		mask = GDMA4_SGMII1_RX_FRAG_SIZE_MASK;
> +		val = FIELD_PREP(GDMA4_SGMII1_RX_FRAG_SIZE_MASK,
> +				 frag_size_tx);
> +	} else {
> +		mask = GDMA4_SGMII0_RX_FRAG_SIZE_MASK;
> +		val = FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> +				 frag_size_tx);
> +	}

[Severity: Medium]
Should this use frag_size_rx instead of frag_size_tx when configuring the
RX fragment size? This looks like a typo that could cause the hardware to be
configured incorrectly when the RX and TX fragment sizes differ.

> +	airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG, mask, val);
> +}

[ ... ]

> +static int airoha_setup_phylink(struct net_device *netdev)
> +{
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct device_node *np = netdev->dev.of_node;
> +	struct airoha_gdm_port *port = dev->port;
> +	struct phylink_config *config;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
> +	int err;
> +
> +	err = of_get_phy_mode(np, &phy_mode);
> +	if (err) {
> +		dev_err(&netdev->dev, "incorrect phy-mode\n");
> +		return err;
> +	}
> +
> +	config = &dev->phylink_config;
> +
> +	config->dev = &netdev->dev;
> +	config->type = PHYLINK_NETDEV;
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |  MAC_10000FD;
> +	if (port->id > AIROHA_GDM1_IDX)
> +		config->mac_capabilities |= MAC_10 | MAC_100 | MAC_1000 |
> +					    MAC_2500FD | MAC_5000FD;
> +
> +	err = fwnode_phylink_pcs_parse(dev_fwnode(&netdev->dev), NULL,
> +				       &config->num_available_pcs);
> +	if (err)
> +		return err;

[Severity: High]
Does this unconditional call to fwnode_phylink_pcs_parse() cause probe to
fail for the GDM1 port?

The commit message mentions that GDM1 does not require a PCS. If its device
tree node lacks a pcs-handle property, fwnode_phylink_pcs_parse() will return
-ENODEV and abort the probe. Should this parsing logic be conditionally
skipped if port->id == AIROHA_GDM1_IDX?

> +
> +	config->fill_available_pcs = airoha_fill_available_pcs;
> +
> +	/*
> +	 * GDM1 only supports internal for Embedded Switch
> +	 * and doesn't require a PCS.
> +	 */

[ ... ]

> @@ -3435,8 +3595,10 @@ static int airoha_probe(struct platform_device *pdev)
>  				continue;
>  
>  			netdev = netdev_from_priv(dev);
> -			if (netdev->reg_state == NETREG_REGISTERED)
> +			if (netdev->reg_state == NETREG_REGISTERED) {
> +				phylink_destroy(dev->phylink);
>  				unregister_netdev(netdev);
> +			}

[Severity: Medium]
Does this leak the phylink instance for devices that were successfully
allocated but failed to register?

In airoha_alloc_gdm_device(), the phylink structure is successfully allocated
prior to device registration. If airoha_probe() fails before or during device
registration, the NETREG_REGISTERED check here skips calling
phylink_destroy() for unregistered devices, which would leave the memory
leaked.


[Severity: High]
Is it safe to destroy the phylink instance before calling
unregister_netdev()?

If the network interface is running (IFF_UP), unregister_netdev() will
synchronously bring the device down by invoking its ndo_stop callback, which
is airoha_dev_stop().

Inside airoha_dev_stop(), the driver attempts to halt phylink:
	phylink_stop(dev->phylink);
	phylink_disconnect_phy(dev->phylink);

Will this result in a use-after-free since dev->phylink was already destroyed
here?

>  			of_node_put(netdev->dev.of_node);
>  		}
>  		airoha_metadata_dst_free(port);

[ ... ]

> @@ -3472,6 +3634,7 @@ static void airoha_remove(struct platform_device *pdev)
>  				continue;
>  
>  			netdev = netdev_from_priv(dev);
> +			phylink_destroy(dev->phylink);
>  			unregister_netdev(netdev);

[Severity: High]
Similarly to the error path in airoha_probe(), does destroying the phylink
instance before calling unregister_netdev() cause a use-after-free when
airoha_dev_stop() tries to use dev->phylink?

>  			of_node_put(netdev->dev.of_node);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609151212.29469-1-ansuelsmth@gmail.com?part=12