linux-kernel - Re: [PATCH RFC 06/10] crypto: engine: introduce ct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <VI1PR04MB44455F7F7830159B6ED336648C360@VI1PR04MB4445.eurprd04.prod.outlook.com>
Date:   Thu, 16 Jan 2020 11:34:19 +0000
From:   Iuliana Prodan <iuliana.prodan@....com>
To:     Corentin Labbe <clabbe.montjoie@...il.com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "mripard@...nel.org" <mripard@...nel.org>,
        "wens@...e.org" <wens@...e.org>,
        Horia Geanta <horia.geanta@....com>,
        Aymen Sghaier <aymen.sghaier@....com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-sunxi@...glegroups.com" <linux-sunxi@...glegroups.com>
Subject: Re: [PATCH RFC 06/10] crypto: engine: introduce ct

On 1/14/2020 4:00 PM, Corentin Labbe wrote:
> We will store the number of request in a batch in engine->ct.
> This patch adds all loop to unprepare all requests of a batch.
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@...il.com>
> ---
>   crypto/crypto_engine.c  | 30 ++++++++++++++++++------------
>   include/crypto/engine.h |  2 ++
>   2 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
> index b72873550587..591dea5ddeec 100644
> --- a/crypto/crypto_engine.c
> +++ b/crypto/crypto_engine.c
> @@ -28,6 +28,7 @@ static void crypto_finalize_request(struct crypto_engine *engine,
>   	bool finalize_cur_req = false;
>   	int ret;
>   	struct crypto_engine_ctx *enginectx;
> +	int i = 0;
>   
>   	spin_lock_irqsave(&engine->queue_lock, flags);
>   	if (engine->cur_reqs[0].req == req)
You're checking here just the first request, but do the completion for 
all? Why? Shouldn't we check for each request if it was done by hw or not?

I've also seen that the do_one_request is called only on the first 
request, from the batch.

In your driver you do the prepare/unprepare for the whole batch at once, 
but not all drivers, who uses crypto-engine, are doing this (see virtio, 
amlogic, stm32). And I don't know if they can...

> @@ -35,17 +36,20 @@ static void crypto_finalize_request(struct crypto_engine *engine,
>   	spin_unlock_irqrestore(&engine->queue_lock, flags);
>   
>   	if (finalize_cur_req) {
> -		enginectx = crypto_tfm_ctx(engine->cur_reqs[0].req->tfm);
> -		if (engine->cur_reqs[0].prepared &&
> -		    enginectx->op.unprepare_request) {
> -			ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[0].req);
> -			if (ret)
> -				dev_err(engine->dev, "failed to unprepare request\n");
> -		}
> -		engine->cur_reqs[0].req->complete(engine->cur_reqs[0].req, err);
> +		do {
> +			enginectx = crypto_tfm_ctx(engine->cur_reqs[i].req->tfm);
> +			if (engine->cur_reqs[i].prepared &&
> +			    enginectx->op.unprepare_request) {
> +				ret = enginectx->op.unprepare_request(engine, engine->cur_reqs[i].req);
> +				if (ret)
> +					dev_err(engine->dev, "failed to unprepare request\n");
> +			}
> +			engine->cur_reqs[i].prepared = false;
> +			engine->cur_reqs[i].req->complete(engine->cur_reqs[i].req, err);
> +		} while (++i < engine->ct);
>   		spin_lock_irqsave(&engine->queue_lock, flags);
> -		engine->cur_reqs[0].prepared = false;
> -		engine->cur_reqs[0].req = NULL;
> +		while (engine->ct-- > 0)
> +			engine->cur_reqs[engine->ct].req = NULL;
>   		spin_unlock_irqrestore(&engine->queue_lock, flags);
>   	} else {
>   		req->complete(req, err);
> @@ -109,13 +113,14 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>   		goto out;
>   	}
>   
> +	engine->ct = 0;
>   	/* Get the fist request from the engine queue to handle */
>   	backlog = crypto_get_backlog(&engine->queue);
>   	async_req = crypto_dequeue_request(&engine->queue);
>   	if (!async_req)
>   		goto out;
>   
> -	engine->cur_reqs[0].req = async_req;
> +	engine->cur_reqs[engine->ct].req = async_req;
>   	if (backlog)
>   		backlog->complete(backlog, -EINPROGRESS);
>   
> @@ -144,8 +149,9 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>   				ret);
>   			goto req_err;
>   		}
> -		engine->cur_reqs[0].prepared = true;
> +		engine->cur_reqs[engine->ct].prepared = true;
>   	}
> +	engine->ct++;
>   	if (!enginectx->op.do_one_request) {
>   		dev_err(engine->dev, "failed to do request\n");
>   		ret = -EINVAL;
> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
> index 362134e226f4..021136a47b93 100644
> --- a/include/crypto/engine.h
> +++ b/include/crypto/engine.h
> @@ -50,6 +50,7 @@ struct cur_req {
>    * @priv_data: the engine private data
>    * @rmax:	The number of request which can be processed in one batch
>    * @cur_reqs: 	A list for requests to be processed
> + * @ct:		How many requests will be handled in one batch
>    */
>   struct crypto_engine {
>   	char			name[ENGINE_NAME_LEN];
> @@ -73,6 +74,7 @@ struct crypto_engine {
>   	void				*priv_data;
>   	int 				rmax;
>   	struct cur_req 			*cur_reqs;
> +	int ct;
>   };
>   
>   /*
>