On Wed, 18 Feb 2026 06:39:43 +0100, Przemek Kitszel wrote:
> On 2/14/26 20:14, Kohei Enju wrote:
> > iavf incorrectly uses real_num_tx_queues for ETH_SS_STATS. Since the
> > value could change in runtime, we should use num_tx_queues instead.
> >
>
> [...]
>
> > Use immutable num_tx_queues in all related functions to avoid the issue.
> >
> > [1]
> > BUG: KASAN: vmalloc-out-of-bounds in iavf_add_one_ethtool_stat+0x200/0x270
>
> [...]
>
> Thank you very much Enju-san,
> this is a nice improvement!
> Reviewed-by: Przemek Kitszel <[email protected]>
>
> Side note: I'm working on enabling 256 queues for iavf, what means
Cool, and thank you for reviewing!
> changing the reported max number of queues, what finally means screens
> of zero stats printed, unfortunate, but better be correct that pretty ;)
Indeed.
>
> >
> > Fixes: 64430f70ba6f ("iavf: Fix displaying queue statistics shown by
> > ethtool")
> > Signed-off-by: Kohei Enju <[email protected]>
> > ---
> > .../net/ethernet/intel/iavf/iavf_ethtool.c | 31 +++++++++----------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > index 6ff3842a1ff1..98bec3afc200 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > @@ -313,14 +313,13 @@ static int iavf_get_sset_count(struct net_device
> > *netdev, int sset)
> > {
> > /* Report the maximum number queues, even if not every queue is
> > * currently configured. Since allocation of queues is in pairs,
> > - * use netdev->real_num_tx_queues * 2. The real_num_tx_queues is set
> > - * at device creation and never changes.
> > + * use netdev->num_tx_queues * 2. The num_tx_queues is set at
> > + * device creation and never changes.
> > */
> >
> > if (sset == ETH_SS_STATS)
> > return IAVF_STATS_LEN +
> > - (IAVF_QUEUE_STATS_LEN * 2 *
> > - netdev->real_num_tx_queues);
> > + (IAVF_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues);
> > else
> > return -EINVAL;
> > }
> > @@ -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.
>
> <rambling>
> zeroing inactive stats is bad behavior, not introduced by you of course,
> but worth to fix (as a followup, not necessarily by you)
I agree.
>
> unrelated to the prev paragraph,
> even if we remove explicit = 0 from the driver, ethtool userspace will
> still print (lots of) zeroes for "under-reported" stats, thanks to
> multi-syscall stats retrieval, that could be "fixed" in ethtool, but
> perhaps does not matter, as users use wrappers anyway
> </rambling>
The root cause of the OOB this time is that iavf_get_sset_count() uses
real_num_tx_queues for allocating buffers, and iavf_get_ethtool_stats()
uses num_active_queues for writing [1].
This may warrant more thought, but I was thinking that using
num_active_queues in all related functions would also be acceptable.
That's because, thanks to netdev_lock(), we can ensure num_active_queues
is unchanged within each ioctl call (ETHTOOL_GSSET_INFO,
ETHTOOL_GSTRINGS, ETHTOOL_GSTATS).
In other words, num_active_queues is effectively immutable for the
duration of a single ioctl, avoiding OOB.
[1]
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()
