Re: [PATCH net 1/2] virtio_net: check AF_XDP queue index before use

On Fri, Aug 7, 2026 at 12:23 AM Jakub Kicinski <[email protected]> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> virtio_net: check AF_XDP queue index before use
>
> This moves the qid >= vi->curr_queue_pairs test in
> virtnet_xsk_pool_enable() so that it runs before the vi->rq[qid].page_pool
> check instead of after it.  The rest of the function is unchanged.
>
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 3e2a5876c6c8c..6160aa8ba595e 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -5896,15 +5896,15 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
> >       if (vi->hdr_len > xsk_pool_get_headroom(pool))
> >               return -EINVAL;
> >
> > +     if (qid >= vi->curr_queue_pairs)
> > +             return -EINVAL;
> > +
> >       /* In big_packets mode, xdp cannot work, so there is no need to
> >        * initialize xsk of rq.
> >        */
> >       if (!vi->rq[qid].page_pool)
> >               return -ENOENT;
> >
> > -     if (qid >= vi->curr_queue_pairs)
> > -             return -EINVAL;
> > -
>
> [Severity: Medium]
> The changelog says this "keeps an out of range queue id from reaching
> vi->rq[qid] while checking page_pool".  Was an out of range qid actually
> reachable here?
>
> vi->rq and vi->sq are sized max_queue_pairs in virtnet_alloc_queues():
>
>         vi->sq = kzalloc_objs(*vi->sq, vi->max_queue_pairs);
>         ...
>         vi->rq = kzalloc_objs(*vi->rq, vi->max_queue_pairs);
>
> and the netdev is created with alloc_etherdev_mq(..., max_queue_pairs), so
> dev->num_rx_queues == dev->num_tx_queues == max_queue_pairs.
>
> The only path into virtnet_xsk_pool_enable() is
> xp_assign_dev() -> ndo_bpf(XDP_SETUP_XSK_POOL) -> virtnet_xsk_pool_setup(),
> and xp_assign_dev() first goes through xsk_reg_pool_at_qid() in
> net/xdp/xsk.c:
>
>         if (queue_id >= max(dev->real_num_rx_queues,
>                             dev->real_num_tx_queues))
>                 return -EINVAL;
>
> real_num_rx_queues/real_num_tx_queues can never exceed num_rx_queues/
> num_tx_queues, so any qid that reaches the driver is already inside the
> allocated arrays and vi->rq[qid].page_pool was an in-bounds read.
>
> For a qid in [curr_queue_pairs, max_queue_pairs) the old ordering still
> rejected the request before sq/rq were used, so the only visible difference
> after the reorder appears to be -EINVAL instead of -ENOENT.  Could the
> changelog describe this as a defensive reordering rather than as preventing
> an out of range dereference?  As written it reads like a memory safety fix
> and may attract a stable backport.

Point taken. Sashiko is good at finding things like this stuff...

I don't see the necessity to keep this patch then.

Thanks,
Jason