| 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: <20250527010226.GA19906@system.software.com>
Date: Tue, 27 May 2025 10:02:26 +0900
From: Byungchul Park <byungchul@...com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Mina Almasry <almasrymina@...gle.com>, willy@...radead.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, kernel_team@...ynix.com, kuba@...nel.org,
ilias.apalodimas@...aro.org, harry.yoo@...cle.com, hawk@...nel.org,
akpm@...ux-foundation.org, davem@...emloft.net,
john.fastabend@...il.com, andrew+netdev@...n.ch, toke@...hat.com,
tariqt@...dia.com, edumazet@...gle.com, pabeni@...hat.com,
saeedm@...dia.com, leon@...nel.org, ast@...nel.org,
daniel@...earbox.net, david@...hat.com, lorenzo.stoakes@...cle.com,
Liam.Howlett@...cle.com, vbabka@...e.cz, rppt@...nel.org,
surenb@...gle.com, mhocko@...e.com, horms@...nel.org,
linux-rdma@...r.kernel.org, bpf@...r.kernel.org,
vishal.moola@...il.com
Subject: Re: [PATCH 18/18] mm, netmem: remove the page pool members in struct
page
On Mon, May 26, 2025 at 05:58:10PM +0100, Pavel Begunkov wrote:
> On 5/26/25 02:37, Byungchul Park wrote:
> > On Fri, May 23, 2025 at 10:55:54AM -0700, Mina Almasry wrote:
> > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@...com> wrote:
> > > >
> > > > Now that all the users of the page pool members in struct page have been
> > > > gone, the members can be removed from struct page.
> > > >
> > > > However, since struct netmem_desc might still use the space in struct
> > > > page, the size of struct netmem_desc should be checked, until struct
> > > > netmem_desc has its own instance from slab, to avoid conficting with
> > > > other members within struct page.
> > > >
> > > > Remove the page pool members in struct page and add a static checker for
> > > > the size.
> > > >
> > > > Signed-off-by: Byungchul Park <byungchul@...com>
> > > > ---
> > > > include/linux/mm_types.h | 11 -----------
> > > > include/net/netmem.h | 28 +++++-----------------------
> > > > 2 files changed, 5 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > > > index 873e820e1521..5a7864eb9d76 100644
> > > > --- a/include/linux/mm_types.h
> > > > +++ b/include/linux/mm_types.h
> > > > @@ -119,17 +119,6 @@ struct page {
> > > > */
> > > > unsigned long private;
> > > > };
> > > > - struct { /* page_pool used by netstack */
> > > > - unsigned long _pp_mapping_pad;
> > > > - /**
> > > > - * @pp_magic: magic value to avoid recycling non
> > > > - * page_pool allocated pages.
> > > > - */
> > > > - unsigned long pp_magic;
> > > > - struct page_pool *pp;
> > > > - unsigned long dma_addr;
> > > > - atomic_long_t pp_ref_count;
> > > > - };
> > > > struct { /* Tail pages of compound page */
> > > > unsigned long compound_head; /* Bit zero is set */
> > > > };
> > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > index c63a7e20f5f3..257c22398d7a 100644
> > > > --- a/include/net/netmem.h
> > > > +++ b/include/net/netmem.h
> > > > @@ -77,30 +77,12 @@ struct net_iov_area {
> > > > unsigned long base_virtual;
> > > > };
> > > >
> > > > -/* These fields in struct page are used by the page_pool and net stack:
> > > > - *
> > > > - * struct {
> > > > - * unsigned long _pp_mapping_pad;
> > > > - * unsigned long pp_magic;
> > > > - * struct page_pool *pp;
> > > > - * unsigned long dma_addr;
> > > > - * atomic_long_t pp_ref_count;
> > > > - * };
> > > > - *
> > > > - * We mirror the page_pool fields here so the page_pool can access these fields
> > > > - * without worrying whether the underlying fields belong to a page or net_iov.
> > > > - *
> > > > - * The non-net stack fields of struct page are private to the mm stack and must
> > > > - * never be mirrored to net_iov.
> > > > +/* XXX: The page pool fields in struct page have been removed but they
> > > > + * might still use the space in struct page. Thus, the size of struct
> > > > + * netmem_desc should be under control until struct netmem_desc has its
> > > > + * own instance from slab.
> > > > */
> > > > -#define NET_IOV_ASSERT_OFFSET(pg, iov) \
> > > > - static_assert(offsetof(struct page, pg) == \
> > > > - offsetof(struct net_iov, iov))
> > > > -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> > > > -NET_IOV_ASSERT_OFFSET(pp, pp);
> > > > -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> > > > -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> > > > -#undef NET_IOV_ASSERT_OFFSET
> > > > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> > > >
> > >
> > > Removing these asserts is actually a bit dangerous. Functions like
> > > netmem_or_pp_magic() rely on the fact that the offsets are the same
> > > between struct page and struct net_iov to access these fields without
> >
> > Worth noting this patch removes the page pool fields from struct page.
>
> static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> {
> return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> }
>
> static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem)
> {
> return &__netmem_clear_lsb(netmem)->pp_ref_count;
> }
>
> That's a snippet of code after applying the series. So, let's say we
> take a page, it's casted to netmem, then the netmem (as it was before)
> is casted to net_iov. Before it relied on net_iov and the pp's part of
> the page having the same layout, which was checked by static asserts,
> but now, unless I'm mistaken, it's aligned in the exactly same way but
> points to a seemingly random offset of the page. We should not be doing
> that.
I told it in another thread. My bad. I will fix it.
> Just to be clear, I think casting pages to struct net_iov *, as it
> currently is, is quite ugly, but that's something netmem_desc and this
> effort can help with.
>
> What you likely want to do is:
>
> Patch 1:
>
> struct page {
> unsigned long flags;
> union {
> struct_group_tagged(netmem_desc, netmem_desc) {
> // same layout as before
> ...
> struct page_pool *pp;
> ...
> };
This part will be gone shortly. The matters come from absence of this
part.
> }
> }
>
> struct net_iov {
> unsigned long flags_padding;
> union {
> struct {
> // same layout as in page + build asserts;
> ...
> struct page_pool *pp;
> ...
> };
> struct netmem_desc desc;
> };
> };
>
> struct netmem_desc *page_to_netmem_desc(struct page *page)
> {
> return &page->netmem_desc;
page will not have any netmem things in it after this, that matters.
> }
>
> struct netmem_desc *netmem_to_desc(netmem_t netmem)
> {
> if (netmem_is_page(netmem))
> return page_to_netmem_desc(netmem_to_page(netmem);
> return &netmem_to_niov(netmem)->desc;
> }
>
> The compiler should be able to optimise the branch in netmem_to_desc(),
> but we might need to help it a bit.
>
>
> Then, patch 2 ... N convert page pool and everyone else accessing
> those page fields directly to netmem_to_desc / etc.
>
> And the final patch replaces the struct group in the page with a
> new field:
>
> struct netmem_desc {
> struct page_pool *pp;
> ...
> };
>
> struct page {
> unsigned long flags_padding;
> union {
> struct netmem_desc desc;
^
should be gone.
Byungchul
> ...
> };
> };
>
> net_iov will drop its union in a later series to avoid conflicts.
>
> btw, I don't think you need to convert page pool to netmem for this
> to happen, so that can be done in a separate unrelated series. It's
> 18 patches, and netdev usually requires it to be no more than 15.
>
> --
> Pavel Begunkov
Powered by blists - more mailing lists