Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC_MINALIGN

From: Ard Biesheuvel <ardb@kernel.org>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux Memory Management List <linux-mm@kvack.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC_MINALIGN
Date: Tue, 19 Apr 2022 23:50:11 +0200	[thread overview]
Message-ID: <CAMj1kXEGdPageO3tb2=eLnGAR9-nZtmTGXcGf5CiTQFC4JiXOg@mail.gmail.com> (raw)
In-Reply-To: <Yl2Vda/8S7qAvMjC@arm.com>

On Mon, 18 Apr 2022 at 18:44, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Mon, Apr 18, 2022 at 04:37:17PM +0800, Herbert Xu wrote:
> > On Sun, Apr 17, 2022 at 05:30:27PM +0100, Catalin Marinas wrote:
> > > Do you mean as per Ard's proposal here:
> > >
> > > https://lore.kernel.org/r/CAMj1kXH0x5Va7Wgs+mU1ONDwwsazOBuN4z4ihVzO2uG-n41Kbg@mail.gmail.com
> > >
> > > struct crypto_request {
> > >     union {
> > >             struct {
> > >                     ... fields ...
> > >             };
> > >             u8 __padding[ARCH_DMA_MINALIGN];
> > >     };
> > >     void __ctx[]  __aligned(CRYPTO_MINALIGN);
> > > };
> > >
> > > If CRYPTO_MINALIGN is lowered to, say, 8 (to be the same as lowest
> > > ARCH_KMALLOC_MINALIGN), the __alignof__(req->__ctx) would be 8.
> > > Functions like crypto_tfm_ctx_alignment() will return 8 when what you
> > > need is 128. We can change those functions to return ARCH_DMA_MINALIGN
> > > instead or always bump cra_alignmask to ARCH_DMA_MINALIGN-1.
> >
> > OK, at this point I think we need to let the code do the talking :)
> >
> > I've seen Ard's patches already and I think I understand what your
> > needs are.  So let me whip up some code to show you guys what I
> > think needs to be done.
>
> BTW before you have a go at this, there's also Linus' idea that does not
> change the crypto code (at least not functionally). Of course, you and
> Ard can still try to figure out how to reduce the padding but if we go
> with Linus' idea of a new GFP_NODMA flag, there won't be any changes to
> the crypto code as long as it doesn't pass such flag. So, the options:
>
> 1. Change ARCH_KMALLOC_MINALIGN to 8 (or ARCH_SLAB_MINALIGN if higher)
>    while keeping ARCH_DMA_MINALIGN to 128. By default kmalloc() will
>    honour the 128-byte alignment, unless GDP_NODMA is passed. This still
>    requires changing CRYPTO_MINALIGN to ARCH_DMA_MINALIGN but there is
>    no functional change, kmalloc() without the new flag will return
>    CRYPTO_MINALIGN-aligned pointers.
>
> 2. Leave ARCH_KMALLOC_MINALIGN as ARCH_DMA_MINALIGN (128) and introduce
>    a new GFP_PACKED (I think it fits better than 'NODMA') flag that
>    reduces the minimum kmalloc() below ARCH_KMALLOC_MINALIGN (and
>    probably at least ARCH_SLAB_MINALIGN). It's equivalent to (1) but
>    does not touch the crypto code at all.
>
> (1) and (2) are the same, just minor naming difference. Happy to go with
> any of them. They still have the downside that we need to add the new
> GFP_ flag to those hotspots that allocate small objects (Arnd provided
> an idea on how to find them with ftrace) but at least we know it won't
> inadvertently break anything.
>

I'm not sure GFP_NODMA adds much here.

The way I see it, the issue in the crypto code is that we are relying
on a ARCH_KMALLOC_ALIGN aligned zero length __ctx[] array for three
different things:
- ensuring/informing the compiler that top-level request/TFM
structures are aligned to ARCH_KMALLOC_ALIGN,
- adding padding to ensure that driver context structures that are
embedded in those top-level request/TFM structures are sufficiently
aligned so that any member C types appear at the expected alignment
(but those structures are not usually defined as being aligned to
ARCH_KMALLOC_ALIGN)
- adding padding to ensure that these driver context structures do not
share cache lines with the preceding top-level struct.

One thing to note here is that the padding is only necessary when the
driver context size > 0, and has nothing to do with the alignment of
the top-level struct.

Using a single aligned ctx member was a nice way to accomplish all of
these when it was introduced, but I think it might be better to get
rid of it, and move the padding logic to the static inline helpers
instead.

So something like

struct skcipher_request {
  ...
} CRYPTO_MINALIGN_ATTR;

which states/ensures the alignment of the struct, and

void *skcipher_request_ctx(struct skcipher_request *req) {
  return (void *)PTR_ALIGN(req + 1, ARCH_DMA_MINALIGN);
}

to get at the context struct, instead of using a struct field.

Then, we could update skcipher_request_alloc() to only round up
sizeof(struct skipher_request) to ARCH_DMA_MINALIGN if the reqsize is
>0 to begin with, and if it is, to also round reqsize up to
ARCH_DMA_MINALIGN when accessed. That way, we spell out the DMA
padding requirements with relying on aligned struct members.

If we do it this way, we have a clear distinction between expectations
about what kmalloc returns in terms of alignment, and adding padding
to influence the placement of the context struct. It also makes it
easier to either apply the changes I proposed in the series I sent out
a couple of weeks ago, or get rid of DMA alignment for request/TFM
structures altogether, if we manage to identify and fix the drivers
that are relying on this. In any case, it decouples these two things
in a way that allows Catalin to make his kmalloc changes without
having to redefine CRYPTO_MINALIGN to ARCH_DMA_MINALIGN.

-- 
Ard.