netdev - Re: [PATCH v5 bpf 2/2] veth: update mem type in xdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2212b2f7-d426-4f2f-abe5-3746dbb31631@meta.com>
Date: Fri, 31 Oct 2025 09:45:20 -0400
From: Chris Mason <clm@...a.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>, bot+bpf-ci@...nel.org
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, hawk@...nel.org,
        netdev@...r.kernel.org, magnus.karlsson@...el.com,
        aleksander.lobakin@...el.com, ilias.apalodimas@...aro.org,
        toke@...hat.com, lorenzo@...nel.org, kuba@...nel.org,
        andrii@...nel.org, martin.lau@...nel.org, eddyz87@...il.com,
        yonghong.song@...ux.dev, ihor.solodrai@...ux.dev
Subject: Re: [PATCH v5 bpf 2/2] veth: update mem type in xdp_buff

On 10/31/25 8:46 AM, Maciej Fijalkowski wrote:
> On Wed, Oct 29, 2025 at 10:59:36PM +0000, bot+bpf-ci@...nel.org wrote:
>>
>> ```
>> commit 96d6e22337a81e8606de33d22173ff80ab961bc8
>> Author: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
>>
>> veth: update mem type in xdp_buff
>>
>> The commit fixes a page_pool leak by properly detecting page_pool memory
>> and using appropriate reference counting APIs. The xdp_buff mem type is
>> now dynamically set based on skb->pp_recycle instead of always using
>> MEM_TYPE_PAGE_SHARED.
>>
>> Closes: https://lore.kernel.org/bpf/CAADnVQ+bBofJDfieyOYzSmSujSfJwDTQhiz3aJw7hE+4E2_iPA@mail.gmail.com/ 
>>
>>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>>> index a3046142c..187f30e2c 100644
>>> --- a/drivers/net/veth.c
>>> +++ b/drivers/net/veth.c
>>
>> [ ... ]
>>
>>> @@ -733,7 +733,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
>>>  	}
>>>  }
>>>
>>> -static void veth_xdp_get(struct xdp_buff *xdp)
>>> +static void veth_xdp_get_shared(struct xdp_buff *xdp)
>>>  {
>>>  	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>>>  	int i;
>>> @@ -746,12 +746,33 @@ static void veth_xdp_get(struct xdp_buff *xdp)
>>>  		__skb_frag_ref(&sinfo->frags[i]);
>>>  }
>>>
>>> +static void veth_xdp_get_pp(struct xdp_buff *xdp)
>>> +{
>>> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>>> +	int i;
>>> +
>>> +	page_pool_ref_page(virt_to_page(xdp->data));
>>> +	if (likely(!xdp_buff_has_frags(xdp)))
>>> +		return;
>>> +
>>> +	for (i = 0; i < sinfo->nr_frags; i++) {
>>> +		skb_frag_t *frag = &sinfo->frags[i];
>>> +
>>> +		page_pool_ref_page(netmem_to_page(frag->netmem));
>>> +	}
>>> +}
>>> +
>>> +static void veth_xdp_get(struct xdp_buff *xdp)
>>> +{
>>> +	xdp->rxq->mem.type == MEM_TYPE_PAGE_POOL ?
>>> +		veth_xdp_get_pp(xdp) : veth_xdp_get_shared(xdp);
>>> +}
>>> +
>>
>> [ ... ]
>>
>>> @@ -822,24 +831,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>>>  	case XDP_TX:
>>>  		veth_xdp_get(xdp);
>>>  		consume_skb(skb);
>>> -		xdp->rxq->mem = rq->xdp_mem;
>>>  		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
>>>  			trace_xdp_exception(rq->dev, xdp_prog, act);
>>>  			stats->rx_drops++;
>>>  			goto err_xdp;
>>>  		}
>>
>> Can veth_xdp_rcv_skb() leak page references in the XDP_TX error path?
>> When veth_xdp_tx() fails, the code jumps to err_xdp without releasing
>> the page references acquired by veth_xdp_get(). The skb was already
>> consumed, so the pages still have the extra references from either
>> veth_xdp_get_pp() or veth_xdp_get_shared().
> 
> You're missing xdp_return_buff() on err_xdp path.

Thanks, looking at the logs, that's exactly what happened.  AI thinks
the goto code looks like this:

drop:
    stats->rx_drops++;
xdp_drop:
    rcu_read_unlock();
    kfree_skb(skb);
err_xdp:
    return NULL;

Since I can't find this code snippet elsewhere in the kernel, I think it
just made it up.  I'll add some language that forces it to confirm the
snippets are correct.

-chris