netdev - Re: [RFC] net: esp: fix bad handling of pages from 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: <CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com>
Date: Wed, 6 Mar 2024 10:46:45 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>, Liang Chen <liangchen.linux@...il.com>, 
	Yunsheng Lin <linyunsheng@...wei.com>
Cc: Dragos Tatulea <dtatulea@...dia.com>, "davem@...emloft.net" <davem@...emloft.net>, 
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>, Gal Pressman <gal@...dia.com>, 
	"dsahern@...nel.org" <dsahern@...nel.org>, 
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	Leon Romanovsky <leonro@...dia.com>, "edumazet@...gle.com" <edumazet@...gle.com>, 
	"ian.kumlien@...il.com" <ian.kumlien@...il.com>, 
	"Anatoli.Chechelnickiy@...nterpipe.biz" <Anatoli.Chechelnickiy@...nterpipe.biz>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC] net: esp: fix bad handling of pages from page_pool

On Wed, Mar 6, 2024 at 10:41 AM Mina Almasry <almasrymina@...gle.com> wrote:
>
> On Wed, Mar 6, 2024 at 9:41 AM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Wed, 6 Mar 2024 17:09:57 +0000 Dragos Tatulea wrote:
> > > > Does the caller need to check skb->pp_recycle? pp_recycle seems like a
> > > > redundant bit. We can tell whether the page is pp by checking
> > > > is_pp_page(page). the pages in the frag must be pp pages when
> > > > skb->pp_recycle is set and must be non pp pages when the
> > > > skb->pp_recycle is not set, so it all seems redundant to me.
> > > >
> > > AFAIU we don't have to check for pp_recycle, at least not in this specific case.
> >
> > Definitely not something we assuming in a fix that needs to go
> > to stable.
> >
> > So far, AFAIU, it's legal to have an skb without skb->pp_recycle
> > set, which holds full page refs to a PP page.
>
> Interesting. I apologize then I did not realize this combination is
> legal. But I have a follow up question: what is the ref code supposed
> to do in this combination? AFAIU:
>
> - skb->pp_recycle && is_pp_page()
> ref via page_pool_ref_page()
> unref via page_pool_unref_page()
>
> - !skb->pp_recycle && !is_pp_page()
> ref via get_page()
> unref via put_page()
>
> - !skb->pp_recycle && is_pp_page()
> ref via ?
> unref via?
>
> Also is this combination also legal you think? and if so what to do?
> - skb->pp_recycle && !is_pp_page()
> ref via ?
> unref via?
>
> I'm asking because I'm starting to wonder if this patch has some issue in it:
> https://patchwork.kernel.org/project/netdevbpf/patch/20231215033011.12107-4-liangchen.linux@gmail.com/
>
> Because in this patch, if we have a !skb->pp_recycle & is_pp_page()
> combination we end up doing in skb_try_coalesce():
> ref via page_pool_ref_page()
> unref via put_page() via eventual napi_frag_unref()
>
> which seems like an issue, no?
>

Gah, nevermind, skb_pp_frag_ref() actually returns -EINVAL if
!skb->pp_recycle, and in the call site we do a skb_frag_ref() on this
error, so all in all we end up doing a get_page/put_page pair. Sorry
for the noise.

So we're supposed to:
- !skb->pp_recycle && is_pp_page()
ref via get_page
unref via put_page

Very subtle stuff (for me at least). I'll try to propose some cleanup
to make this a bit simpler using helpers that handle all these subtle
details internally so that the call sites don't have to do this
special handling.


-- 
Thanks,
Mina