| 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
| ||
|
Message-ID: <AM0PR04MB71714D3EA035F07DEF13BBB58C0A0@AM0PR04MB7171.eurprd04.prod.outlook.com>
Date: Tue, 28 Jan 2020 11:00:02 +0000
From: Iuliana Prodan <iuliana.prodan@....com>
To: Corentin Labbe <clabbe.montjoie@...il.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
"mripard@...nel.org" <mripard@...nel.org>,
"wens@...e.org" <wens@...e.org>,
"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>,
Silvano Di Ninno <silvano.dininno@....com>,
Franck Lenormand <franck.lenormand@....com>
Subject: Re: [PATCH 5/9] crypto: engine: add enqueue_request/can_do_more
On 1/28/2020 10:40 AM, Corentin Labbe wrote:
> On Mon, Jan 27, 2020 at 10:58:36PM +0000, Iuliana Prodan wrote:
>> On 1/22/2020 12:45 PM, Corentin Labbe wrote:
>>> This patchs adds two new function wrapper in crypto_engine.
>>> - enqueue_request() for drivers enqueuing request to hardware.
>>> - can_queue_more() for letting drivers to tell if they can
>>> enqueue/prepare more.
>>>
>>> Since some drivers (like caam) only enqueue request without "doing"
>>> them, do_one_request() is now optional.
>>>
>>> Signed-off-by: Corentin Labbe <clabbe.montjoie@...il.com>
>>> ---
>>> crypto/crypto_engine.c | 25 ++++++++++++++++++++++---
>>> include/crypto/engine.h | 14 ++++++++------
>>> 2 files changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
>>> index 5bcb1e740fd9..4a28548c49aa 100644
>>> --- a/crypto/crypto_engine.c
>>> +++ b/crypto/crypto_engine.c
>>> @@ -83,6 +83,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>> goto out;
>>> }
>>>
>>> +retry:
>>> /* Get the fist request from the engine queue to handle */
>>> backlog = crypto_get_backlog(&engine->queue);
>>> async_req = crypto_dequeue_request(&engine->queue);
>>> @@ -118,10 +119,28 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>> goto req_err2;
>>> }
>>> }
>>> +
>>> + if (enginectx->op.enqueue_request) {
>>> + ret = enginectx->op.enqueue_request(engine, async_req);
>>> + if (ret) {
>>> + dev_err(engine->dev, "failed to enqueue request: %d\n",
>>> + ret);
>>> + goto req_err;
>>> + }
>>> + }
>>> + if (enginectx->op.can_queue_more && engine->queue.qlen > 0) {
>>> + ret = enginectx->op.can_queue_more(engine, async_req);
>>> + if (ret > 0) {
>>> + spin_lock_irqsave(&engine->queue_lock, flags);
>>> + goto retry;
>>> + }
>>> + if (ret < 0) {
>>> + dev_err(engine->dev, "failed to call can_queue_more\n");
>>> + /* TODO */
>>> + }
>>> + }
>>> if (!enginectx->op.do_one_request) {
>>> - dev_err(engine->dev, "failed to do request\n");
>>> - ret = -EINVAL;
>>> - goto req_err;
>>> + return;
>>> }
>>> ret = enginectx->op.do_one_request(engine, async_req);
>>> if (ret) {
>>> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
>>> index 03d9f9ec1cea..8ab9d26e30fe 100644
>>> --- a/include/crypto/engine.h
>>> +++ b/include/crypto/engine.h
>>> @@ -63,14 +63,16 @@ struct crypto_engine {
>>> * @prepare__request: do some prepare if need before handle the current request
>>> * @unprepare_request: undo any work done by prepare_request()
>>> * @do_one_request: do encryption for current request
>>> + * @enqueue_request: Enqueue the request in the hardware
>>> + * @can_queue_more: if this function return > 0, it will tell the crypto
>>> + * engine that more space are availlable for prepare/enqueue request
>>> */
>>> struct crypto_engine_op {
>>> - int (*prepare_request)(struct crypto_engine *engine,
>>> - void *areq);
>>> - int (*unprepare_request)(struct crypto_engine *engine,
>>> - void *areq);
>>> - int (*do_one_request)(struct crypto_engine *engine,
>>> - void *areq);
>>> + int (*prepare_request)(struct crypto_engine *engine, void *areq);
>>> + int (*unprepare_request)(struct crypto_engine *engine, void *areq);
>>> + int (*do_one_request)(struct crypto_engine *engine, void *areq);
>>> + int (*enqueue_request)(struct crypto_engine *engine, void *areq);
>>> + int (*can_queue_more)(struct crypto_engine *engine, void *areq);
>>> };
>>
>> As I mentioned in another thread [1], these crypto-engine patches (#1 -
>> #5) imply modifications in all the drivers that use crypto-engine.
>> It's not backwards compatible.
>
> This is wrong. This is false.
> AS I HAVE ALREADY SAID, I have tested and didnt see any behavour change in the current user of crypto engine.
> I have tested my serie with omap, virtio, amlogic, sun8i-ss, sun8i-ce and didnt see any change in behavour WITHOUT CHANGING them.
> I resaid, I didnt touch omap, virtio, etc...
> Only stm32 is not tested because simply there are not board with this driver enabled.
>
I'm not saying that doesn't compile or anything, is just that you change
the API and those drivers will not comply on this new API.
I believe that "it works" is not sufficient, should work properly!
> I have also tested your serie which adds support for crypto engine to caam, and the crash is the same with/without my serie.
> So no behavour change.
>
Thanks for testing this. I'll look into it!
>> Your changes imply that do_one_request executes the request & waits for
>> completion and enqueue_request sends it to hardware. That means that all
>> the other drivers need to be modify, to implement enqueue_request,
>> instead of do_one_request. They need to be compliant with the new
>> changes, new API. Otherwise, they are not using crypto-engine right,
>> don't you think?
>>
>
> My change imply nothing, current user work the same.
> But if they want, they COULD switch to enqueue_request().
>
>> Also, do_one_request it shouldn’t be blocking. We got this confirmation
>> from Herbert [2].
>
> Re-read what Herbert said, "It certainly shouldn't be blocking in the general case." But that means it could.
> But this wont change my patch since both behavour are supported.
>
Since your driver is the one being different (implements do_one_request
as blocking), it's not fair to change the other drivers just for you
special case when we update the crypto-engine. It should be the other
way around.
Add a special case for you and let the other drivers unchanged.
The updated crypto-engine API should be consistent (same semantics) with
the old one. Your proposal doesn't extend the current API, but
reshuffles the callbacks changing their meaning.
We should agree on how we should update crypto-engine to accommodate all
the scenarios, but maintaining backwards compatibility.
Thanks,
Iulia
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FVI1PR04MB44455343230CBA7400D21C998C0C0%40VI1PR04MB4445.eurprd04.prod.outlook.com%2F&data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&sdata=rgzYhX0g9hrzlYcHs7aUWVNFYs6mj86gDu7YIowy0Nk%3D&reserved=0
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20200122144134.axqpwx65j7xysyy3%40gondor.apana.org.au%2F&data=02%7C01%7Ciuliana.prodan%40nxp.com%7C238e3e9a8e5f4d934cf308d7a3cdc3da%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637157976462463995&sdata=Jdp0Q8xPnn5uXtcv6hrk3sFbeC5PgzfwRys2itmL09w%3D&reserved=0
>
Powered by blists - more mailing lists