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@xxxxxxxxxxxxxxxxxxx>
> ---
> 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
