crypto: skcipher_walk cleanups

PATCH Superseded Eric Biggers 2 participants 12 messages Jan 5, 2025 – Jan 15, 2025 Crypto API

lore

Share & tools

lore

Eric Biggers

author

2025-01-05 19:34 UTC #0 lore raw

v3 cover crypto: skcipher_walk cleanups

This series cleans up and optimizes some of the skcipher_walk code.

I've split this out from my original series
"crypto: scatterlist handling improvements"
(https://lore.kernel.org/linux-crypto/[email protected]/).
Please consider applying this smaller set for 6.14, and we can do patches 11-29 of the original series later.

Other changes in v3:
- Added comments in the patch
"crypto: skcipher - optimize initializing skcipher_walk fields"

Eric Biggers (8):
crypto: skcipher - document skcipher_walk_done() and rename some vars
crypto: skcipher - remove unnecessary page alignment of bounce buffer
crypto: skcipher - remove redundant clamping to page size
crypto: skcipher - remove redundant check for SKCIPHER_WALK_SLOW
crypto: skcipher - fold skcipher_walk_skcipher() into
skcipher_walk_virt()
crypto: skcipher - clean up initialization of skcipher_walk::flags
crypto: skcipher - optimize initializing skcipher_walk fields
crypto: skcipher - call cond_resched() directly

 crypto/skcipher.c                  | 206 +++++++++++++----------------
 include/crypto/internal/skcipher.h |   2 +-
 2 files changed, 90 insertions(+), 118 deletions(-)

base-commit: 7fa4817340161a34d5b4ca39e96d6318d37c1d3a

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #1 lore raw

PATCH v3 1/8 crypto: skcipher - document skcipher_walk_done() and rename some vars

From: Eric Biggers <[email protected]>

skcipher_walk_done() has an unusual calling convention, and some of its local variables have unclear names. Document it and rename variables to make it a bit clearer what is going on. No change in behavior.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c                  | 50 ++++++++++++++++++++----------
 include/crypto/internal/skcipher.h |  2 +-
 2 files changed, 35 insertions(+), 17 deletions(-)

crypto/skcipher.c +34 ~~-16~~

index d5fe0eca3826..8749c44f98a2 100644
@@ -87,21 +87,39 @@ static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
 	addr = skcipher_get_spot(addr, bsize);
 	scatterwalk_copychunks(addr, &walk->out, bsize, 1);
 	return 0;
 }
 
-int skcipher_walk_done(struct skcipher_walk *walk, int err)
+/**
+ * skcipher_walk_done() - finish one step of a skcipher_walk
+ * @walk: the skcipher_walk
+ * @res: number of bytes *not* processed (>= 0) from walk->nbytes,
+ *	 or a -errno value to terminate the walk due to an error
+ *
+ * This function cleans up after one step of walking through the source and
+ * destination scatterlists, and advances to the next step if applicable.
+ * walk->nbytes is set to the number of bytes available in the next step,
+ * walk->total is set to the new total number of bytes remaining, and
+ * walk->{src,dst}.virt.addr is set to the next pair of data pointers.  If there
+ * is no more data, or if an error occurred (i.e. -errno return), then
+ * walk->nbytes and walk->total are set to 0 and all resources owned by the
+ * skcipher_walk are freed.
+ *
+ * Return: 0 or a -errno value.  If @res was a -errno value then it will be
+ *	   returned, but other errors may occur too.
+ */
+int skcipher_walk_done(struct skcipher_walk *walk, int res)
 {
-	unsigned int n = walk->nbytes;
-	unsigned int nbytes = 0;
+	unsigned int n = walk->nbytes; /* num bytes processed this step */
+	unsigned int total = 0; /* new total remaining */
 
 	if (!n)
 		goto finish;
 
-	if (likely(err >= 0)) {
-		n -= err;
-		nbytes = walk->total - n;
+	if (likely(res >= 0)) {
+		n -= res; /* subtract num bytes *not* processed */
+		total = walk->total - n;
 	}
 
 	if (likely(!(walk->flags & (SKCIPHER_WALK_SLOW |
 				    SKCIPHER_WALK_COPY |
 				    SKCIPHER_WALK_DIFF)))) {

@@ -113,35 +131,35 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err)
 	} else if (walk->flags & SKCIPHER_WALK_COPY) {
 		skcipher_map_dst(walk);
 		memcpy(walk->dst.virt.addr, walk->page, n);
 		skcipher_unmap_dst(walk);
 	} else if (unlikely(walk->flags & SKCIPHER_WALK_SLOW)) {
-		if (err > 0) {
+		if (res > 0) {
 			/*
 			 * Didn't process all bytes.  Either the algorithm is
 			 * broken, or this was the last step and it turned out
 			 * the message wasn't evenly divisible into blocks but
 			 * the algorithm requires it.
 			 */
-			err = -EINVAL;
-			nbytes = 0;
+			res = -EINVAL;
+			total = 0;
 		} else
 			n = skcipher_done_slow(walk, n);
 	}
 
-	if (err > 0)
-		err = 0;
+	if (res > 0)
+		res = 0;
 
-	walk->total = nbytes;
+	walk->total = total;
 	walk->nbytes = 0;
 
 	scatterwalk_advance(&walk->in, n);
 	scatterwalk_advance(&walk->out, n);
-	scatterwalk_done(&walk->in, 0, nbytes);
-	scatterwalk_done(&walk->out, 1, nbytes);
+	scatterwalk_done(&walk->in, 0, total);
+	scatterwalk_done(&walk->out, 1, total);
 
-	if (nbytes) {
+	if (total) {
 		crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ?
 			     CRYPTO_TFM_REQ_MAY_SLEEP : 0);
 		return skcipher_walk_next(walk);
 	}

@@ -156,11 +174,11 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err)
 		kfree(walk->buffer);
 	if (walk->page)
 		free_page((unsigned long)walk->page);
 
 out:
-	return err;
+	return res;
 }
 EXPORT_SYMBOL_GPL(skcipher_walk_done);
 
 static int skcipher_next_slow(struct skcipher_walk *walk, unsigned int bsize)
 {

include/crypto/internal/skcipher.h +1 -1

index 08d1e8c63afc..4f49621d3eb6 100644
@@ -194,11 +194,11 @@ void crypto_unregister_lskcipher(struct lskcipher_alg *alg);
 int crypto_register_lskciphers(struct lskcipher_alg *algs, int count);
 void crypto_unregister_lskciphers(struct lskcipher_alg *algs, int count);
 int lskcipher_register_instance(struct crypto_template *tmpl,
 				struct lskcipher_instance *inst);
 
-int skcipher_walk_done(struct skcipher_walk *walk, int err);
+int skcipher_walk_done(struct skcipher_walk *walk, int res);
 int skcipher_walk_virt(struct skcipher_walk *walk,
 		       struct skcipher_request *req,
 		       bool atomic);
 int skcipher_walk_aead_encrypt(struct skcipher_walk *walk,
 			       struct aead_request *req, bool atomic);

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #2 lore raw

PATCH v3 2/8 crypto: skcipher - remove unnecessary page alignment of bounce buffer

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

In the slow path of skcipher_walk where it uses a slab bounce buffer for the data and/or IV, do not bother to avoid crossing a page boundary in the part(s) of this buffer that are used, and do not bother to allocate extra space in the buffer for that purpose. The buffer is accessed only by virtual address, so pages are irrelevant for it.

This logic may have been present due to the physical address support in skcipher_walk, but that has now been removed. Or it may have been present to be consistent with the fast path that currently does not hand back addresses that span pages, but that behavior is a side effect of the pages being "mapped" one by one and is not actually a requirement.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 62 ++++++++++++-----------------------------------
 1 file changed, 15 insertions(+), 47 deletions(-)

crypto/skcipher.c +15 ~~-47~~

index 8749c44f98a2..887cbce8f78d 100644
@@ -61,32 +61,20 @@ static inline void skcipher_unmap_dst(struct skcipher_walk *walk)
 static inline gfp_t skcipher_walk_gfp(struct skcipher_walk *walk)
 {
 	return walk->flags & SKCIPHER_WALK_SLEEP ? GFP_KERNEL : GFP_ATOMIC;
 }
 
-/* Get a spot of the specified length that does not straddle a page.
- * The caller needs to ensure that there is enough space for this operation.
- */
-static inline u8 *skcipher_get_spot(u8 *start, unsigned int len)
-{
-	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK);
-
-	return max(start, end_page);
-}
-
 static inline struct skcipher_alg *__crypto_skcipher_alg(
 	struct crypto_alg *alg)
 {
 	return container_of(alg, struct skcipher_alg, base);
 }
 
 static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
 {
-	u8 *addr;
+	u8 *addr = PTR_ALIGN(walk->buffer, walk->alignmask + 1);
 
-	addr = (u8 *)ALIGN((unsigned long)walk->buffer, walk->alignmask + 1);
-	addr = skcipher_get_spot(addr, bsize);
 	scatterwalk_copychunks(addr, &walk->out, bsize, 1);
 	return 0;
 }
 
 /**

@@ -181,37 +169,26 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
 EXPORT_SYMBOL_GPL(skcipher_walk_done);
 
 static int skcipher_next_slow(struct skcipher_walk *walk, unsigned int bsize)
 {
 	unsigned alignmask = walk->alignmask;
-	unsigned a;
 	unsigned n;
 	u8 *buffer;
 
 	if (!walk->buffer)
 		walk->buffer = walk->page;
 	buffer = walk->buffer;
-	if (buffer)
-		goto ok;
-
-	/* Start with the minimum alignment of kmalloc. */
-	a = crypto_tfm_ctx_alignment() - 1;
-	n = bsize;
-
-	/* Minimum size to align buffer by alignmask. */
-	n += alignmask & ~a;
-
-	/* Minimum size to ensure buffer does not straddle a page. */
-	n += (bsize - 1) & ~(alignmask | a);
-
-	buffer = kzalloc(n, skcipher_walk_gfp(walk));
-	if (!buffer)
-		return skcipher_walk_done(walk, -ENOMEM);
-	walk->buffer = buffer;
-ok:
+	if (!buffer) {
+		/* Min size for a buffer of bsize bytes aligned to alignmask */
+		n = bsize + (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
+
+		buffer = kzalloc(n, skcipher_walk_gfp(walk));
+		if (!buffer)
+			return skcipher_walk_done(walk, -ENOMEM);
+		walk->buffer = buffer;
+	}
 	walk->dst.virt.addr = PTR_ALIGN(buffer, alignmask + 1);
-	walk->dst.virt.addr = skcipher_get_spot(walk->dst.virt.addr, bsize);
 	walk->src.virt.addr = walk->dst.virt.addr;
 
 	scatterwalk_copychunks(walk->src.virt.addr, &walk->in, bsize, 0);
 
 	walk->nbytes = bsize;

@@ -294,34 +271,25 @@ static int skcipher_walk_next(struct skcipher_walk *walk)
 	return skcipher_next_fast(walk);
 }
 
 static int skcipher_copy_iv(struct skcipher_walk *walk)
 {
-	unsigned a = crypto_tfm_ctx_alignment() - 1;
 	unsigned alignmask = walk->alignmask;
 	unsigned ivsize = walk->ivsize;
-	unsigned bs = walk->stride;
-	unsigned aligned_bs;
+	unsigned aligned_stride = ALIGN(walk->stride, alignmask + 1);
 	unsigned size;
 	u8 *iv;
 
-	aligned_bs = ALIGN(bs, alignmask + 1);
-
-	/* Minimum size to align buffer by alignmask. */
-	size = alignmask & ~a;
-
-	size += aligned_bs + ivsize;
-
-	/* Minimum size to ensure buffer does not straddle a page. */
-	size += (bs - 1) & ~(alignmask | a);
+	/* Min size for a buffer of stride + ivsize, aligned to alignmask */
+	size = aligned_stride + ivsize +
+	       (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
 
 	walk->buffer = kmalloc(size, skcipher_walk_gfp(walk));
 	if (!walk->buffer)
 		return -ENOMEM;
 
-	iv = PTR_ALIGN(walk->buffer, alignmask + 1);
-	iv = skcipher_get_spot(iv, bs) + aligned_bs;
+	iv = PTR_ALIGN(walk->buffer, alignmask + 1) + aligned_stride;
 
 	walk->iv = memcpy(iv, walk->iv, walk->ivsize);
 	return 0;
 }

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #3 lore raw

PATCH v3 3/8 crypto: skcipher - remove redundant clamping to page size

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

In the case where skcipher_walk_next() allocates a bounce page, that page by definition has size PAGE_SIZE. The number of bytes to copy 'n' is guaranteed to fit in it, since earlier in the function it was clamped to be at most a page. Therefore remove the unnecessary logic that tried to clamp 'n' again to fit in the bounce page.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

crypto/skcipher.c +1 -5

index 887cbce8f78d..c627e267b125 100644
@@ -248,28 +248,24 @@ static int skcipher_walk_next(struct skcipher_walk *walk)
 			return skcipher_walk_done(walk, -EINVAL);
 
 slow_path:
 		return skcipher_next_slow(walk, bsize);
 	}
+	walk->nbytes = n;
 
 	if (unlikely((walk->in.offset | walk->out.offset) & walk->alignmask)) {
 		if (!walk->page) {
 			gfp_t gfp = skcipher_walk_gfp(walk);
 
 			walk->page = (void *)__get_free_page(gfp);
 			if (!walk->page)
 				goto slow_path;
 		}
-
-		walk->nbytes = min_t(unsigned, n,
-				     PAGE_SIZE - offset_in_page(walk->page));
 		walk->flags |= SKCIPHER_WALK_COPY;
 		return skcipher_next_copy(walk);
 	}
 
-	walk->nbytes = n;
-
 	return skcipher_next_fast(walk);
 }
 
 static int skcipher_copy_iv(struct skcipher_walk *walk)
 {

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #4 lore raw

PATCH v3 4/8 crypto: skcipher - remove redundant check for SKCIPHER_WALK_SLOW

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

In skcipher_walk_done(), remove the check for SKCIPHER_WALK_SLOW because it is always true. All other flags (and lack thereof) were checked earlier in the function, leaving SKCIPHER_WALK_SLOW as the only remaining possibility.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

crypto/skcipher.c +1 -1

index c627e267b125..98606def1bf9 100644
@@ -118,11 +118,11 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
 		goto unmap_src;
 	} else if (walk->flags & SKCIPHER_WALK_COPY) {
 		skcipher_map_dst(walk);
 		memcpy(walk->dst.virt.addr, walk->page, n);
 		skcipher_unmap_dst(walk);
-	} else if (unlikely(walk->flags & SKCIPHER_WALK_SLOW)) {
+	} else { /* SKCIPHER_WALK_SLOW */
 		if (res > 0) {
 			/*
 			 * Didn't process all bytes.  Either the algorithm is
 			 * broken, or this was the last step and it turned out
 			 * the message wasn't evenly divisible into blocks but

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #5 lore raw

PATCH v3 5/8 crypto: skcipher - fold skcipher_walk_skcipher() into skcipher_walk_virt()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Fold skcipher_walk_skcipher() into skcipher_walk_virt() which is its only remaining caller. No change in behavior.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

crypto/skcipher.c +8 ~~-15~~

index 98606def1bf9..17f4bc79ca8b 100644
@@ -304,23 +304,26 @@ static int skcipher_walk_first(struct skcipher_walk *walk)
 	walk->page = NULL;
 
 	return skcipher_walk_next(walk);
 }
 
-static int skcipher_walk_skcipher(struct skcipher_walk *walk,
-				  struct skcipher_request *req)
+int skcipher_walk_virt(struct skcipher_walk *walk,
+		       struct skcipher_request *req, bool atomic)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_alg *alg = crypto_skcipher_alg(tfm);
+	int err = 0;
+
+	might_sleep_if(req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP);
 
 	walk->total = req->cryptlen;
 	walk->nbytes = 0;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
 
 	if (unlikely(!walk->total))
-		return 0;
+		goto out;
 
 	scatterwalk_start(&walk->in, req->src);
 	scatterwalk_start(&walk->out, req->dst);
 
 	walk->flags &= ~SKCIPHER_WALK_SLEEP;

@@ -334,22 +337,12 @@ static int skcipher_walk_skcipher(struct skcipher_walk *walk,
 	if (alg->co.base.cra_type != &crypto_skcipher_type)
 		walk->stride = alg->co.chunksize;
 	else
 		walk->stride = alg->walksize;
 
-	return skcipher_walk_first(walk);
-}
-
-int skcipher_walk_virt(struct skcipher_walk *walk,
-		       struct skcipher_request *req, bool atomic)
-{
-	int err;
-
-	might_sleep_if(req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP);
-
-	err = skcipher_walk_skcipher(walk, req);
-
+	err = skcipher_walk_first(walk);
+out:
 	walk->flags &= atomic ? ~SKCIPHER_WALK_SLEEP : ~0;
 
 	return err;
 }
 EXPORT_SYMBOL_GPL(skcipher_walk_virt);

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #6 lore raw

PATCH v3 6/8 crypto: skcipher - clean up initialization of skcipher_walk::flags

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

- Initialize SKCIPHER_WALK_SLEEP in a consistent way, and check for atomic=true at the same time as CRYPTO_TFM_REQ_MAY_SLEEP. Technically atomic=true only needs to apply after the first step, but it is very rarely used. We should optimize for the common case. So, check 'atomic' alongside CRYPTO_TFM_REQ_MAY_SLEEP. This is more efficient.

- Initialize flags other than SKCIPHER_WALK_SLEEP to 0 rather than preserving them. No caller actually initializes the flags, which makes it impossible to use their original values for anything. Indeed, that does not happen and all meaningful flags get overridden anyway. It may have been thought that just clearing one flag would be faster than clearing all flags, but that's not the case as the former is a read-write operation whereas the latter is just a write.

- Move the explicit clearing of SKCIPHER_WALK_SLOW, SKCIPHER_WALK_COPY, and SKCIPHER_WALK_DIFF into skcipher_walk_done(), since it is now only needed on non-first steps.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

crypto/skcipher.c +13 ~~-26~~

index 17f4bc79ca8b..e54d1ad46566 100644
@@ -146,10 +146,12 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
 	scatterwalk_done(&walk->out, 1, total);
 
 	if (total) {
 		crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ?
 			     CRYPTO_TFM_REQ_MAY_SLEEP : 0);
+		walk->flags &= ~(SKCIPHER_WALK_SLOW | SKCIPHER_WALK_COPY |
+				 SKCIPHER_WALK_DIFF);
 		return skcipher_walk_next(walk);
 	}
 
 finish:
 	/* Short-circuit for the common/fast path. */

@@ -233,13 +235,10 @@ static int skcipher_next_fast(struct skcipher_walk *walk)
 static int skcipher_walk_next(struct skcipher_walk *walk)
 {
 	unsigned int bsize;
 	unsigned int n;
 
-	walk->flags &= ~(SKCIPHER_WALK_SLOW | SKCIPHER_WALK_COPY |
-			 SKCIPHER_WALK_DIFF);
-
 	n = walk->total;
 	bsize = min(walk->stride, max(n, walk->blocksize));
 	n = scatterwalk_clamp(&walk->in, n);
 	n = scatterwalk_clamp(&walk->out, n);

@@ -309,55 +308,53 @@ static int skcipher_walk_first(struct skcipher_walk *walk)
 int skcipher_walk_virt(struct skcipher_walk *walk,
 		       struct skcipher_request *req, bool atomic)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_alg *alg = crypto_skcipher_alg(tfm);
-	int err = 0;
 
 	might_sleep_if(req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP);
 
 	walk->total = req->cryptlen;
 	walk->nbytes = 0;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
+	if ((req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) && !atomic)
+		walk->flags = SKCIPHER_WALK_SLEEP;
+	else
+		walk->flags = 0;
 
 	if (unlikely(!walk->total))
-		goto out;
+		return 0;
 
 	scatterwalk_start(&walk->in, req->src);
 	scatterwalk_start(&walk->out, req->dst);
 
-	walk->flags &= ~SKCIPHER_WALK_SLEEP;
-	walk->flags |= req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
-		       SKCIPHER_WALK_SLEEP : 0;
-
 	walk->blocksize = crypto_skcipher_blocksize(tfm);
 	walk->ivsize = crypto_skcipher_ivsize(tfm);
 	walk->alignmask = crypto_skcipher_alignmask(tfm);
 
 	if (alg->co.base.cra_type != &crypto_skcipher_type)
 		walk->stride = alg->co.chunksize;
 	else
 		walk->stride = alg->walksize;
 
-	err = skcipher_walk_first(walk);
-out:
-	walk->flags &= atomic ? ~SKCIPHER_WALK_SLEEP : ~0;
-
-	return err;
+	return skcipher_walk_first(walk);
 }
 EXPORT_SYMBOL_GPL(skcipher_walk_virt);
 
 static int skcipher_walk_aead_common(struct skcipher_walk *walk,
 				     struct aead_request *req, bool atomic)
 {
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
-	int err;
 
 	walk->nbytes = 0;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
+	if ((req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) && !atomic)
+		walk->flags = SKCIPHER_WALK_SLEEP;
+	else
+		walk->flags = 0;
 
 	if (unlikely(!walk->total))
 		return 0;
 
 	scatterwalk_start(&walk->in, req->src);

@@ -367,26 +364,16 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk,
 	scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2);
 
 	scatterwalk_done(&walk->in, 0, walk->total);
 	scatterwalk_done(&walk->out, 0, walk->total);
 
-	if (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP)
-		walk->flags |= SKCIPHER_WALK_SLEEP;
-	else
-		walk->flags &= ~SKCIPHER_WALK_SLEEP;
-
 	walk->blocksize = crypto_aead_blocksize(tfm);
 	walk->stride = crypto_aead_chunksize(tfm);
 	walk->ivsize = crypto_aead_ivsize(tfm);
 	walk->alignmask = crypto_aead_alignmask(tfm);
 
-	err = skcipher_walk_first(walk);
-
-	if (atomic)
-		walk->flags &= ~SKCIPHER_WALK_SLEEP;
-
-	return err;
+	return skcipher_walk_first(walk);
 }
 
 int skcipher_walk_aead_encrypt(struct skcipher_walk *walk,
 			       struct aead_request *req, bool atomic)
 {

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #7 lore raw

PATCH v3 7/8 crypto: skcipher - optimize initializing skcipher_walk fields

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

The helper functions like crypto_skcipher_blocksize() take in a pointer to a tfm object, but they actually return properties of the algorithm. As the Linux kernel is compiled with -fno-strict-aliasing, the compiler has to assume that the writes to struct skcipher_walk could clobber the tfm's pointer to its algorithm. Thus it gets repeatedly reloaded in the generated code. Therefore, replace the use of these helper functions with staightforward accesses to the struct fields.

Note that while *users* of the skcipher and aead APIs are supposed to use the helper functions, this particular code is part of the API *implementation* in crypto/skcipher.c, which already accesses the algorithm struct directly in many cases. So there is no reason to prefer the helper functions here.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

crypto/skcipher.c +20 ~~-10~~

index e54d1ad46566..6b62d816f08d 100644
@@ -306,12 +306,12 @@ static int skcipher_walk_first(struct skcipher_walk *walk)
 }
 
 int skcipher_walk_virt(struct skcipher_walk *walk,
 		       struct skcipher_request *req, bool atomic)
 {
-	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
-	struct skcipher_alg *alg = crypto_skcipher_alg(tfm);
+	const struct skcipher_alg *alg =
+		crypto_skcipher_alg(crypto_skcipher_reqtfm(req));
 
 	might_sleep_if(req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP);
 
 	walk->total = req->cryptlen;
 	walk->nbytes = 0;

@@ -326,13 +326,18 @@ int skcipher_walk_virt(struct skcipher_walk *walk,
 		return 0;
 
 	scatterwalk_start(&walk->in, req->src);
 	scatterwalk_start(&walk->out, req->dst);
 
-	walk->blocksize = crypto_skcipher_blocksize(tfm);
-	walk->ivsize = crypto_skcipher_ivsize(tfm);
-	walk->alignmask = crypto_skcipher_alignmask(tfm);
+	/*
+	 * Accessing 'alg' directly generates better code than using the
+	 * crypto_skcipher_blocksize() and similar helper functions here, as it
+	 * prevents the algorithm pointer from being repeatedly reloaded.
+	 */
+	walk->blocksize = alg->base.cra_blocksize;
+	walk->ivsize = alg->co.ivsize;
+	walk->alignmask = alg->base.cra_alignmask;
 
 	if (alg->co.base.cra_type != &crypto_skcipher_type)
 		walk->stride = alg->co.chunksize;
 	else
 		walk->stride = alg->walksize;

@@ -342,11 +347,11 @@ int skcipher_walk_virt(struct skcipher_walk *walk,
 EXPORT_SYMBOL_GPL(skcipher_walk_virt);
 
 static int skcipher_walk_aead_common(struct skcipher_walk *walk,
 				     struct aead_request *req, bool atomic)
 {
-	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
+	const struct aead_alg *alg = crypto_aead_alg(crypto_aead_reqtfm(req));
 
 	walk->nbytes = 0;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
 	if ((req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) && !atomic)

@@ -364,14 +369,19 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk,
 	scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2);
 
 	scatterwalk_done(&walk->in, 0, walk->total);
 	scatterwalk_done(&walk->out, 0, walk->total);
 
-	walk->blocksize = crypto_aead_blocksize(tfm);
-	walk->stride = crypto_aead_chunksize(tfm);
-	walk->ivsize = crypto_aead_ivsize(tfm);
-	walk->alignmask = crypto_aead_alignmask(tfm);
+	/*
+	 * Accessing 'alg' directly generates better code than using the
+	 * crypto_aead_blocksize() and similar helper functions here, as it
+	 * prevents the algorithm pointer from being repeatedly reloaded.
+	 */
+	walk->blocksize = alg->base.cra_blocksize;
+	walk->stride = alg->chunksize;
+	walk->ivsize = alg->ivsize;
+	walk->alignmask = alg->base.cra_alignmask;
 
 	return skcipher_walk_first(walk);
 }
 
 int skcipher_walk_aead_encrypt(struct skcipher_walk *walk,

Signature · 1 line

2.47.1

Eric Biggers

author

2025-01-05 19:34 UTC #8 lore raw

PATCH v3 8/8 crypto: skcipher - call cond_resched() directly

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

In skcipher_walk_done(), instead of calling crypto_yield() which requires a translation between flags, just call cond_resched() directly. This has the same effect.

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/skcipher.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

crypto/skcipher.c +2 -2

index 6b62d816f08d..a9eb2dcf2898 100644
@@ -144,12 +144,12 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
 	scatterwalk_advance(&walk->out, n);
 	scatterwalk_done(&walk->in, 0, total);
 	scatterwalk_done(&walk->out, 1, total);
 
 	if (total) {
-		crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ?
-			     CRYPTO_TFM_REQ_MAY_SLEEP : 0);
+		if (walk->flags & SKCIPHER_WALK_SLEEP)
+			cond_resched();
 		walk->flags &= ~(SKCIPHER_WALK_SLOW | SKCIPHER_WALK_COPY |
 				 SKCIPHER_WALK_DIFF);
 		return skcipher_walk_next(walk);
 	}

Signature · 1 line

2.47.1

Herbert Xu

maintainer

2025-01-14 03:46 UTC #9 lore raw

In reply to Eric Biggers

Quoted 23 lines · Eric Biggers

This series cleans up and optimizes some of the skcipher_walk code.
I've split this out from my original series
"crypto: scatterlist handling improvements"
(https://lore.kernel.org/linux-crypto/[email protected]/).
Please consider applying this smaller set for 6.14, and we can do patches 11-29 of the original series later.
Other changes in v3:
- Added comments in the patch
"crypto: skcipher - optimize initializing skcipher_walk fields"
Eric Biggers (8):
crypto: skcipher - document skcipher_walk_done() and rename some vars
crypto: skcipher - remove unnecessary page alignment of bounce buffer
crypto: skcipher - remove redundant clamping to page size
crypto: skcipher - remove redundant check for SKCIPHER_WALK_SLOW
crypto: skcipher - fold skcipher_walk_skcipher() into
skcipher_walk_virt()
crypto: skcipher - clean up initialization of skcipher_walk::flags
crypto: skcipher - optimize initializing skcipher_walk fields
crypto: skcipher - call cond_resched() directly

Quoted 6 lines

crypto/skcipher.c | 206 +++++++++++++----------------
include/crypto/internal/skcipher.h | 2 +-
2 files changed, 90 insertions(+), 118 deletions(-)
base-commit: 7fa4817340161a34d5b4ca39e96d6318d37c1d3a

All applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Biggers

author

2025-01-15 02:40 UTC #10 lore raw

Hi Herbert,

Quoted 24 lines · Herbert Xu

Quoted 23 lines · Eric Biggers
This series cleans up and optimizes some of the skcipher_walk code.
I've split this out from my original series
"crypto: scatterlist handling improvements"
(https://lore.kernel.org/linux-crypto/[email protected]/).
Please consider applying this smaller set for 6.14, and we can do patches 11-29 of the original series later.
Other changes in v3:
- Added comments in the patch
"crypto: skcipher - optimize initializing skcipher_walk fields"
Eric Biggers (8):
crypto: skcipher - document skcipher_walk_done() and rename some vars
crypto: skcipher - remove unnecessary page alignment of bounce buffer
crypto: skcipher - remove redundant clamping to page size
crypto: skcipher - remove redundant check for SKCIPHER_WALK_SLOW
crypto: skcipher - fold skcipher_walk_skcipher() into
skcipher_walk_virt()
crypto: skcipher - clean up initialization of skcipher_walk::flags
crypto: skcipher - optimize initializing skcipher_walk fields
crypto: skcipher - call cond_resched() directly

Quoted 9 lines

Quoted 6 lines
crypto/skcipher.c | 206 +++++++++++++----------------
include/crypto/internal/skcipher.h | 2 +-
2 files changed, 90 insertions(+), 118 deletions(-)
base-commit: 7fa4817340161a34d5b4ca39e96d6318d37c1d3a
All applied. Thanks.

Did you forget to push this out?

- Eric

Herbert Xu

maintainer

2025-01-15 04:42 UTC #11 lore raw

Quoted 2 lines · Eric Biggers

Did you forget to push this out?

Oops I did indeed. It should be there now.

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt