Re: [PATCH 3.14 73/92] dm crypt: fix deadlock when async crypto algorithm returns -EBUSY

[Ben Collins]

> On May 4, 2015, at 5:32 PM, Rabin Vincent <rabin@xxxxxx> wrote:
> 
> On Sat, May 02, 2015 at 09:03:28PM +0200, Greg Kroah-Hartman wrote:
>> 3.14-stable review patch.  If anyone has any objections, please let me know.
>> 
>> ------------------
>> 
>> From: Ben Collins <ben.c@xxxxxxxxxxxx>
>> 
>> commit 0618764cb25f6fa9fb31152995de42a8a0496475 upstream.
> 
> The commit in question was recently merged to mainline and is now being
> sent to various stable kernels.  But I think the patch is wrong and the
> real problem lies in the Freescale CAAM driver which is described as
> having being tested with.

There may be something quirky with CAAM, yes, but the fact is, the code
in the dm-crypt module was very dissimilar to other drivers performing
the same loop/logic.

I donât think that this patch, which was deemed correct, should be held
up based on lower level logic âpossiblyâ being partially at fault. On the
contrary, I think errors you point out may just be showing the error in
dm-crypt more easily, rather than being the complete problem.

> commit 0618764cb25f6fa9fb31152995de42a8a0496475
> Author: Ben Collins <ben.c@xxxxxxxxxxxx>
> Date:   Fri Apr 3 16:09:46 2015 +0000
> 
>     dm crypt: fix deadlock when async crypto algorithm returns -EBUSY
> 
>     I suspect this doesn't show up for most anyone because software
>     algorithms typically don't have a sense of being too busy.  However,
>     when working with the Freescale CAAM driver it will return -EBUSY on
>     occasion under heavy -- which resulted in dm-crypt deadlock.
> 
>     After checking the logic in some other drivers, the scheme for
>     crypt_convert() and it's callback, kcryptd_async_done(), were not
>     correctly laid out to properly handle -EBUSY or -EINPROGRESS.
> 
>     Fix this by using the completion for both -EBUSY and -EINPROGRESS.  Now
>     crypt_convert()'s use of completion is comparable to
>     af_alg_wait_for_completion().  Similarly, kcryptd_async_done() follows
>     the pattern used in af_alg_complete().
> 
>     Before this fix dm-crypt would lockup within 1-2 minutes running with
>     the CAAM driver.  Fix was regression tested against software algorithms
>     on PPC32 and x86_64, and things seem perfectly happy there as well.
> 
>     Signed-off-by: Ben Collins <ben.c@xxxxxxxxxxxx>
>     Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
>     Cc: stable@xxxxxxxxxxxxxxx
> 
> dm-crypt uses CRYPTO_TFM_REQ_MAY_BACKLOG.  This means the the crypto
> driver should internally backlog requests which arrive when the queue is
> full and process them later.  Until the crypto hw's queue becomes full,
> the driver returns -EINPROGRESS.  When the crypto hw's queue if full,
> the driver returns -EBUSY, and if CRYPTO_TFM_REQ_MAY_BACKLOG is set, is
> expected to backlog the request and process it when the hardware has
> queue space.  At the point when the driver takes the request from the
> backlog and starts processing it, it calls the completion function with
> a status of -EINPROGRESS.  The completion function is called (for a
> second time, in the case of backlogged requests) with a status/err of 0
> when a request is done.
> 
> Crypto drivers for hardware without hardware queueing use the helpers,
> crypto_init_queue(), crypto_enqueue_request(), crypto_dequeue_request()
> and crypto_get_backlog() helpers to implement this behaviour correctly,
> while others implement this behaviour without these helpers (ccp, for
> example).
> 
> dm-crypt (before this patch) uses this API correctly.  It queues up as
> many requests as the hw queues will allow (i.e. as long as it gets back
> -EINPROGRESS from the request function).  Then, when it sees at least
> one backlogged request (gets -EBUSY), it waits till that backlogged
> request is handled (completion gets called with -EINPROGRESS), and then
> continues.  The references to af_alg_wait_for_completion() and
> af_alg_complete() in the commit message are irrelevant because those
> functions only handle one request at a time, unlink dm-crypt.
> 
> The problem is that the Freescale CAAM driver, which this problem is
> described as being seen on, fails to implement the backlogging behaviour
> correctly.  In cam_jr_enqueue(), if the hardware queue is full, it
> simply returns -EBUSY without backlogging the request.  What the
> observed deadlock was is not described in the commit message but it is
> obviously the wait_for_completion() in crypto_convert() where dm-crypto
> would wait for the completion being called with -EINPROGRESS in the case
> of backlogged requests.  This completion will never be completed due to
> the bug in the CAAM driver.
> 
> What this patch does is that it makes dm-crypt wait for every request,
> even when the driver/hardware queues are not full, which means that
> dm-crypt will never see -EBUSY.  This means that this patch will cause a
> performance regression on all crypto drivers which implement the API
> correctly.
> 
> So, I think this patch should be reverted in mainline and the stable
> kernels where it has been merged, and correct backlog handling should be
> implemented in the CAAM driver instead.

ââ
Ben Collins
Cyphre Champion
ââââââââââââââ
VP of Engineering
Servergy, Inc.

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail