Re: [PATCH v5 7/8] crypto: arm64/polyval: Add PMULL accelerated implementation of POLYVAL

Eric Biggers
2022-05-02T18:11:37Z

On Sun, May 01, 2022 at 01:21:52PM -0700, Eric Biggers wrote:
> > +static int polyval_arm64_update(struct shash_desc *desc,
> > + const u8 *src, unsigned int srclen)
> > +{
> > + struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> > + struct polyval_tfm_ctx *ctx = crypto_shash_ctx(desc->tfm);
> > + u8 *pos;
> > + unsigned int nblocks;
> > + unsigned int n;
> > +
> > + if (dctx->bytes) {
> > + n = min(srclen, dctx->bytes);
> > + pos = dctx->buffer + POLYVAL_BLOCK_SIZE - dctx->bytes;
> > +
> > + dctx->bytes -= n;
> > + srclen -= n;
> > +
> > + while (n--)
> > + *pos++ ^= *src++;
> > +
> > + if (!dctx->bytes)
> > + internal_polyval_mul(dctx->buffer,
> > + ctx->key_powers[NUM_KEY_POWERS-1]);
> > + }
> > +
> > + nblocks = srclen/POLYVAL_BLOCK_SIZE;
> > + internal_polyval_update(ctx, src, nblocks, dctx->buffer);
> > + srclen -= nblocks*POLYVAL_BLOCK_SIZE;
>
> This is executing a kernel_neon_begin()/kernel_neon_end() section of unbounded
> length. To allow the task to be preempted occasionally, it needs to handle the
> input in chunks, e.g. 4K at a time, like the existing code for other algorithms
> does. Something like the following would work:
>
> @@ -122,13 +118,16 @@ static int polyval_arm64_update(struct shash_desc *desc,
> ctx->key_powers[NUM_KEY_POWERS-1]);
> }
>
> - nblocks = srclen/POLYVAL_BLOCK_SIZE;
> - internal_polyval_update(ctx, src, nblocks, dctx->buffer);
> - srclen -= nblocks*POLYVAL_BLOCK_SIZE;
> + while (srclen >= POLYVAL_BLOCK_SIZE) {
> + /* Allow rescheduling every 4K bytes. */
> + nblocks = min(srclen, 4096U) / POLYVAL_BLOCK_SIZE;
> + internal_polyval_update(ctx, src, nblocks, dctx->buffer);
> + srclen -= nblocks * POLYVAL_BLOCK_SIZE;
> + src += nblocks * POLYVAL_BLOCK_SIZE;
> + }
>
> if (srclen) {
> dctx->bytes = POLYVAL_BLOCK_SIZE - srclen;
> - src += nblocks*POLYVAL_BLOCK_SIZE;
> pos = dctx->buffer;
> while (srclen--)
> *pos++ ^= *src++;
>

Also to be clear, this problem is specific to the "shash" API. You don't need
to worry about it for "skcipher" algorithms such as xctr(*), as they have to
walk a scatterlist to get their data, and that happens a page at a time.

- Eric