Using the aesni generic gcm(aes) aead in atomic context

Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

Hi,
I've tried using the aesni generic gcm(aes) aead to implement TLS SW fallback and
I'm getting 
[ 3356.839506] BUG: sleeping function called from invalid context at ./include/crypto/algapi.h:417

The warning is coming from a ___might_sleep() macro that is called if CRYPTO_TFM_REQ_MAY_SLEEP is set.
I'm getting the warning regardless of if pass CRYPTO_ALG_ASYNC or 0 as flags to crypto_alloc_aead("gcm(aes)", 0, flags).

I've also noticed that rfc4106_encrypt() includes a irq_fpu_usable() check while generic_gcmaes_encrypt() doesn't. 
Is the generic gcm(aes) aead unsafe in atomic context?
And if so which aead should I use?

Finally, out of curiosity, doesn't macsec crypto run in atomic context?

Thanks,
Ilya

Re: Using the aesni generic gcm(aes) aead in atomic context

[Herbert Xu]

On Mon, Oct 30, 2017 at 03:18:21PM +0000, Ilya Lesokhin wrote:
> Hi,
> I've tried using the aesni generic gcm(aes) aead to implement TLS SW fallback and
> I'm getting 
> [ 3356.839506] BUG: sleeping function called from invalid context at ./include/crypto/algapi.h:417
> 
> The warning is coming from a ___might_sleep() macro that is called if CRYPTO_TFM_REQ_MAY_SLEEP is set.
> I'm getting the warning regardless of if pass CRYPTO_ALG_ASYNC or 0 as flags to crypto_alloc_aead("gcm(aes)", 0, flags).
> 
> I've also noticed that rfc4106_encrypt() includes a irq_fpu_usable() check while generic_gcmaes_encrypt() doesn't. 
> Is the generic gcm(aes) aead unsafe in atomic context?
> And if so which aead should I use?
> 
> Finally, out of curiosity, doesn't macsec crypto run in atomic context?

Are you allocating the tfm from atomic context? That is not allowed.

Normally you would allocate the tfm in process context, e.g., when
the connection is setup.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

On Mon Tuesday, October 31, 2017 6:10 AM, Herbert Xu wrote:
> 
> Are you allocating the tfm from atomic context? That is not allowed.
> 
> Normally you would allocate the tfm in process context, e.g., when the
> connection is setup.
> 

I call crypto_alloc_aead("gcm(aes)", 0, flags) in process context.
and aead_request_set_tfm(aead_req, aead) in atomic context.
I'm assuming by tfm you are referring to the struct crypto_aead allocated in the first call I mentioned.

I was able to resolve the  "sleeping function called from invalid context" issue
By clearing the CRYPTO_TFM_REQ_MAY_SLEEP flag using 
aead_request_set_callback with flags = 0.

However I'm still concerned about the lack of irq_fpu_usable() check.
and I don't really want to do the check myself outside of the crypto code as it is
arch specific.

Re: Using the aesni generic gcm(aes) aead in atomic context

[Herbert Xu]

On Tue, Oct 31, 2017 at 07:14:12AM +0000, Ilya Lesokhin wrote:
>
> However I'm still concerned about the lack of irq_fpu_usable() check.
> and I don't really want to do the check myself outside of the crypto code as it is
> arch specific.

Users of the crypto API shouldn't need to check irq_fpu_usable().
The crypto API should work regardless of what context you're in.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

On Tuesday, October 31, 2017 9:17 AM, Herbert Xu wrote:
> 
> Users of the crypto API shouldn't need to check irq_fpu_usable().
> The crypto API should work regardless of what context you're in.
> 

I agree, I'm just saying that as far as I can tell that's not true
for the aesni generic gcm(aes) aead.
It just assume the FPU is available without checking.

Re: Using the aesni generic gcm(aes) aead in atomic context

[Herbert Xu]

On Tue, Oct 31, 2017 at 07:23:35AM +0000, Ilya Lesokhin wrote:
> On Tuesday, October 31, 2017 9:17 AM, Herbert Xu wrote:
> > 
> > Users of the crypto API shouldn't need to check irq_fpu_usable().
> > The crypto API should work regardless of what context you're in.
> 
> I agree, I'm just saying that as far as I can tell that's not true
> for the aesni generic gcm(aes) aead.
> It just assume the FPU is available without checking.

You are right.  generic-gcm-aesni is completely broken.

It needs to be rewritten to use a wrapper as is done with rfc4106.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

On Tuesday, October 31, 2017 9:33 AM, Herbert Xu wrote:
> You are right.  generic-gcm-aesni is completely broken.
> 
> It needs to be rewritten to use a wrapper as is done with rfc4106.

I think we should consider having a synchronous implementation that falls back
to integer implementation when the FPU is not available.
This would spare the users from having to handle the asynchronous case.

Hopefully the situation where the FPU is not available is rare enough 
So it won't hurt the performance too much.

Re: Using the aesni generic gcm(aes) aead in atomic context

[Herbert Xu]

On Tue, Oct 31, 2017 at 07:39:08AM +0000, Ilya Lesokhin wrote:
> 
> I think we should consider having a synchronous implementation that falls back
> to integer implementation when the FPU is not available.
> This would spare the users from having to handle the asynchronous case.
> 
> Hopefully the situation where the FPU is not available is rare enough 
> So it won't hurt the performance too much.

For your intended use case I think async processing should work just
fine as it does for IPsec.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

On Tuesday, October 31, 2017 9:45 AM, Herbert Xu wrote:
> 
> For your intended use case I think async processing should work just fine as it
> does for IPsec.
> 

I haven't dived into the async IPSEC fallback code yet, but it seems complicated.
I'm not sure it make the correct performance/complexity tradeoff.

Re: Using the aesni generic gcm(aes) aead in atomic context

[Steffen Klassert]

On Tue, Oct 31, 2017 at 03:44:38PM +0800, Herbert Xu wrote:
> On Tue, Oct 31, 2017 at 07:39:08AM +0000, Ilya Lesokhin wrote:
> > 
> > I think we should consider having a synchronous implementation that falls back
> > to integer implementation when the FPU is not available.
> > This would spare the users from having to handle the asynchronous case.
> > 
> > Hopefully the situation where the FPU is not available is rare enough 
> > So it won't hurt the performance too much.
> 
> For your intended use case I think async processing should work just
> fine as it does for IPsec.

I think Ilya talks about the case where the TLS crypto is intended
to be offloaded to a NIC. In this case we need a software crypto
fallback e.g. if a packet got rerouted to a device that does not
support crypto offloading. For IPsec, we catch these cases in
validate_xmit_skb() either with the ESP GSO handler, or in the non
GSO case with validate_xmit_xfrm(). We currently request for
a sync algorithm to avoid the async handling for this case.

Allowing for async crypto would require some way to handle
async returnes or the -EBUSY case from the crypto layer inside
of a qdisc. Also, in the GSO case it is not clear how to unwind
the GSO call stack on a async return.

I had a discussion with davem at the netfilter workshop about
this. Based on this discussion, I prepared some patches that
I hope to be (RFC) ready until the netdev2.2 next week.

RE: Using the aesni generic gcm(aes) aead in atomic context

[Ilya Lesokhin]

On Tuesday, October 31, 2017 11:14 AM Steffen Klassert wrote:

> I think Ilya talks about the case where the TLS crypto is intended to be offloaded
> to a NIC. In this case we need a software crypto fallback e.g. if a packet got
> rerouted to a device that does not support crypto offloading. 
You are correct.
> 
> Allowing for async crypto would require some way to handle async returnes or
> the -EBUSY case from the crypto layer inside of a qdisc. Also, in the GSO case it is
> not clear how to unwind the GSO call stack on a async return.
> 
> I had a discussion with davem at the netfilter workshop about this. Based on this
> discussion, I prepared some patches that I hope to be (RFC) ready until the
> netdev2.2 next week.

Are you sure supporting ASYNC crypto for fallback is worth the trouble?
This path is going to be slower that the path were you do the crypto in advance, right?

Are you concerned about the lack of synchronous crypto implementations for some algorithms?

Re: Using the aesni generic gcm(aes) aead in atomic context

[Steffen Klassert]

On Tue, Oct 31, 2017 at 09:41:24AM +0000, Ilya Lesokhin wrote:
> 
> Are you sure supporting ASYNC crypto for fallback is worth the trouble?

It is not just for fallback, I plan to support the IPsec GSO codepath
for software crypto too. In this case we should be able to handle all
algorithms, including the async ones.

> This path is going to be slower that the path were you do the crypto in advance, right?

If the cryptd is used, yes. At least that's what I messured for IPsec
forwarding. But I think this is because we enqueue requests much
faster that the cryptd dequeues them.