linux-kernel - Re: [PATCH v3 01/28] crypto: change backlog return code to -EIOCBQUEUED

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOtvUMdoMfrpZPuRfD_cQo4=FDp5Vn4uv51Vzc-0Gitmvr2V6g@mail.gmail.com>
Date:   Mon, 3 Jul 2017 16:23:18 +0300
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jonathan Corbet <corbet@....net>,
        David Howells <dhowells@...hat.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Gary Hook <gary.hook@....com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Arnaud Ebalard <arno@...isbad.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Alasdair Kergon <agk@...hat.com>,
        Mike Snitzer <snitzer@...hat.com>,
        device-mapper development <dm-devel@...hat.com>,
        Shaohua Li <shli@...nel.org>, Steve French <sfrench@...ba.org>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>, linux-crypto@...r.kernel.org,
        linux-doc@...r.kernel.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        keyrings@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-raid@...r.kernel.org,
        linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
        linux-fscrypt@...r.kernel.org,
        linux-ima-devel@...ts.sourceforge.net,
        linux-ima-user@...ts.sourceforge.net,
        linux-security-module@...r.kernel.org,
        Ofir Drang <ofir.drang@....com>
Subject: Re: [PATCH v3 01/28] crypto: change backlog return code to -EIOCBQUEUED

On Mon, Jul 3, 2017 at 3:35 PM, Herbert Xu <herbert@...dor.apana.org.au> wrote:
> On Sun, Jul 02, 2017 at 05:41:43PM +0300, Gilad Ben-Yossef wrote:
>> The crypto API was using the -EBUSY return value to indicate
>> both a hard failure to submit a crypto operation into a
>> transformation provider when the latter was busy and the backlog
>> mechanism was not enabled as well as a notification that the operation
>> was queued into the backlog when the backlog mechanism was enabled.
>>
>> Having the same return code indicate two very different conditions
>> depending on a flag is both error prone and requires extra runtime
>> check like the following to discern between the cases:
>>
>>       if (err == -EINPROGRESS ||
>>           (err == -EBUSY && (ahash_request_flags(req) &
>>                              CRYPTO_TFM_REQ_MAY_BACKLOG)))
>>
>> This patch changes the return code used to indicate a crypto op
>> was queued in the backlog to -EIOCBQUEUED, thus resolving both
>> issues.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@...yossef.com>
>
> So you're changing the success case from EBUSY to EIOCBQUEUED.
> This results in a lot of churn as you have to change every single
> driver and caller.
>
> How about changing the error case to use something other than
> EBUSY instead?

That was indeed my first thought - I wanted to change EBUSY to EAGAIN.
It might even be a more descriptive error message since the failure is
transient.

What stopped me was that I saw the algif interface passes this EBUSY value
to user space. I don't know of any software that depends on this value but
I'm really averse to changing user space API.

Of course, we can just plant a (ret != AGAIN : ? EBUSY) in the algif interface
return to user space code path but that felt like a kludge to me.

And it doesn't really save any churn, I think - I'd still have to
visit all the places that
use the flags value to distinguish between the two EBUSY use cases and  I need
to visit most places that check for EBUSY as backlog indication because I'm
replacing the check with the generic replacement, so it doesn't look
like in the end
it will be a smaller change.

Having said that, if you prefer me to replace the error case I'd
happily do that.

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru