Re: [PATCH v1 iwl-net] iavf: fix out-of-bounds writes in iavf_get_ethtool_stats()

[Simon Horman <horms@kernel.org>]

On Mon, Feb 16, 2026 at 02:44:57PM +0000, Kohei Enju wrote:
> On Mon, 16 Feb 2026 13:19:09 +0000, Simon Horman wrote:
> 
> > > @@ -345,19 +344,19 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
> > >  	iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
> > >  
> > >  	rcu_read_lock();
> > > -	/* As num_active_queues describe both tx and rx queues, we can use
> > > -	 * it to iterate over rings' stats.
> > > +	/* Use num_tx_queues to report stats for the maximum number of queues.
> > > +	 * Queues beyond num_active_queues will report zero.
> > >  	 */
> > > -	for (i = 0; i < adapter->num_active_queues; i++) {
> > > -		struct iavf_ring *ring;
> > > +	for (i = 0; i < netdev->num_tx_queues; i++) {
> > > +		struct iavf_ring *tx_ring = NULL, *rx_ring = NULL;
> > >  
> > > -		/* Tx rings stats */
> > > -		ring = &adapter->tx_rings[i];
> > > -		iavf_add_queue_stats(&data, ring);
> > > +		if (i < adapter->num_active_queues) {
> > 
> > Hi Enju-san,
> 
> Hi Horman-san, thank you for reviewing!
> 
> > 
> > If I understand things correctly, in the scenario described in the patch
> > description, num_active_queues will be 8 here.
> 
> Yes.
> 
> > 
> > Won't that result in an overflow?
> 
> I think it won't overflow.
> 
> In Thread 1, iavf_set_channels(), which allocates {tx,rx}_rings and
> updates num_active_queues, is executed under netdev lock. Therefore
> Thread 3, which is also executed under the netdev lock, sees updated
> num_active_queues and {tx,rx}_rings.
> 
> The scenario flow lacked netdev_(un)lock, my bad.
> 
> Thread 1 (ethtool -L)       Thread 2 (work)        Thread 3 (ethtool -S)
> netdev_lock()
> iavf_set_channels()
> ...
> iavf_alloc_queues()
> -> alloc {tx,rx}_rings
> -> num_active_queues = 8
> iavf_schedule_finish_config()
> netdev_unlock()
>                                                    netdev_lock()
>                                                    iavf_get_sset_count()
>                                                    real_num_tx_queues: 1
>                                                    -> buffer for 1 queue
>                                                    iavf_get_ethtool_stats()
>                                                    num_active_queues: 8
>                                                    -> out-of-bounds!
>                                                    netdev_unlock()
>                             iavf_finish_config()
>                             netdev_lock()
>                             -> real_num_tx_queues = 8
>                             netdev_unlock()

Thanks, and sorry for missing that the first time around.
With that clarified in my mind this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>