Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221213161310.2205802-1-ardb@kernel.org> <Y5jXnE8e2SRHFRQN@sol.localdomain>
In-Reply-To: <Y5jXnE8e2SRHFRQN@sol.localdomain>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 13 Dec 2022 21:58:45 +0100
Message-ID: <CAMj1kXFKhoLMMW8R8UbWrUf6W3xYBwzeD+w0mQwBn_=3qJJ+dA@mail.gmail.com>
Subject: Re: [RFC PATCH] crypto: use kmap_local() not kmap_atomic()
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "Elliott, Robert (Servers)" <elliott@hpe.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 13 Dec 2022 at 20:50, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Dec 13, 2022 at 05:13:10PM +0100, Ard Biesheuvel wrote:
> > kmap_atomic() is used to create short-lived mappings of pages that may
> > not be accessible via the kernel direct map. This is only needed on
> > 32-bit architectures that implement CONFIG_HIGHMEM, but it can be used
> > on 64-bit other architectures too, where the returned mapping is simply
> > the kernel direct address of the page.
> >
> > However, kmap_atomic() does not support migration on CONFIG_HIGHMEM
> > configurations, due to the use of per-CPU kmap slots, and so it disables
> > preemption on all architectures, not just the 32-bit ones. This implies
> > that all scatterwalk based crypto routines essentially execute with
> > preemption disabled all the time, which is less than ideal.
> >
> > So let's switch scatterwalk_map/_unmap and the shash/ahash routines to
> > kmap_local() instead, which serves a similar purpose, but without the
> > resulting impact on preemption on architectures that have no need for
> > CONFIG_HIGHMEM.
> >
> > Cc: Eric Biggers <ebiggers@kernel.org>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: "Elliott, Robert (Servers)" <elliott@hpe.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  crypto/ahash.c               | 4 ++--
> >  crypto/shash.c               | 4 ++--
> >  include/crypto/scatterwalk.h | 4 ++--
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> >
>
> Thanks Ard, this looks good to me, especially given the broader effort to
> replace kmap_atomic() with kmap_local_page() in the kernel.
>
> One question: should the kmap_atomic() in crypto/skcipher.c be replaced as well?
>

Yeah, probably, but that one does not actually affect 64-bit at the
moment, so it matters less.