Re: [PATCH v4 06/33] crypto: rockchip: add fallback for cipher

[John Keeping <john@xxxxxxxxxxxx>]

On Fri, Apr 01, 2022 at 08:17:37PM +0000, Corentin Labbe wrote:
> The hardware does not handle 0 size length request, let's add a
> fallback.
> Furthermore fallback will be used for all unaligned case the hardware
> cannot handle.
> 
> Fixes: ce0183cb6464b ("crypto: rockchip - switch to skcipher API")
> Signed-off-by: Corentin Labbe <clabbe@xxxxxxxxxxxx>
> ---
> diff --git a/drivers/crypto/rockchip/rk3288_crypto_skcipher.c b/drivers/crypto/rockchip/rk3288_crypto_skcipher.c
> index bbd0bf52bf07..c6b601086c04 100644
> --- a/drivers/crypto/rockchip/rk3288_crypto_skcipher.c
> +++ b/drivers/crypto/rockchip/rk3288_crypto_skcipher.c
> @@ -13,6 +13,71 @@
>  
>  #define RK_CRYPTO_DEC			BIT(0)
>  
> +static int rk_cipher_need_fallback(struct skcipher_request *req)
> +{
> +	struct scatterlist *sgs, *sgd;
> +	unsigned int todo, len;
> +	unsigned int bs = crypto_skcipher_blocksize(tfm);
> +
> +	if (!req->cryptlen)
> +		return true;
> +
> +	len = req->cryptlen;
> +	sgs = req->src;
> +	while (sgs) {
> +		if (!IS_ALIGNED(sgs->offset, sizeof(u32))) {
> +			return true;
> +		}
> +		todo = min(len, sgs->length);
> +		if (todo % bs) {
> +			return true;
> +		}
> +		len -= todo;
> +		sgs = sg_next(sgs);
> +	}
> +	len = req->cryptlen;
> +	sgd = req->dst;
> +	while (sgd) {
> +		if (!IS_ALIGNED(sgd->offset, sizeof(u32))) {
> +			return true;
> +		}
> +		todo = min(len, sgd->length);
> +		if (todo % bs) {
> +			return true;
> +		}
> +		len -= todo;
> +		sgd = sg_next(sgd);
> +	}
> +	sgs = req->src;
> +	sgd = req->dst;
> +	while (sgs && sgd) {
> +		if (sgs->length != sgd->length)

This check still seems to be triggering the fallback when it is not
needed.

I've done some testing with fscrypt and the series is working great, but
the stats show the fallback triggering more than I'd expect.  With some
extra logging here I see output like:

	sgs->length=32 sgd->length=255 req->cryptlen=16

In this case sgs and sgd are both the first (and only) entries in the
list.  Should this take account of req->cryptlen as well?

In fact, can't this whole function be folded into one loop over src and
dst at the same time, since all the checks must be the same?  Something
like this (untested):

	while (sgs && sgd) {
		if (!IS_ALIGNED(sgs->offset, sizeof(u32)) ||
		    !IS_ALIGNED(sgd->offset, sizeof(u32)))
			return true;

		todo = min(len, sgs->length);
		if (todo % bs)
			return true;

		if (sgd->length < todo)
			return true;

		len -= todo;
		sgs = sg_next(sgs);
		sgd = sg_next(sgd);
	}

	if (len)
		return true;

> +			return true;
> +		sgs = sg_next(sgs);
> +		sgd = sg_next(sgd);
> +	}
> +	return false;
> +}