netdev - Re: [BUG] Possible unsafe page

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: <CAC_iWjL6gnfqC685Mv9MwqtOts4kHmLWFFtWSYV3rp82eJ_VEg@mail.gmail.com>
Date: Thu, 24 Aug 2023 10:42:21 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>, netdev@...r.kernel.org, 
	Ratheesh Kannoth <rkannoth@...vell.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Geetha sowjanya <gakula@...vell.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Subbaraya Sundeep <sbhatta@...vell.com>, Sunil Goutham <sgoutham@...vell.com>, 
	Thomas Gleixner <tglx@...utronix.de>, hariprasad <hkelam@...vell.com>, 
	Alexander Lobakin <aleksander.lobakin@...el.com>, 
	Qingfang DENG <qingfang.deng@...lower.com.cn>
Subject: Re: [BUG] Possible unsafe page_pool usage in octeontx2

On Thu, 24 Aug 2023 at 10:21, Ilias Apalodimas
<ilias.apalodimas@...aro.org> wrote:
>
> [...]
>
> > >
> > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > > index 7ff80b80a6f9f..b50e219470a36 100644
> > > --- a/net/core/page_pool.c
> > > +++ b/net/core/page_pool.c
> > > @@ -612,7 +612,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
> > >                       page_pool_dma_sync_for_device(pool, page,
> > >                                                     dma_sync_size);
> > >
> > > -             if (allow_direct && in_softirq() &&
> > > +             if (allow_direct && in_serving_softirq() &&
> >
> > This is the "return/free/put" code path, where we have "allow_direct" as
> > a protection in the API.  API users are suppose to use
> > page_pool_recycle_direct() to indicate this, but as some point we
> > allowed APIs to expose 'allow_direct'.
> >
> > The PP-alloc side is more fragile, and maybe the in_serving_softirq()
> > belongs there.
> >
> > >                   page_pool_recycle_in_cache(page, pool))
> > >                       return NULL;
> > >
> > > because the intention (as I understand it) is to be invoked from within
> > > the NAPI callback (while softirq is served) and not if BH is just
> > > disabled due to a lock or so.
> > >
> >
> > True, and it used-to-be like this (in_serving_softirq), but as Ilias
> > wrote it was changed recently.  This was to support threaded-NAPI (in
> > 542bcea4be866b ("net: page_pool: use in_softirq() instead")), which
> > I understood was one of your (Sebastian's) use-cases.
> >
> >
> > > It would also make sense to a add WARN_ON_ONCE(!in_serving_softirq()) to
> > > page_pool_alloc_pages() to spot usage outside of softirq. But this will
> > > trigger in every driver since the same function is used in the open
> > > callback to initially setup the HW.
> > >
> >
> > I'm very open to ideas of detecting this.  Since mentioned commit PP is
> > open to these kind of miss-uses of the API.
> >
> > One idea would be to leverage that NAPI napi->list_owner will have been
> > set to something else than -1, when this is NAPI context.  Getting hold
> > of napi object, could be done via pp->p.napi (but as Jakub wrote this is
> > opt-in ATM).
>
> I mentioned this earlier, but can't we add the softirq check in
> __page_pool_get_cached()?
> In theory, when a driver comes up and allocates pages to fill in its
> descriptors it will call page_pool_alloc_pages().  That will go
> through the slow allocation path, fill up the caches, and return the
> last page.  After that, most of the allocations will be served by
> __page_pool_get_cached(), and this is supposed to be running during
> the driver Rx routine which runs under NAPI.  So eventually we will
> hit that warning.

Right... Scratch that, this will still warn on the initial allocation.
The first descriptor will get a page of the slow path, but the rest
will be filled via the caches.


/Ilias
>
> Thanks
> /Ilias
>
> >
> > --Jesper