Re: [PATCH net-next v3 5/6] net: xilinx: axienet: Dispatch statistics through axienet_config ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array write in `axienet_1g_get_ethtool_stats()` due to mismatched statistics array indices.
- [High] Reading a `seqcount_mutex_t` from interrupt context can cause a deadlock if the writer is preempted.
--

commit a22a50757854e007ab76536560cb1108e8d22fd4
Author: Suraj Gupta <suraj.gupta2@amd.com>

net: xilinx: axienet: Dispatch statistics through axienet_config ops

This commit refactors the AXI Ethernet driver's statistics reporting by
dispatching stats reads through operation pointers in struct axienet_config.
This prepares the driver to support different MAC types without inline
branches in the statistics callbacks.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 212816613aa86..8100cc53cd643 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1878,29 +1878,11 @@ static int axienet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	return phylink_mii_ioctl(lp->phylink, rq, cmd);
>  }
>  
> -static void
> -axienet_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> +static void axienet_1g_get_stats64(struct axienet_local *lp,
> +				   struct rtnl_link_stats64 *stats)
>  {
> -	struct axienet_local *lp = netdev_priv(dev);
>  	unsigned int start;
>  
> -	netdev_stats_to_stats64(stats, &dev->stats);
> -
> -	do {
> -		start = u64_stats_fetch_begin(&lp->rx_stat_sync);
> -		stats->rx_packets = u64_stats_read(&lp->rx_packets);
> -		stats->rx_bytes = u64_stats_read(&lp->rx_bytes);
> -	} while (u64_stats_fetch_retry(&lp->rx_stat_sync, start));
> -
> -	do {
> -		start = u64_stats_fetch_begin(&lp->tx_stat_sync);
> -		stats->tx_packets = u64_stats_read(&lp->tx_packets);
> -		stats->tx_bytes = u64_stats_read(&lp->tx_bytes);
> -	} while (u64_stats_fetch_retry(&lp->tx_stat_sync, start));
> -
> -	if (!(lp->features & XAE_FEATURE_STATS))
> -		return;
> -
>  	do {
>  		start = read_seqcount_begin(&lp->hw_stats_seqcount);

[Severity: High]
This is a pre-existing issue, but is it safe to read this seqcount_mutex_t
from this context?

The hw_stats_seqcount writer runs in process context (a delayed work) holding
a mutex, meaning it can be preempted by softirqs. The reader here in
axienet_1g_get_stats64() is called via ndo_get_stats64, which can be invoked
from atomic or softirq context (such as via dev_get_stats() in the Open
vSwitch datapath).

If a softirq fires on the same CPU while the writer is inside the seqcount
write section, won't the reader spin infinitely in read_seqcount_begin()
waiting for the odd sequence number to become even, resulting in a hard
deadlock?

[ ... ]
> -static void axienet_ethtools_get_ethtool_stats(struct net_device *dev,
> -					       struct ethtool_stats *stats,
> -					       u64 *data)
> +static void axienet_1g_get_ethtool_stats(struct axienet_local *lp, u64 *data)
>  {
> -	struct axienet_local *lp = netdev_priv(dev);
>  	unsigned int start;
>  
>  	do {
> @@ -2381,6 +2387,17 @@ static void axienet_ethtools_get_ethtool_stats(struct net_device *dev,

[Severity: High]
This isn't a bug introduced by this patch, but does this function overflow the
data array allocated by the ethtool core?

When a user invokes ethtool -S, the core allocates a buffer for 9 elements
based on the return value of axienet_1g_get_sset_count(). However, inside
the seqcount loop in axienet_1g_get_ethtool_stats(), the code writes beyond
these bounds:

		data[8] = axienet_stat(lp, STAT_USER_DEFINED0);
		data[9] = axienet_stat(lp, STAT_USER_DEFINED1);
		data[10] = axienet_stat(lp, STAT_USER_DEFINED2);

Can this overrun the 72-byte allocated buffer by 16 bytes and cause memory
corruption?

[ ... ]
>  static const char axienet_ethtool_stats_strings[][ETH_GSTRING_LEN] = {
>  	"Received bytes",
>  	"Transmitted bytes",
> @@ -2393,12 +2410,26 @@ static const char axienet_ethtool_stats_strings[][ETH_GSTRING_LEN] = {
>  	"User Defined Counter 2",
>  };
>  
> +static void axienet_1g_get_strings(u8 *data)
> +{
> +	memcpy(data, axienet_ethtool_stats_strings,
> +	       sizeof(axienet_ethtool_stats_strings));
> +}
> +
> +static int axienet_1g_get_sset_count(void)
> +{
> +	return ARRAY_SIZE(axienet_ethtool_stats_strings);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150816.1020883-1-suraj.gupta2@amd.com?part=5