Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 11 Apr 2022 10:42:26 +0200
From:   LABBE Corentin <clabbe@baylibre.com>
To:     John Keeping <john@metanate.com>
Cc:     heiko@sntech.de, herbert@gondor.apana.org.au, krzk+dt@kernel.org,
        robh+dt@kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-crypto@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org
Subject: Re: [PATCH v4 06/33] crypto: rockchip: add fallback for cipher
Message-ID: <YlPp8rkH7B742TZM@Red>
References: <20220401201804.2867154-1-clabbe@baylibre.com>
 <20220401201804.2867154-7-clabbe@baylibre.com>
 <YkrV1z5GPVXc+d/X@donbot>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YkrV1z5GPVXc+d/X@donbot>
Precedence: bulk

Le Mon, Apr 04, 2022 at 12:26:15PM +0100, John Keeping a ?crit :
> 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@baylibre.com>
> > ---
> > 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;
> 

Thanks, for this hint, I will use it.

Regards