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
