Re: [PATCH net-next v20 10/10] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Missing mutex unlock on error paths permanently deadlocks `priv->reg_mutex`.
- [High] Kconfig dependency mismatch selects the wrong DSA tag driver, breaking switch probe.
- [High] Inverted logic for BR_LEARNING disables hardware MAC address learning when requested to be enabled.
- [High] Array index of `-1` for `mib->field_high` accesses the `regmap` pointer, causing a type confusion kernel panic.
- [High] Kernel panics due to NULL pointer dereferences from missing/uninitialized regmap fields.
- [High] Unprotected lockless access to `dev->phydev` of other ports causes Use-After-Free.
- [Medium] Duplication of standard kernel statistics in private `ethtool -S` counters.
- [Medium] Swapped TX and RX histograms in RMON stats mappings.
- [Low] `an8855_get_sset_count` incorrectly returns 0 instead of -EOPNOTSUPP for unsupported string sets.
--

--- Patch [10]: [PATCH net-next v20 10/10] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver ---
commit 2771206895bf8d2182821a844b4664df5776fc10
Author: Christian Marangi <ansuelsmth@gmail.com>

net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

This commit introduces the driver for the Airoha AN8855 5-Port Gigabit
DSA switch. It adds configuration for MAC and PHY settings, ethtool
statistics, bridging, and VLAN capabilities.

> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -24,6 +24,15 @@ config NET_DSA_LOOP
[ ... ]
> +config NET_DSA_AN8855
> +	tristate "Airoha AN8855 Ethernet switch support"
> +	depends on MFD_AIROHA_AN8855 || COMPILE_TEST
> +	depends on NET_DSA
> +	select NET_DSA_TAG_MTK

[Severity: High]
Does this config mistakenly select the MTK tagger instead of the AIROHA tagger?

In an8855_get_tag_protocol(), the driver returns DSA_TAG_PROTO_AIROHA. If
NET_DSA_TAG_AIROHA is not independently selected, the required Airoha tagger
won't be compiled, which will cause the switch initialization to abort.

> diff --git a/drivers/net/dsa/an8855.c b/drivers/net/dsa/an8855.c
> --- /dev/null
> +++ b/drivers/net/dsa/an8855.c
> @@ -0,0 +1,1716 @@
[ ... ]
> +static const struct mt7530_reg_field an8855_fields[] = {
> +	{ MT7530_BC_FFP, REG_FIELD(AN8855_BCF, 0, 7) },
[ ... ]
> +	{ MT7530_PSC_SA_DIR, REG_FIELD_ID(AN8855_PSC, 4, 4, AN8855_NUM_PORTS, 0x200) },

[Severity: High]
Does this inverted logic disable hardware MAC address learning when it should
be enabled?

The MT7530_PSC_SA_DIR field is mapped to AN8855_SA_DIS (Source Address
Disable, bit 4). When mt7530_lib_port_bridge_flags() processes the BR_LEARNING
flag, it writes a 1 to this field to enable learning, which instead sets the
disable bit and turns the feature off.

[ ... ]
> +	{ MT7530_MIB_TX_PKT_SZ_512_TO_1023, REG_FIELD_ID(AN8855_PORT_MIB_TX_PKT_SZ_512_TO_1023, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1024_TO_1518, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_1024_TO_1518, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1519_TO_MAX, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_1519_TO_MAX, 0, 32, AN8855_NUM_PORTS, 0x200) },

[Severity: High]
Could the omission of several expected mapping IDs in this array lead to
kernel panics?

The array appears to omit MT7530_MIRROR_EN, MT7530_MIRROR_PORT, and
AN8855_MIB_TX_OVERSIZE_DROP. When mt7530_lib_port_mirror_add() or
an8855_get_ethtool_stats() execute, they will unconditionally read from these
uninitialized pointers, resulting in a NULL pointer dereference.

[ ... ]
> +static const struct mt7530_mib_desc an8855_mib[] = {
> +	MIB_DESC(MT7530_MIB_TX_DROP, -1, "TxDrop"),
> +	MIB_DESC(MT7530_MIB_TX_CRC_ERR, -1, "TxCrcErr"),
> +	MIB_DESC(MT7530_MIB_TX_COLLISION, -1, "TxCollision"),

[Severity: Medium]
Does this array add standardized UAPI counters to the private ethtool
statistics?

Standard kernel subsystem rules mandate using the standard UAPI (such as
rtnl_link_stats64 or ethtool_eth_mac_stats) rather than ethtool -S for widely
standardized counters like TxDrop and TxCollision.

[ ... ]
> +static void an8855_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	int ret;
> +
> +	mutex_lock(&priv->reg_mutex);
> +
> +	/* Set to clean Dynamic entry */
> +	ret = regmap_write(priv->regmap, AN8855_ATA2, AN8855_ATA2_TYPE);
> +	if (ret) {
> +		dev_err(priv->ds->dev, "failed to update ATA2 reg: %d\n", ret);
> +		return;
> +	}
> +
> +	/* Set Port */
> +	ret = regmap_write(priv->regmap, AN8855_ATWD2,
> +			   FIELD_PREP(AN8855_ATWD2_PORT, BIT(port)));
> +	if (ret) {
> +		dev_err(priv->ds->dev, "failed to update ATWD2 reg: %d\n", ret);
> +		return;
> +	}

[Severity: High]
Can these error paths cause a permanent deadlock on priv->reg_mutex?

If regmap_write() fails, the function returns directly without executing
mutex_unlock(&priv->reg_mutex), deadlocking future driver operations.

[ ... ]
> +static int an8855_get_sset_count(struct dsa_switch *ds, int port,
> +				 int sset)
> +{
> +	if (sset != ETH_SS_STATS)
> +		return 0;
> +
> +	return ARRAY_SIZE(an8855_mib);
> +}

[Severity: Low]
Should this return -EOPNOTSUPP instead of 0 for unsupported string sets?

Standard kernel API rules require returning -EOPNOTSUPP for unsupported
ethtool sets to prevent userspace tools from misinterpreting the capability
as existing but having 0 elements.

[ ... ]
> +static void an8855_get_ethtool_stats(struct dsa_switch *ds, int port,
> +				     uint64_t *data)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	const struct mt7530_mib_desc *mib;
> +	struct mt7530_lib_priv *lib_priv;
> +	int i;
> +
> +	lib_priv = &priv->lib_priv;
> +	for (i = 0; i < ARRAY_SIZE(an8855_mib); i++) {
> +		mib = &an8855_mib[i];
> +
> +		mt7530_lib_read_port_stats(lib_priv->fields[mib->field_low],
> +					   lib_priv->fields[mib->field_high],
> +					   port, data + i);
> +	}
> +}

[Severity: High]
Will passing an array index of -1 for mib->field_high cause a type confusion
kernel panic?

Several MIB definitions pass -1 for field_high. Since the fields array
follows the regmap pointer in struct mt7530_lib_priv, an index of -1
evaluates to the address of the regmap pointer. This disguised pointer will
then be passed to regmap_fields_read(), causing a crash.

[ ... ]
> +static const struct mt7530_rmon_hist an8855_rmon_hist_info = {
> +	.ranges_fields = (unsigned int []){
> +		MT7530_MIB_TX_PKT_SZ_64,
> +		MT7530_MIB_TX_PKT_SZ_65_TO_127,
> +		MT7530_MIB_TX_PKT_SZ_128_TO_255,
> +		MT7530_MIB_TX_PKT_SZ_256_TO_511,
> +		MT7530_MIB_TX_PKT_SZ_512_TO_1023,
> +		AN8855_MIB_RX_PKT_SZ_1024_TO_1518,
> +		AN8855_MIB_RX_PKT_SZ_1519_TO_MAX,
> +	},
> +	.ranges_tx_fields = (unsigned int []){
> +		MT7530_MIB_RX_PKT_SZ_64,
> +		MT7530_MIB_RX_PKT_SZ_65_TO_127,
> +		MT7530_MIB_RX_PKT_SZ_128_TO_255,
> +		MT7530_MIB_RX_PKT_SZ_256_TO_511,
> +		MT7530_MIB_RX_PKT_SZ_512_TO_1023,
> +		AN8855_MIB_RX_PKT_SZ_1024_TO_1518,
> +		AN8855_MIB_RX_PKT_SZ_1519_TO_MAX,
> +	},

[Severity: Medium]
Are the TX and RX histograms swapped in these RMON stats mappings?

The ranges_fields array (intended for RX histograms) is populated with
TX_PKT_SZ register references, while ranges_tx_fields (intended for TX
histograms) is populated with RX_PKT_SZ references.

[ ... ]
> +static void an8855_phylink_mac_link_up(struct phylink_config *config,
> +				       struct phy_device *phydev, unsigned int mode,
> +				       phy_interface_t interface, int speed,
> +				       int duplex, bool tx_pause, bool rx_pause)
> +{
[ ... ]
> +	} else {
> +		reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK;
> +
> +		reg &= ~AN8855_PMCR_FORCE_SPEED;
> +		switch (speed) {
> +		case SPEED_10:
> +			reg |= AN8855_PMCR_FORCE_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			reg |= AN8855_PMCR_FORCE_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			reg |= AN8855_PMCR_FORCE_SPEED_1000;
> +			break;
> +		case SPEED_2500:
> +			reg |= AN8855_PMCR_FORCE_SPEED_2500;
> +			break;
> +		case SPEED_5000:
> +			dev_err(priv->ds->dev, "Missing support for 5G speed. Aborting...\n");
> +			return;
> +		}

[Severity: High]
Does this error path leave priv->reg_mutex permanently locked?

Returning directly in the SPEED_5000 case skips the mutex_unlock() call,
which will deadlock any future driver operations requiring this mutex.

[ ... ]
> +static int an8855_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> +					    u32 timer, bool tx_clock_stop)
> +{
[ ... ]
> +	if (timer) {
> +		dsa_switch_for_each_user_port(other_dp, ds) {
> +			struct phy_device *phydev;
> +			struct net_device *dev;
> +
> +			if (other_dp == dp)
> +				continue;
> +
> +			dev = other_dp->user;
> +			if (!dev)
> +				continue;
> +
> +			phydev = dev->phydev;
> +			if (!phydev)
> +				continue;
> +
> +			if (timer < phydev->eee_cfg.tx_lpi_timer)
> +				timer = phydev->eee_cfg.tx_lpi_timer;
> +		}

[Severity: High]
Can this lockless access to another port's dev->phydev cause a
use-after-free?

Because this function runs without holding the rtnl_lock or any lock
protecting other ports, it is possible for another interface to concurrently
be brought down. If that happens, phy_disconnect() could clear and free that
phydev under rtnl_lock, resulting in this loop accessing freed memory.

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