netdev - Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page pool.

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:   Thu, 20 Dec 2018 16:56:45 -0800
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Saeed Mahameed" <saeedm@...lanox.com>
Cc:     brouer@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page
 pool.

On 20 Dec 2018, at 15:41, Saeed Mahameed wrote:

> On Thu, 2018-12-20 at 14:11 -0800, Jonathan Lemon wrote:
>> (Resending due to mailer issues)
>>
>> On 20 Dec 2018, at 5:03, Jesper Dangaard Brouer wrote:
>>
>>> On Wed, 19 Dec 2018 12:06:51 -0800
>>> Jonathan Lemon <jonathan.lemon@...il.com> wrote:
>>>
>>>> Return pfmemalloc pages back to the page allocator, instead of
>>>> holding them in the page pool.
>>>
>>> Have you experience this issue in practice or is it theory?
>>
>> We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and
>> then
>> return them
>> back to the page allocator.  (it's triggering the
>> mlx5e_page_is_reserved() test).
>> The page pool code isn't in production use, but the code paths
>> appear
>> identical.
>>
>>
>>>>  While here, also use the __page_pool_return_page() API.
>>>
>>> Don't combine several unrelated changed in one patch.
>>
>> Okay - will send as 2 separate patches
>>
>>
>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>>> index 43a932cb609b..091007ff14a3 100644
>>>> --- a/net/core/page_pool.c
>>>> +++ b/net/core/page_pool.c
>>>> @@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool
>>>> *pool,
>>>>  	 *
>>>>  	 * refcnt == 1 means page_pool owns page, and can recycle it.
>>>>  	 */
>>>> -	if (likely(page_ref_count(page) == 1)) {
>>>> +	if (likely(page_ref_count(page) == 1 &&
>>>> !page_is_pfmemalloc(page)))
>>>> {
>>>
>
> I think this is wrong, if refcount is 1, then this page belongs to
> pagepool and you must enter this statement's true block, and test
> page_is_pfmemalloc inside (mark it unlikely),
> to return a pfmemalloc page, you need to call __page_pool_return_page()
> to dma_unmap and other cleanups if required.

If the page belongs to the pool, but it isn't recyclable, the fallback
path is "__page_pool_return_page()".  If it doesn't belong to the pool,
it goes to the non-XDP mode, which is also __page_pool_return_page().

Does your dislike stem from the fact that the "non-XDP" mode is taken
for the "refcount=1, pfmemalloc=T" case?
-- 
Jonathan