Thank you for your contribution! Sashiko AI review found 4 potential issues to consider: - [High] virtio_net: virtnet_xsk_pools_attach() attaches unmapped AF_XDP pool - [Medium] virtio_net: UAF on vi->rq in virtnet_xsk_pool_disable() after failed restore - [Medium] virtio_net: DMA map leak in virtnet_xsk_pool_disable() after XDP detach - [Medium] virtio_net: missing rtnl_lock() around xsk_pool_dma_unmap() on freeze --
commit f8bbdffa48d34460262d93d81019cacaef0e299f Author: Haishuang Yan <[email protected]> virtio_net: Keep AF_XDP zero-copy pools bound across suspend This commit preserves AF_XDP zero-copy pools across suspend and resume by releasing only the per-queue driver state on freeze while keeping the pools DMA-mapped, then re-attaching them to the newly allocated queues on restore. > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index f9d227060375c..d9c920f421268 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c [ ... ] > @@ -5808,6 +5810,7 @@ static int virtnet_restore_up(struct virtio_device > *vdev) > err_destroy_pools: > virtio_reset_device(vdev); > free_unused_bufs(vi); > + virtnet_xsk_pools_release(vi); > virtnet_destroy_page_pools(vi); > virtnet_del_vqs(vi); > return err; > @@ -6038,6 +6041,13 @@ static int virtnet_xsk_pool_disable(struct net_device > *dev, u16 qid) > rq = &vi->rq[qid]; > > pool = rq->xsk_pool; > + if (!pool) { > + /* Binding it again on restore failed, only the DMA map is left > */ > + pool = xsk_get_pool_from_qid(dev, qid); > + if (pool) > + xsk_pool_dma_unmap(pool, 0); > + return 0; > + } [Severity: Medium] Can virtnet_xsk_pool_disable() dereference a freed or NULL vi->rq after a failed restore? If virtnet_restore_up() fails (for example at err_destroy_pools, or earlier in init_vqs()), virtnet_del_vqs() -> virtnet_free_queues() frees vi->rq and vi->sq while the net_device and bound AF_XDP pools remain registered. When the socket is later closed, virtnet_xsk_pool_disable() reads vi->rq[qid].xsk_pool from the dangling vi->rq pointer. [Severity: Medium] Can this fallback also leak the pool's DMA mappings if XDP is detached before the socket is closed? When virtnet_xsk_pool_attach() fails on restore for a pool bound to an XDP queue (qid >= curr_qp), rq->xsk_pool is left NULL while the pool stays DMA-mapped. If the XDP program is then removed via virtnet_xdp_set(), netif_set_real_num_rx_queues() lowers dev->real_num_rx_queues to curr_qp <= qid (and dev->real_num_tx_queues is also curr_qp). When the socket is subsequently closed, xsk_get_pool_from_qid(dev, qid) returns NULL because qid is no longer below real_num_rx_queues or real_num_tx_queues, skipping xsk_pool_dma_unmap(). Unmapping the pool directly in virtnet_xsk_pools_attach() when virtnet_xsk_pool_attach() fails would avoid leaving a half-bound DMA map behind. > @@ -6053,6 +6063,133 @@ static int virtnet_xsk_pool_disable(struct net_device > *dev, u16 qid) > return err; > } > > +/* > + * Freeze frees the receive and send queues, but the AF_XDP sockets stay > + * bound and the xsk core keeps their pools. Release only the driver state > + * of the zero-copy pools here, once the device is reset and the buffers > + * posted to it are back in the pools. The pools stay DMA mapped, since the > + * DMA device is still the same after restore and the buffers on their free > + * lists keep their DMA addresses. > + * > + * A pool left above curr_queue_pairs by removing XDP is not bound again > + * on restore, so unmap it as well. > + */ > +static void virtnet_xsk_pools_release(struct virtnet_info *vi) > +{ > + int i; > + > + for (i = 0; i < vi->max_queue_pairs; i++) { > + struct receive_queue *rq = &vi->rq[i]; > + struct send_queue *sq = &vi->sq[i]; > + > + if (rq->xsk_pool) { > + if (i >= vi->curr_queue_pairs) > + xsk_pool_dma_unmap(rq->xsk_pool, 0); [Severity: Medium] Does calling xsk_pool_dma_unmap() here without holding rtnl_lock() race on pool->umem->xsk_dma_list? remove_vq_common() calls virtnet_xsk_pools_release() outside rtnl_lock() during virtnet_freeze(). xp_dma_unmap() walks and deletes entries from pool->umem->xsk_dma_list, which is protected by rtnl_lock. If an xdp_umem is shared across multiple devices via XDP_SHARED_UMEM, concurrent async suspends can corrupt xsk_dma_list. [ ... ] > +/* > + * On restore, set the zero-copy pools up again on the new queues before > + * they are filled and their NAPI is enabled. > + */ > +static void virtnet_xsk_pools_attach(struct virtnet_info *vi) > +{ > + struct xsk_buff_pool *pool; > + int i, err; > + > + ASSERT_RTNL(); > + > + for (i = 0; i < vi->curr_queue_pairs; i++) { > + pool = xsk_get_pool_from_qid(vi->dev, i); > + if (!pool || !pool->umem->zc) > + continue; > + > + /* The pool stays DMA mapped until it is disabled */ > + err = virtnet_xsk_pool_attach(vi, i, pool); [Severity: High] Can virtnet_xsk_pools_attach() re-attach a pool that was already DMA-unmapped by virtnet_xsk_pools_release()? If a pool is bound to an XDP queue qid, detaching XDP lowers vi->curr_queue_pairs below qid, and the next freeze unmaps the pool in virtnet_xsk_pools_release() via xsk_pool_dma_unmap(). xp_dma_unmap() sets pool->dma_pages = NULL and pool->dev = NULL, but pool->umem->zc and dev->_rx[qid].pool remain set. If XDP is re-attached after resume (raising vi->curr_queue_pairs above qid again) and the system suspends a second time, virtnet_xsk_pools_attach() sees pool->umem->zc == true and attaches the unmapped pool to the queue, leading to a NULL pointer dereference on pool->dma_pages or pool->dev during RX/TX. Should this check pool->dma_pages instead of pool->umem->zc, or re-map the pool when pool->dma_pages is NULL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
