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

[Mike Snitzer]

On Tue, May 05 2015 at  2:42am -0400,
Milan Broz <mbroz at redhat.com> wrote:

> On 05/05/2015 05:22 AM, Mike Snitzer wrote:
> > On Mon, May 04 2015 at  5:32pm -0400,
> > Rabin Vincent <rabin at rab.in> 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 at servergy.com>
> >>>
> >>> 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.
> ...
> 
> >> 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.
> 
> 
> Mike, I thought there was a discussion with crypto API maintainer before
> merging this patch, I think there were some comments that the patch seems
> to be not correct...

I unfortunately didn't cc Herbert on the original v2 patch (but I did
later bounce the mail to him IIRC).  Regardless, no I didn't see any
feedback and I really should've been more active in engaging him.

> This API (EBUSY/EINPROGESS codes) use was there since dmcrypt switched
> to async crypto API.
> 
> There should tests for it (some years ago we tested the async path by forcing
> to use cryptd, this was able to clearly simulate the BUSY path as well),
> but not sure if it is still possible so easily.
> 
> > Any chance you'd be willing to provide in-code comments in the
> > appropriate places in dm-crypt.c (after having reverted this patch in
> > question)?
> > 
> > I'd be happy to take the combination of the revert and documentation in
> > a single patch and get it to Linus for 4.0-rc3.
> 
> Please, do not mix revert patches with additions (even if it is just comment),
> someone could be even more confused what's going on here.

I was only saying to mix comments and revert if the patch in question
didn't get into stable@ at all.

But safer to just do a pure revert.  I get it queued up to send to Linus.

> If nobody volunteers, I'll try to add some comments later to dmcrypt code,
> but that is definitely not for stable.

Yeap, thanks.