Re: [PATCH] crypto: skcipher - Elinimate duplicate virt.addr field

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Mar 06, 2025 at 02:07:50PM +0800, Herbert Xu wrote:
> On Wed, Mar 05, 2025 at 07:59:37PM -0800, Eric Biggers wrote:
> >
> > I don't think it will be quite that simple, since the skcipher_walk code relies
> > on the different parts being split up so that it can do things like calculate
> > the length before it starts mapping anything.  If you can make it work, we can
> > do that.  But until that additional patch is ready I don't think it makes sense
> > to merge this one, as it leaves things half-baked with the redundant pointers.
> 
> Sure, fixing it might not be easy, partly because the new interface
> wasn't designed for its needs.
> 
> But getting rid of the duplicate field isn't hard, because we're
> already assuming that the user does not modify walk->XXX.virt.addr,
> at least not far enough to break the unmap (see the WALK_DIFF
> clause).  In fact, grepping through the arch code seems to show
> that nobody actually modifies them at all.  So we could even
> simplify the WALK_SLOW done path.
> 
> ---8<---
> Reuse the addr field from struct scatter_walk for skcipher_walk.
> In order to maintain backwards compatibility with existing users,
> retain the original virt.addr fields through unions.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  crypto/skcipher.c                  | 25 ++++++++++---------------
>  include/crypto/algapi.h            |  3 ++-
>  include/crypto/internal/skcipher.h | 20 +++++++++++++++-----
>  3 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/crypto/skcipher.c b/crypto/skcipher.c
> index d321c8746950..f770307abb8e 100644
> --- a/crypto/skcipher.c
> +++ b/crypto/skcipher.c
> @@ -43,14 +43,12 @@ static inline void skcipher_map_src(struct skcipher_walk *walk)
>  {
>  	/* XXX */
>  	walk->in.addr = scatterwalk_map(&walk->in);
> -	walk->src.virt.addr = walk->in.addr;
>  }
>  
>  static inline void skcipher_map_dst(struct skcipher_walk *walk)
>  {
>  	/* XXX */
>  	walk->out.addr = scatterwalk_map(&walk->out);
> -	walk->dst.virt.addr = walk->out.addr;
>  }
>  
>  static inline gfp_t skcipher_walk_gfp(struct skcipher_walk *walk)
> @@ -100,8 +98,7 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
>  				    SKCIPHER_WALK_DIFF)))) {
>  		scatterwalk_advance(&walk->in, n);
>  	} else if (walk->flags & SKCIPHER_WALK_DIFF) {
> -		scatterwalk_unmap(walk->src.virt.addr);
> -		scatterwalk_advance(&walk->in, n);
> +		scatterwalk_done_src(&walk->in, n);
>  	} else if (walk->flags & SKCIPHER_WALK_COPY) {
>  		scatterwalk_advance(&walk->in, n);
>  		skcipher_map_dst(walk);
> @@ -116,11 +113,8 @@ int skcipher_walk_done(struct skcipher_walk *walk, int res)
>  			 */
>  			res = -EINVAL;
>  			total = 0;
> -		} else {
> -			u8 *buf = PTR_ALIGN(walk->buffer, walk->alignmask + 1);
> -
> -			memcpy_to_scatterwalk(&walk->out, buf, n);
> -		}
> +		} else
> +			memcpy_to_scatterwalk(&walk->out, walk->out.addr, n);
>  		goto dst_done;
>  	}
>  
> @@ -176,10 +170,11 @@ static int skcipher_next_slow(struct skcipher_walk *walk, unsigned int bsize)
>  			return skcipher_walk_done(walk, -ENOMEM);
>  		walk->buffer = buffer;
>  	}
> -	walk->dst.virt.addr = PTR_ALIGN(buffer, alignmask + 1);
> -	walk->src.virt.addr = walk->dst.virt.addr;
>  
> -	memcpy_from_scatterwalk(walk->src.virt.addr, &walk->in, bsize);
> +	buffer = PTR_ALIGN(buffer, alignmask + 1);
> +	memcpy_from_scatterwalk(buffer, &walk->in, bsize);
> +	walk->out.addr = buffer;
> +	walk->in.addr = walk->out.addr;
>  
>  	walk->nbytes = bsize;
>  	walk->flags |= SKCIPHER_WALK_SLOW;
> @@ -199,8 +194,8 @@ static int skcipher_next_copy(struct skcipher_walk *walk)
>  	 * processed (which might be less than walk->nbytes) is known.
>  	 */
>  
> -	walk->src.virt.addr = tmp;
> -	walk->dst.virt.addr = tmp;
> +	walk->in.addr = tmp;
> +	walk->out.addr = tmp;
>  	return 0;
>  }
>  
> @@ -214,7 +209,7 @@ static int skcipher_next_fast(struct skcipher_walk *walk)
>  		(u8 *)(sg_page(walk->out.sg) + (walk->out.offset >> PAGE_SHIFT));
>  
>  	skcipher_map_src(walk);
> -	walk->dst.virt.addr = walk->src.virt.addr;
> +	walk->out.addr = walk->in.addr;
>  
>  	if (diff) {
>  		walk->flags |= SKCIPHER_WALK_DIFF;
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 41733a0b45dd..94147ea8c14d 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -120,9 +120,10 @@ struct crypto_queue {
>  };
>  
>  struct scatter_walk {
> +	/* Must be the first member, see struct skcipher_walk. */
> +	void *addr;
>  	struct scatterlist *sg;
>  	unsigned int offset;
> -	void *addr;
>  };
>  
>  struct crypto_attr_alg {
> diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
> index d6ae7a86fed2..357441b56c1e 100644
> --- a/include/crypto/internal/skcipher.h
> +++ b/include/crypto/internal/skcipher.h
> @@ -57,14 +57,24 @@ struct crypto_lskcipher_spawn {
>  struct skcipher_walk {
>  	union {
>  		struct {
> -			void *addr;
> -		} virt;
> -	} src, dst;
> +			struct {
> +				void *const addr;
> +			} virt;
> +		} src;
> +		struct scatter_walk in;
> +	};
>  
> -	struct scatter_walk in;
>  	unsigned int nbytes;
>  
> -	struct scatter_walk out;
> +	union {
> +		struct {
> +			struct {
> +				void *const addr;
> +			} virt;
> +		} dst;
> +		struct scatter_walk out;
> +	};
> +

Those unions are ugly, but I guess this is good enough.  Please also delete the
/* XXX */ comments, fix the typo in the title, and resend this as a real patch.
Thanks!

- Eric