netdev - Re: [PATCH net] net: page_pool: don't try to stash the napi id

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]

Message-ID: <CAHS8izOv=tUiuzha6NFq1-ZurLGz9Jdi78jb3ey4ExVJirMprA@mail.gmail.com>
Date: Fri, 24 Jan 2025 15:49:32 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net, netdev@...r.kernel.org, 
	edumazet@...gle.com, pabeni@...hat.com, andrew+netdev@...n.ch, 
	horms@...nel.org, hawk@...nel.org, ilias.apalodimas@...aro.org, 
	asml.silence@...il.com, kaiyuanz@...gle.com, willemb@...gle.com, 
	mkarsten@...terloo.ca, jdamato@...tly.com
Subject: Re: [PATCH net] net: page_pool: don't try to stash the napi id

On Fri, Jan 24, 2025 at 2:18 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Mina Almasry <almasrymina@...gle.com> writes:
>
> > On Thu, Jan 23, 2025 at 3:16 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >>
> >> Page ppol tried to cache the NAPI ID in page pool info to avoid
> >
> > Page pool
> >
> >> having a dependency on the life cycle of the NAPI instance.
> >> Since commit under Fixes the NAPI ID is not populated until
> >> napi_enable() and there's a good chance that page pool is
> >> created before NAPI gets enabled.
> >>
> >> Protect the NAPI pointer with the existing page pool mutex,
> >> the reading path already holds it. napi_id itself we need
> >
> > The reading paths in page_pool.c don't hold the lock, no? Only the
> > reading paths in page_pool_user.c seem to do.
> >
> > I could not immediately wrap my head around why pool->p.napi can be
> > accessed in page_pool_napi_local with no lock, but needs to be
> > protected in the code in page_pool_user.c. It seems
> > READ_ONCE/WRITE_ONCE protection is good enough to make sure
> > page_pool_napi_local doesn't race with
> > page_pool_disable_direct_recycling in a way that can crash (the
> > reading code either sees a valid pointer or NULL). Why is that not
> > good enough to also synchronize the accesses between
> > page_pool_disable_direct_recycling and page_pool_nl_fill? I.e., drop
> > the locking?
>
> It actually seems that this is *not* currently the case. See the
> discussion here:
>
> https://lore.kernel.org/all/8734h8qgmz.fsf@toke.dk/
>
> IMO (as indicated in the message linked above), we should require users
> to destroy the page pool before freeing the NAPI memory, rather than add
> additional synchronisation.
>

Ah, I see. I wonder if we should make this part of the API via comment
and/or add DEBUG_NET_WARN_ON to catch misuse, something like:

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index ed4cd114180a..3919ca302e95 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -257,6 +257,10 @@ struct xdp_mem_info;

 #ifdef CONFIG_PAGE_POOL
 void page_pool_disable_direct_recycling(struct page_pool *pool);
+
+/* page_pool_destroy or page_pool_disable_direct_recycling must be
called before
+ * netif_napi_del if pool->p.napi is set.
+ */
 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);

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5c4b788b811b..dc82767b2516 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -1161,6 +1161,8 @@ void page_pool_destroy(struct page_pool *pool)
        if (!page_pool_put(pool))
                return;

+       DEBUG_NET_WARN_ON(pool->p.napi && !napi_is_valid(pool->p.napi));
+
        page_pool_disable_direct_recycling(pool);
        page_pool_free_frag(pool);

I also took a quick spot check - which could be wrong - but it seems
to me both gve and bnxt free the napi before destroying the pool :(

But I think this entire discussion is unrelated to this patch, so and
the mutex sync in this patch seems necessary for the page_pool_user.c
code which runs outside of softirq context:

Reviewed-by: Mina Almasry <almasrymina@...gle.com>


-- 
Thanks,
Mina