| 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
| ||
|
Message-ID: <893B4BFD-1FDA-46DE-82D5-9E5CBDD90068@nvidia.com>
Date: Mon, 07 Apr 2025 10:15:02 -0400
From: Zi Yan <ziy@...dia.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Jesper Dangaard Brouer <hawk@...nel.org>, Saeed Mahameed <saeedm@...dia.com>,
Leon Romanovsky <leon@...nel.org>, Tariq Toukan <tariqt@...dia.com>,
Andrew Lunn <andrew+netdev@...n.ch>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Ilias Apalodimas <ilias.apalodimas@...aro.org>,
Simon Horman <horms@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
Mina Almasry <almasrymina@...gle.com>, Yonglong Liu <liuyonglong@...wei.com>,
Yunsheng Lin <linyunsheng@...wei.com>,
Pavel Begunkov <asml.silence@...il.com>,
Matthew Wilcox <willy@...radead.org>, <netdev@...r.kernel.org>,
<bpf@...r.kernel.org>, <linux-rdma@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH net-next v7 1/2] page_pool: Move pp_magic check into
helper functions
On 7 Apr 2025, at 9:36, Zi Yan wrote:
> On 7 Apr 2025, at 9:14, Toke Høiland-Jørgensen wrote:
>
>> Zi Yan <ziy@...dia.com> writes:
>>
>>> Resend to fix my signature.
>>>
>>> On 7 Apr 2025, at 4:53, Toke Høiland-Jørgensen wrote:
>>>
>>>> "Zi Yan" <ziy@...dia.com> writes:
>>>>
>>>>> On Fri Apr 4, 2025 at 6:18 AM EDT, Toke Høiland-Jørgensen wrote:
>>>>>> Since we are about to stash some more information into the pp_magic
>>>>>> field, let's move the magic signature checks into a pair of helper
>>>>>> functions so it can be changed in one place.
>>>>>>
>>>>>> Reviewed-by: Mina Almasry <almasrymina@...gle.com>
>>>>>> Tested-by: Yonglong Liu <liuyonglong@...wei.com>
>>>>>> Acked-by: Jesper Dangaard Brouer <hawk@...nel.org>
>>>>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@...aro.org>
>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
>>>>>> ---
>>>>>> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++--
>>>>>> include/net/page_pool/types.h | 18 ++++++++++++++++++
>>>>>> mm/page_alloc.c | 9 +++------
>>>>>> net/core/netmem_priv.h | 5 +++++
>>>>>> net/core/skbuff.c | 16 ++--------------
>>>>>> net/core/xdp.c | 4 ++--
>>>>>> 6 files changed, 32 insertions(+), 24 deletions(-)
>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>>>>>> index 36eb57d73abc6cfc601e700ca08be20fb8281055..df0d3c1608929605224feb26173135ff37951ef8 100644
>>>>>> --- a/include/net/page_pool/types.h
>>>>>> +++ b/include/net/page_pool/types.h
>>>>>> @@ -54,6 +54,14 @@ struct pp_alloc_cache {
>>>>>> netmem_ref cache[PP_ALLOC_CACHE_SIZE];
>>>>>> };
>>>>>>
>>>>>> +/* Mask used for checking in page_pool_page_is_pp() below. page->pp_magic is
>>>>>> + * OR'ed with PP_SIGNATURE after the allocation in order to preserve bit 0 for
>>>>>> + * the head page of compound page and bit 1 for pfmemalloc page.
>>>>>> + * page_is_pfmemalloc() is checked in __page_pool_put_page() to avoid recycling
>>>>>> + * the pfmemalloc page.
>>>>>> + */
>>>>>> +#define PP_MAGIC_MASK ~0x3UL
>>>>>> +
>>>>>> /**
>>>>>> * struct page_pool_params - page pool parameters
>>>>>> * @fast: params accessed frequently on hotpath
>>>>>> @@ -264,6 +272,11 @@ void page_pool_destroy(struct page_pool *pool);
>>>>>> void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>>>>>> const struct xdp_mem_info *mem);
>>>>>> void page_pool_put_netmem_bulk(netmem_ref *data, u32 count);
>>>>>> +
>>>>>> +static inline bool page_pool_page_is_pp(struct page *page)
>>>>>> +{
>>>>>> + return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>>>>> +}
>>>>>> #else
>>>>>> static inline void page_pool_destroy(struct page_pool *pool)
>>>>>> {
>>>>>> @@ -278,6 +291,11 @@ static inline void page_pool_use_xdp_mem(struct page_pool *pool,
>>>>>> static inline void page_pool_put_netmem_bulk(netmem_ref *data, u32 count)
>>>>>> {
>>>>>> }
>>>>>> +
>>>>>> +static inline bool page_pool_page_is_pp(struct page *page)
>>>>>> +{
>>>>>> + return false;
>>>>>> +}
>>>>>> #endif
>>>>>>
>>>>>> void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
>>>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>>>> index f51aa6051a99867d2d7d8c70aa7c30e523629951..347a3cc2c188f4a9ced85e0d198947be7c503526 100644
>>>>>> --- a/mm/page_alloc.c
>>>>>> +++ b/mm/page_alloc.c
>>>>>> @@ -55,6 +55,7 @@
>>>>>> #include <linux/delayacct.h>
>>>>>> #include <linux/cacheinfo.h>
>>>>>> #include <linux/pgalloc_tag.h>
>>>>>> +#include <net/page_pool/types.h>
>>>>>> #include <asm/div64.h>
>>>>>> #include "internal.h"
>>>>>> #include "shuffle.h"
>>>>>> @@ -897,9 +898,7 @@ static inline bool page_expected_state(struct page *page,
>>>>>> #ifdef CONFIG_MEMCG
>>>>>> page->memcg_data |
>>>>>> #endif
>>>>>> -#ifdef CONFIG_PAGE_POOL
>>>>>> - ((page->pp_magic & ~0x3UL) == PP_SIGNATURE) |
>>>>>> -#endif
>>>>>> + page_pool_page_is_pp(page) |
>>>>>> (page->flags & check_flags)))
>>>>>> return false;
>>>>>>
>>>>>> @@ -926,10 +925,8 @@ static const char *page_bad_reason(struct page *page, unsigned long flags)
>>>>>> if (unlikely(page->memcg_data))
>>>>>> bad_reason = "page still charged to cgroup";
>>>>>> #endif
>>>>>> -#ifdef CONFIG_PAGE_POOL
>>>>>> - if (unlikely((page->pp_magic & ~0x3UL) == PP_SIGNATURE))
>>>>>> + if (unlikely(page_pool_page_is_pp(page)))
>>>>>> bad_reason = "page_pool leak";
>>>>>> -#endif
>>>>>> return bad_reason;
>>>>>> }
>>>>>>
>>>>>
>>>>> I wonder if it is OK to make page allocation depend on page_pool from
>>>>> net/page_pool.
>>>>
>>>> Why? It's not really a dependency, just a header include with a static
>>>> inline function...
>>>
>>> The function is checking, not even modifying, an core mm data structure,
>>> struct page, which is also used by almost all subsystems. I do not get
>>> why the function is in net subsystem.
>>
>> Well, because it's using details of the PP definitions, so keeping it
>> there nicely encapsulates things. I mean, that's the whole point of
>> defining a wrapper function - encapsulating the logic :)
>>
>>>>> Would linux/mm.h be a better place for page_pool_page_is_pp()?
>>>>
>>>> That would require moving all the definitions introduced in patch 2,
>>>> which I don't think is appropriate.
>>>
>>> Why? I do not see page_pool_page_is_pp() or PP_SIGNATURE is used anywhere
>>> in patch 2.
>>
>> Look again. Patch 2 redefines PP_MAGIC_MASK in terms of all the other
>> definitions.
>
> OK. Just checked. Yes, the function depends on PP_MAGIC_MASK.
>
> But net/types.h has a lot of unrelated page_pool functions and data structures
> mm/page_alloc.c does not care about. Is there a way of moving page_pool_page_is_pp()
> and its dependency to a separate header and including that in mm/page_alloc.c?
>
> Looking at the use of page_pool_page_is_pp() in mm/page_alloc.c, it seems to be
> just error checking. Why can't page_pool do the error checking?
Or just remove page_pool_page_is_pp() in mm/page_alloc.c. Has it really been used?
--
Best Regards,
Yan, Zi
Powered by blists - more mailing lists