linux-kernel - Re: [PATCH bpf-next v3 4/4] xdp: remove unused {__,}xdp_release

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]

Date:   Tue, 14 Mar 2023 13:27:50 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Yunsheng Lin <linyunsheng@...wei.com>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Song Liu <song@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Menglong Dong <imagedong@...cent.com>,
        "Mykola Lysenko" <mykolal@...com>,
        "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 4/4] xdp: remove unused
 {__,}xdp_release_frame()

From: Yunsheng Lin <linyunsheng@...wei.com>
Date: Tue, 14 Mar 2023 19:37:23 +0800

> On 2023/3/14 5:55, Alexander Lobakin wrote:
>> __xdp_build_skb_from_frame() was the last user of
>> {__,}xdp_release_frame(), which detaches pages from the page_pool.

[...]

>> -/* Only called for MEM_TYPE_PAGE_POOL see xdp.h */
>> -void __xdp_release_frame(void *data, struct xdp_mem_info *mem)
>> -{
>> -	struct xdp_mem_allocator *xa;
>> -	struct page *page;
>> -
>> -	rcu_read_lock();
>> -	xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>> -	page = virt_to_head_page(data);
>> -	if (xa)
>> -		page_pool_release_page(xa->page_pool, page);
> 
> page_pool_release_page() is only call here when xa is not NULL
> and mem->type == MEM_TYPE_PAGE_POOL.
> 
> But skb_mark_for_recycle() is call when mem->type == MEM_TYPE_PAGE_POOL
> without checking xa, it does not seems symmetric to patch 3, if this is
> intended?

Intended. page_pool_return_skb_page() checks for %PP_SIGNATURE and if
a page doesn't belong to any PP, it will be returned to the MM layer.
Moreover, cases `mem->type == MEM_TYPE_PAGE_POOL && xa == NULL` are more
of an exception rather than regular -- this means the page was released
from its PP before reaching the function and IIRC it's even impossible
with our current drivers. Adding a hashtable lookup to
{__,}xdp_build_skb_from_frame() would only add hotpath overhead with no
positive impact.

> 
>> -	rcu_read_unlock();
>> -}
>> -EXPORT_SYMBOL_GPL(__xdp_release_frame);
>> -
>>  void xdp_attachment_setup(struct xdp_attachment_info *info,
>>  			  struct netdev_bpf *bpf)
>>  {
>>

Thanks,
Olek