netdev - Re: [PATCH net-next v1] page_pool: check for dma_sync

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] [day] [month] [year] [list]

Message-ID: <f2e480e8-f4c9-47a5-af85-60c68279a43f@huawei.com>
Date: Wed, 16 Oct 2024 17:09:12 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
CC: Furong Xu <0x1207@...il.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Jesper Dangaard Brouer <hawk@...nel.org>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<xfr@...look.com>
Subject: Re: [PATCH net-next v1] page_pool: check for dma_sync_size earlier

On 2024/10/15 21:25, Ilias Apalodimas wrote:

...

>>>>
>>>> --- a/include/net/page_pool/helpers.h
>>>> +++ b/include/net/page_pool/helpers.h
>>>> @@ -317,8 +317,10 @@ static inline void page_pool_put_netmem(struct page_pool *pool,
>>>>          * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>>>>          */
>>>>  #ifdef CONFIG_PAGE_POOL
>>>> -       if (!page_pool_is_last_ref(netmem))
>>>> +       if (!page_pool_is_last_ref(netmem)) {
>>>> +               /* Big comment why frag API is not support yet */
>>>> +               DEBUG_NET_WARN_ON_ONCE(!dma_sync_size);
>>
>> Note, the above checking is not 100% reliable, as which frag user
>> is the last one depending on runtime execution.
> 
> I am not sure I understand the problem here. If we are about to call
> page_pool_return_page() we don't care what happens to that page.
> If we end up calling __page_pool_put_page() it's the *callers* job now
> to sync the page now once all fragments are released. So why is this
> different from syncing an entire page?

It also would be that we end up calling nothing for non-last-frag-user
too when page_pool_is_last_ref() return false as the above 'if' checking
in page_pool_put_netmem() if frag related API is used.

If the above happens, there is no dma_sync done here even if the caller
passes a non-zero 'dma_sync_size' which means it is not supposed to call
page_pool_put_unrefed_netmem() for the same page with both zero and
non-zero 'dma_sync_size'.

For example:
1. If page_pool_put_page() with dma_sync_size being non-zero is first
   called for a page, and there might be no dma_sync operation if it is
   not the last frag.
2. Then page_pool_put_page() with dma_sync_size being zero is called for
   the same page to skip the dma_sync operation.

Then we might have problem here as the dma_sync operation is only expected
to be skipped for a specific page fragment, but the above calling order causes
the dma_sync operation to be skipped for the whole page.

IOW, there is currently no way to tell which fragment is being freed when
page_pool_put_page() API is called for a page split into multi fragments now,
so we might need to call page_pool_put_page() with 'dma_sync_size' all being
'-1' if the page_pool is expected to do the dma sync operation for now.

> 
>>