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;
Date:   Tue, 13 Dec 2022 11:50:52 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "Elliott, Robert (Servers)" <elliott@hpe.com>
Subject: Re: [RFC PATCH] crypto: use kmap_local() not kmap_atomic()
Message-ID: <Y5jXnE8e2SRHFRQN@sol.localdomain>
References: <20221213161310.2205802-1-ardb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221213161310.2205802-1-ardb@kernel.org>
Precedence: bulk

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?

- Eric