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

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 9/1/2026 8:39 PM, sashiko-bot@kernel.org wrote:
> 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.
> --
> 


Both are pre-existing issues, I'll address them as part of separate series.

Regards,
Suraj

> 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);
>> +}
>