netdev - RE: [PATCH] net: use non-compound pages in frag allocator

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: <VI1PR0501MB1968D785003AA94C1EB0F226D4070@VI1PR0501MB1968.eurprd05.prod.outlook.com>
Date:	Thu, 4 Aug 2016 13:38:41 +0000
From:	Ilya Lesokhin <ilyal@...lanox.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>
Subject: RE: [PATCH] net: use non-compound pages in frag allocator

You are right I just realized that this patch causes a memory leak, so sorry for the spam.

I just had problem understanding when compound pages are needed and when It is enough to use an high order pages.
I thought that that high order pages are enough for this use case, tested it and saw no problems.

Unfortunately it seems you must use compound pages to keep track of  the page order.
put_page assumes that the order of a non-compound page is always 0. 
It would be nice to document it somewhere but I'm not sure where.

Thanks,
Ilya

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> Sent: Thursday, August 04, 2016 4:25 PM
> To: Ilya Lesokhin <ilyal@...lanox.com>
> Cc: netdev@...r.kernel.org; edumazet@...gle.com
> Subject: Re: [PATCH] net: use non-compound pages in frag allocator
> 
> On Thu, 2016-08-04 at 15:47 +0300, Ilya Lesokhin wrote:
> > The code currently always references the first page in the frag
> > therefore there is no need to pay the extra overhead of making the
> > frag page compound
> >
> > Signed-off-by: Ilya Lesokhin <ilyal@...lanox.com>
> > ---
> >  net/core/sock.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c index 08bf97e..77432ba
> > 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -1993,7 +1993,7 @@ bool skb_page_frag_refill(unsigned int sz, struct
> page_frag *pfrag, gfp_t gfp)
> >  	if (SKB_FRAG_PAGE_ORDER) {
> >  		/* Avoid direct reclaim but allow kswapd to wake */
> >  		pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM)
> |
> > -					  __GFP_COMP | __GFP_NOWARN |
> > +					  __GFP_NOWARN |
> >  					  __GFP_NORETRY,
> >  					  SKB_FRAG_PAGE_ORDER);
> >  		if (likely(pfrag->page)) {
> 
> 
> Looks very risky to me.
> 
> What numbers can you show us exactly ?
> 
> splice() is known to play games on pages, make sure it still works ?
> 
>