linux-kernel - Re: [PATCH net-next v3 01/18] net: Copy slab data for sendmsg(MSG_SPLICE

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: <6cf2ea121c4fdbd04682224c5acf6c73cc47f2f7.camel@redhat.com>
Date:   Fri, 23 Jun 2023 11:37:12 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     David Howells <dhowells@...hat.com>
Cc:     netdev@...r.kernel.org,
        Alexander Duyck <alexander.duyck@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        David Ahern <dsahern@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Jens Axboe <axboe@...nel.dk>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Menglong Dong <imagedong@...cent.com>
Subject: Re: [PATCH net-next v3 01/18] net: Copy slab data for
 sendmsg(MSG_SPLICE_PAGES)

On Fri, 2023-06-23 at 10:06 +0100, David Howells wrote:
> Paolo Abeni <pabeni@...hat.com> wrote:
> 
> > IMHO this function uses a bit too much labels and would be more easy to
> > read, e.g. moving the above chunk of code in conditional branch.
> 
> Maybe.  I was trying to put the fast path up at the top without the slow path
> bits in it, but I can put the "insufficient_space" bit there.

I *think* you could move the insufficient_space in a separate helped,
that should achieve your goal with fewer labels and hopefully no
additional complexity.

> 
> > Even without such change, I think the above 'goto try_again;'
> > introduces an unneeded conditional, as at this point we know 'fragsz <=
> > fsize'.
> 
> Good point.
> 
> > > +		cache->pfmemalloc = folio_is_pfmemalloc(spare);
> > > +		if (cache->folio)
> > > +			goto reload;
> > 
> > I think there is some problem with the above.
> > 
> > If cache->folio is != NULL, and cache->folio was not pfmemalloc-ed
> > while the spare one is, it looks like the wrong policy will be used.
> > And should be even worse if folio was pfmemalloc-ed while spare is not.
> > 
> > I think moving 'cache->pfmemalloc' initialization...
> > 
> > > +	}
> > > +
> > 
> > ... here should fix the above.
> 
> Yeah.  We might have raced with someone else or been moved to another cpu and
> there might now be a folio we can allocate from.
> 
> > > +	/* Reset page count bias and offset to start of new frag */
> > > +	cache->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > > +	offset = folio_size(folio);
> > > +	goto try_again;
> > 
> > What if fragsz > PAGE_SIZE, we are consistently unable to allocate an
> > high order page, but order-0, pfmemalloc-ed page allocation is
> > successful? It looks like this could become an unbounded loop?
> 
> It shouldn't.  It should go:
> 
> 	try_again:
> 		if (fragsz > offset)
> 			goto insufficient_space;
> 	insufficient_space:
> 		/* See if we can refurbish the current folio. */
> 		...

I think the critical path is with pfmemalloc-ed pages:

		if (unlikely(cache->pfmemalloc)) {
			__folio_put(folio);
			goto get_new_folio;
		}

just before the following.

> 		fsize = folio_size(folio);
> 		if (unlikely(fragsz > fsize))
> 			goto frag_too_big;
> 	frag_too_big:
> 		...
> 		return NULL;
> 
> Though for safety's sake, it would make sense to put in a size check in the
> case we fail to allocate a larger-order folio.
> 
> > >  		do {
> > >  			struct page *page = pages[i++];
> > >  			size_t part = min_t(size_t, PAGE_SIZE - off, len);
> > > -
> > > -			ret = -EIO;
> > > -			if (WARN_ON_ONCE(!sendpage_ok(page)))
> > > +			bool put = false;
> > > +
> > > +			if (PageSlab(page)) {
> > 
> > I'm a bit concerned from the above. If I read correctly, tcp 0-copy
> 
> Well, splice()-to-tcp will; MSG_ZEROCOPY is unaffected.

Ah right! I got lost in some 'if' branch.

> > will go through that for every page, even if the expected use-case is
> > always !PageSlub(page). compound_head() could be costly if the head
> > page is not hot on cache and I'm not sure if that could be the case for
> > tcp 0-copy. The bottom line is that I fear a possible regression here.
> 
> I can put the PageSlab() check inside the sendpage_ok() so the page flag is
> only checked once.  

Perhaps I'm lost again, but AFAICS:

__PAGEFLAG(Slab, slab, PF_NO_TAIL)

// ...
#define __PAGEFLAG(uname, lname, policy)			\
	TESTPAGEFLAG(uname, lname, policy)			\
// ...

#define TESTPAGEFLAG(uname, lname, policy)				\
static __always_inline bool folio_test_##lname(struct folio *folio)	\
{ return test_bit(PG_##lname, folio_flags(folio, FOLIO_##policy));}	\
static __always_inline int Page##uname(struct page *page)               \
{ return test_bit(PG_##lname, &policy(page, 0)->flags); }
// ... 'policy' is PF_NO_TAIL here

#define PF_NO_TAIL(page, enforce) ({                                    \
                VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page);	\
                PF_POISONED_CHECK(compound_head(page)); })

It looks at compound_head in the end ?!?

> But PageSlab() doesn't check the headpage, only the page
> it is given.  sendpage_ok() is more the problem as it also calls
> page_count().  I could drop the check.

Once the head page is hot on cache due to the previous check, it should
be cheap?

Cheers,

Paolo