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

Mike Snitzer
2015-05-05T12:50:51Z

On Tue, May 05 2015 at 2:42am -0400,
Milan Broz <mbroz@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@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@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.