[07/29] crypto: skcipher - optimize initializing skcipher_walk fields

From: Eric Biggers <ebiggers@google.com>

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.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/skcipher.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

On Sat, Dec 21, 2024 at 07:16:07PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > @@ -326,13 +326,13 @@ 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);
> > +       walk->blocksize = alg->base.cra_blocksize;
> > +       walk->ivsize = alg->co.ivsize;
> > +       walk->alignmask = alg->base.cra_alignmask;
> 
> Please do this instead:
> 
> 	unsigned bs, ivs, am;
> 
> 	bs = crypto_skcipher_blocksize(tfm);
> 	ivs = crypto_skcipher_ivsize(tfm);
> 	am = crypto_skcipher_alignmask(tfm);
> 	walk->blocksize = bs;
> 	walk->ivsize = ivs;
> 	walk->alignmask = am;
> 
> This generates the right thing for me with gcc12.
> 

This seems strictly worse than my version, so I don't plan to do this.  It's
more lines of code, and it causes an extra push and pop to be needed in
skcipher_walk_virt() to free up enough registers to hold all values at once.  It
may be intended that API users are supposed to use the helper functions instead
of accessing the algorithm struct directly, but this code is not a user; it's
part of the API implementation in crypto/skcipher.c.  There are already lots of
other direct accesses to the algorithm struct in the same file, and even another
in skcipher_walk_virt() already.  The helper functions are pointless in this
context and just cause problems like the one this patch is fixing.

- Eric

On Sun, Dec 29, 2024 at 02:10:11PM -0800, Eric Biggers wrote:
>
> This seems strictly worse than my version, so I don't plan to do this.  It's
> more lines of code, and it causes an extra push and pop to be needed in
> skcipher_walk_virt() to free up enough registers to hold all values at once.  It
> may be intended that API users are supposed to use the helper functions instead
> of accessing the algorithm struct directly, but this code is not a user; it's
> part of the API implementation in crypto/skcipher.c.  There are already lots of
> other direct accesses to the algorithm struct in the same file, and even another
> in skcipher_walk_virt() already.  The helper functions are pointless in this
> context and just cause problems like the one this patch is fixing.

OK, but please add a comment where the accesses occur so that
people won't try to turn this back.

Thanks,