algif_aead: AIO broken with more than one iocb

algif_aead: AIO broken with more than one iocb

[Stephan Mueller]

Hi Herbert,

The AIO support for algif_aead is broken when submitting more than one iocb. 
The break happens in aead_recvmsg_async at the following code:

        /* ensure output buffer is sufficiently large */
        if (usedpages < outlen)
                goto free;

The reason is that when submitting, say, two iocb, ctx->used contains the 
buffer length for two AEAD operations (as expected). However, the recvmsg code 
is invoked for each iocb individually and thus usedpages should only be 
expected to point to memory for one AEAD operation. But this violates the 
check above.

For example, I have two independent AEAD operations that I want to trigger. 
The input to each is 48 bytes (including space for AAD and tag). The output 
buffer that I have for each AEAD operation is also 48 bytes and thus 
sufficient for the AEAD operation. Yet, when submitting the two AEAD 
operations in one io_submit (i.e. using two iocb), ctx->used indicates that 
the kernel has 96 bytes to process. This is correct, but only half of it 
should be processed in one recvmsg_async invocation.

Note, the AIO operation works perfectly well, when io_submit only sends one 
iocb.

Do you have any idea on how to fix that?

Ciao
Stephan

Re: algif_aead: AIO broken with more than one iocb

[Jeffrey Walton]

> The AIO support for algif_aead is broken when submitting more than one iocb.
> The break happens in aead_recvmsg_async at the following code:
>

I think the kernel needs to take a half step back, and add the missing
self tests and test cases to be more proactive in detecting breaks
earlier. Speaking first hand, some of these breaks have existed for
months.

I don't take the position you can't break things. I believe you can't
make an omelet without breaking eggs; and if you're not breaking
something, then you're probably not getting anything done. The
engineering defect is not detecting the break.

Jeff

Re: algif_aead: AIO broken with more than one iocb

[Stephan Mueller]

Am Sonntag, 11. September 2016, 08:43:00 CEST schrieb Jeffrey Walton:

Hi Jeffrey,

> > The AIO support for algif_aead is broken when submitting more than one
> > iocb.
> > The break happens in aead_recvmsg_async at the following code:
> I think the kernel needs to take a half step back, and add the missing
> self tests and test cases to be more proactive in detecting breaks
> earlier. Speaking first hand, some of these breaks have existed for
> months.
> 
> I don't take the position you can't break things. I believe you can't
> make an omelet without breaking eggs; and if you're not breaking
> something, then you're probably not getting anything done. The
> engineering defect is not detecting the break.

The testing that is implemented for libkcapi should cover almost all code 
paths of AF_ALG in the kernel. However, I just added the AIO support to the 
library in the last few days as this logic is not straight forward. Thus these 
issues show up now.

If you wish to analyze the AIO support more, I can certainly push my current 
development branch of libkcapi to my github tree so that you would have a 
working AIO user space component.

Ciao
Stephan

Re: algif_aead: AIO broken with more than one iocb

[Herbert Xu]

On Sun, Sep 11, 2016 at 04:59:19AM +0200, Stephan Mueller wrote:
> Hi Herbert,
> 
> The AIO support for algif_aead is broken when submitting more than one iocb. 
> The break happens in aead_recvmsg_async at the following code:
> 
>         /* ensure output buffer is sufficiently large */
>         if (usedpages < outlen)
>                 goto free;
> 
> The reason is that when submitting, say, two iocb, ctx->used contains the 
> buffer length for two AEAD operations (as expected). However, the recvmsg code 

I don't think we should allow that.  We should make it so that you
must start a recvmsg before you can send data for a new request.

Remember that the async path should be identical to the sync path,
except that you don't wait for completion.

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: algif_aead: AIO broken with more than one iocb

[Stephan Mueller]

Am Dienstag, 13. September 2016, 18:12:46 CEST schrieb Herbert Xu:

Hi Herbert,

> I don't think we should allow that.  We should make it so that you
> must start a recvmsg before you can send data for a new request.
> 
> Remember that the async path should be identical to the sync path,
> except that you don't wait for completion.

The question is, how does the algif code knows when more than one iocb was 
submitted? Note, each iocb is translated into an independent call of the 
recvmsg_async.

Ciao
Stephan

Re: algif_aead: AIO broken with more than one iocb

[Stephan Mueller]

Am Dienstag, 13. September 2016, 18:12:46 CET schrieb Herbert Xu:

Hi Herbert,

> On Sun, Sep 11, 2016 at 04:59:19AM +0200, Stephan Mueller wrote:
> > Hi Herbert,
> > 
> > The AIO support for algif_aead is broken when submitting more than one
> > iocb.> 
> > The break happens in aead_recvmsg_async at the following code:
> >         /* ensure output buffer is sufficiently large */
> >         if (usedpages < outlen)
> >         
> >                 goto free;
> > 
> > The reason is that when submitting, say, two iocb, ctx->used contains the
> > buffer length for two AEAD operations (as expected). However, the recvmsg
> > code
> I don't think we should allow that.  We should make it so that you
> must start a recvmsg before you can send data for a new request.
> 
> Remember that the async path should be identical to the sync path,
> except that you don't wait for completion.

Just as a followup: with the patch submitted the other day to cover the AAD 
and tag handling, the algif_aead now supports also multiple iocb.

Ciao
Stephan