On Fri, Sep 26, 2025 at 12:40 PM Jakub Kicinski [off-list ref] wrote:

On Fri, 26 Sep 2025 13:24:12 +0200 Maciej Fijalkowski wrote:

quoted

On Fri, Sep 26, 2025 at 12:33:46AM -0700, Octavian Purdila wrote:

quoted

On Thu, Sep 25, 2025 at 7:12 PM Jakub Kicinski [off-list ref] wrote:
Ah, yes, you are right. So my comment in the commit message about
TUN/TAP registering a page shared memory model is wrong. But I think
the fix is still correct for the reported syzkaller issue. From
bpf_prog_run_generic_xdp:

        rxqueue = netif_get_rxqueue(skb);
        xdp_init_buff(xdp, frame_sz, rxq: &rxqueue->xdp_rxq);

So xdp_buff's rxq is set to the netstack queue for the generic XDP
hook. And adding the check in netif_skb_check_for_xdp based on the
netstack queue should be correct, right?

Per my limited understanding your change is making skb_cow_data_for_xdp()
a dead code as I don't see mem model being registered for these stack
queues - netif_alloc_rx_queues() only calls xdp_rxq_info_reg() and
mem.type defaults to MEM_TYPE_PAGE_SHARED as it's defined as 0, which
means it's never going to be MEM_TYPE_PAGE_POOL.

Hah, that's a great catch, how did I miss that..

The reason for the cow is that frags can be shared, we are not allowed
to modify them. It's orthogonal.

Could we use the same hack that you mention below (declare rxq on the
stack and fill in the mem info there) for do_xdp_generic?

@@ -5442,7 +5448,10 @@ int do_xdp_generic(const struct bpf_prog

*xdp_prog, struct sk_buff **pskb)
        struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;

        if (xdp_prog) {
-               struct xdp_buff xdp;
+               struct xdp_rxq_info rxq = {};
+               struct xdp_buff xdp = {
+                       .rxq = &rxq,
+               };
                u32 act;
                int err;

and then explicitly set the mem type:

@@ -5331,14 +5332,19 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff

*skb, struct xdp_buff *xdp,
        return act;
 }

-static int
-netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog)
+static int netif_skb_check_for_xdp(struct sk_buff **pskb,
+                                  const struct bpf_prog *prog,
+                                  struct xdp_rxq_info *rxq)
 {
        struct sk_buff *skb = *pskb;
        int err, hroom, troom;
+       struct page_pool *pool;

        local_lock_nested_bh(&system_page_pool.bh_lock);
-       err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool),
pskb, prog);
+       pool = this_cpu_read(system_page_pool.pool);
+       err = skb_cow_data_for_xdp(pool, pskb, prog);
+       rxq->mem.type = MEM_TYPE_PAGE_POOL;
+       rxq->mem.id = pool->xdp_mem_id;
        local_unlock_nested_bh(&system_page_pool.bh_lock);
        if (!err)
                return 0;

quoted

IMHO that single case where we rewrite skb to memory backed by page pool
should have it reflected in mem.type so __xdp_return() potentially used in
bpf helpers could act correctly.

quoted

quoted

Well, IDK how helpful the flow below would be but:

veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one()
                                                               |
                            | xdp_convert_frame_to_buff()   <-'
    ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
  can eat frags but now rxq | bpf_prog_run_xdp()
         is veth's          |

I just glanced at the code so >50% changes I'm wrong, but that's what
I meant.

Thanks for the clarification, I thought that "re-stamps" means the:

    xdp->rxq->mem.type = frame->mem_type;

from veth_xdp_rcv_one in the XDP_TX/XDP_REDIRECT cases.

And yes, now I think the same issue can happen because veth sets the
memory model to MEM_TYPE_PAGE_SHARED but veth_convert_skb_to_xdp_buff
calls skb_pp_cow_data that uses page_pool for allocations. I'll try to
see if I can adapt the syzkaller repro to trigger it for confirmation.

That is a good catch.

FWIW I think all calls to xdp_convert_frame_to_buff() must come with
the hack that cpu_map_bpf_prog_run_xdp() is doing today. Declare rxq
on the stack and fill in the mem info there. I wonder if we should add
something to the API (xdp_convert_frame_to_buff()) to make sure people
don't forget to do this..