Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:25:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
> > > If you're making a single call, what guarantees the ordering?
> > 
> > Technically, io_submit is the syscall that triggers the recvmsg. Are you
> > saying that this syscall does not maintain ordering? At least the man page
> > does not add any hints that it would not (unlike the lio_list man page).
> 
> The code certainly does.  But my point is that you can do the
> same thing using the current API.  Just make your list be pairs
> of write/read and it should work.

Over the weekend I spend more time looking into the implementation of 
io_submit and its data structures exchanged between user space and kernel 
space.

During that review I was unable to find any way how an io_submit can be linked 
to a sendmsg/sendpage operation. Therefore, I am unable to see how the 
suggested TX/RX SGL pair can come into existence during sendmsg time.

A TX/RX SGL pair can only come into existence during recvmsg that is triggered 
by io_submit when the kernel learns about the amount of data it shall process. 
That means that for sendmsg all the kernel can do is store the provided data 
in a serial fashion in the TX SGL. During recvmsg, the kernel then takes the 
required data from the TX SGL and processes it so that the output can be 
stored in the RX SGL.

Note, the kernel has to handle dissimilar sendmsg/recvmsg invocations, e.g. 
sendmsg(16 bytes), sendmsg(20 bytes), sendmsg(12bytes), recvmsg(32bytes), 
recvmsg(16 bytes). As we cannot link the sendmsg to the recvmsg calls, all the 
kernel can do is to collect the data during sendmsg and process the parts 
requested during recvmsg.

With the patch set, I exactly do that. I have one TX SGL that is simply filled 
with data as user space uses sendmsg to send data. At the time the recvmsg is 
invoked and the kernel sees how much buffer the caller provides and thus knows 
how much data it can process from the TX SGL (assuming that the kernel shall 
fill the entire recvmsg buffer as much as possible), it performs the crypto 
operation.

After the kernel processed (parts of) the TX SGL data, it could now free up 
the processed SGs in that SGL. That is already performed in the algif_skcipher 
interface, but not in the algif_aead (there, the TX SGL is only freed after 
all its components are processed). Please note that as I mentioned in the 
intro part to the patch, I only tried to fix the RX SGL handling. If my 
approach is accepted, I volunteer to port the algif_skcipher TX SGL handling 
to algif_aead so that in algif_aead the TX SG entries are freed once they are 
processed. Furthermore, I see that now there is huge code duplication 
regarding the RX/TX SGL handling between algif_skcipher and algif_aead which 
can than be handled with common service functions. But again, such work makes 
only sense if the initial approach discussed above and presented with this 
first patch set is accepted.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:25:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
> > > If you're making a single call, what guarantees the ordering?
> > 
> > Technically, io_submit is the syscall that triggers the recvmsg. Are you
> > saying that this syscall does not maintain ordering? At least the man page
> > does not add any hints that it would not (unlike the lio_list man page).
> 
> The code certainly does.  But my point is that you can do the
> same thing using the current API.  Just make your list be pairs
> of write/read and it should work.

How shall these pairs come into existence? The read/write system calls may 
come at unspecified times with potentially dissimilar buffer lengths.


Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:16:27PM +0100, Stephan Müller wrote:
>
> > If you're making a single call, what guarantees the ordering?
> 
> Technically, io_submit is the syscall that triggers the recvmsg. Are you 
> saying that this syscall does not maintain ordering? At least the man page 
> does not add any hints that it would not (unlike the lio_list man page).

The code certainly does.  But my point is that you can do the
same thing using the current API.  Just make your list be pairs
of write/read and it should work.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:12:59 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:10:02PM +0100, Stephan Müller wrote:
> > > Well if ordering is not guaranteed that I don't see how your code
> > > can work either.  Or am I missing something?
> > 
> > The patch simply stores all data it gets from sendmsg in the src SGL. In
> > addition it maintains an offset pointer into that src SGLs.
> > 
> > When the recvmsg call comes in and the dst SGL is prepared, it simply
> > takes as much data from the src SGL as needed to cover the request
> > defined by the dst SGL. After completing that operation, the offset
> > pointer is moved forward to point to a yet unused part of the src SGL. If
> > another recvmsg comes in without an intermediate sendmsg, it simply
> > starts using the data from the src SGL starting from the offset.
> > 
> > Therefore, the code should now be able to handle a write / write / read /
> > read scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes)
> > / read (16 bytes). At least my tests covered a successful testing of that
> > scenario which always crashed the kernel before.
> 
> Are you making separate read calls or just a single one? If you're
> making separate calls, then this is no differnt to just doing
> write/read pairs.  You're not saving any overhead.

I make one read call.
> 
> If you're making a single call, what guarantees the ordering?

Technically, io_submit is the syscall that triggers the recvmsg. Are you 
saying that this syscall does not maintain ordering? At least the man page 
does not add any hints that it would not (unlike the lio_list man page).

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:10:02PM +0100, Stephan Müller wrote:
>
> > Well if ordering is not guaranteed that I don't see how your code
> > can work either.  Or am I missing something?
> 
> The patch simply stores all data it gets from sendmsg in the src SGL. In 
> addition it maintains an offset pointer into that src SGLs.
> 
> When the recvmsg call comes in and the dst SGL is prepared, it simply takes 
> as 
> much data from the src SGL as needed to cover the request defined by the dst 
> SGL. After completing that operation, the offset pointer is moved forward to 
> point to a yet unused part of the src SGL. If another recvmsg comes in 
> without 
> an intermediate sendmsg, it simply starts using the data from the src SGL 
> starting from the offset.
> 
> Therefore, the code should now be able to handle a write / write / read / 
> read 
> scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes) / read 
> (16 bytes). At least my tests covered a successful testing of that scenario 
> which always crashed the kernel before.

Are you making separate read calls or just a single one? If you're
making separate calls, then this is no differnt to just doing
write/read pairs.  You're not saving any overhead.

If you're making a single call, what guarantees the ordering?

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:03:35 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 11:49:05AM +0100, Stephan Müller wrote:
> > According to the man page of lio_listio(3) the provided AIO operations are
> > executed in an unspecified order. I would infer from that statement that
> > even if an order of write / read / write / read is defined by the caller,
> > this order may not be followed by the kernel. Thus we would need to
> > consider the case that in the end, algif has to process the order of
> > write / write / read / read or any other order.
> 
> Well if ordering is not guaranteed that I don't see how your code
> can work either.  Or am I missing something?

The patch simply stores all data it gets from sendmsg in the src SGL. In 
addition it maintains an offset pointer into that src SGLs.

When the recvmsg call comes in and the dst SGL is prepared, it simply takes as 
much data from the src SGL as needed to cover the request defined by the dst 
SGL. After completing that operation, the offset pointer is moved forward to 
point to a yet unused part of the src SGL. If another recvmsg comes in without 
an intermediate sendmsg, it simply starts using the data from the src SGL 
starting from the offset.

Therefore, the code should now be able to handle a write / write / read / read 
scenario. Or it can handle, say, a write(32 bytes) / read (16 bytes) / read 
(16 bytes). At least my tests covered a successful testing of that scenario 
which always crashed the kernel before.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Fri, Jan 13, 2017 at 11:49:05AM +0100, Stephan Müller wrote:
> 
> According to the man page of lio_listio(3) the provided AIO operations are 
> executed in an unspecified order. I would infer from that statement that even 
> if an order of write / read / write / read is defined by the caller, this 
> order may not be followed by the kernel. Thus we would need to consider the 
> case that in the end, algif has to process the order of write / write / read 
> / 
> read or any other order.

Well if ordering is not guaranteed that I don't see how your code
can work either.  Or am I missing something?

> Besides, the crashes I reported for the current AIO implementation in 
> algif_aead and algif_skcipher are always triggered when invoking an aio_read 
> with two or more IOCBs. The most important aspect I want to cover with the 
> patch set is to stop crashing the kernel.

Please stop adding new features and just fix the existing crash.
Once we have that covered and backported to stable then we can
start addressing new features.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 18:21:45 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:19:57PM +0100, Stephan Müller wrote:
> > > I don't understand, what's wrong with:
> > > 
> > > sendmsg(fd, ...)
> > > aio_read(iocb1)
> > > sendmsg(fd, ...)
> > > aio_read(iocb2)
> > 
> > Sure, that works. But here you limit yourself to one IOCB per aio_read.
> > But
> > aio_read supports multiple IOCBs in one invocation. And this is the issue
> > I am considering.
> 
> Not really.  You just lay it out in the same way with lio_listio.
> That is, a write followed by read, etc.

According to the man page of lio_listio(3) the provided AIO operations are 
executed in an unspecified order. I would infer from that statement that even 
if an order of write / read / write / read is defined by the caller, this 
order may not be followed by the kernel. Thus we would need to consider the 
case that in the end, algif has to process the order of write / write / read / 
read or any other order.

Besides, the crashes I reported for the current AIO implementation in 
algif_aead and algif_skcipher are always triggered when invoking an aio_read 
with two or more IOCBs. The most important aspect I want to cover with the 
patch set is to stop crashing the kernel.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:19:57PM +0100, Stephan Müller wrote:
>
> > I don't understand, what's wrong with:
> > 
> > sendmsg(fd, ...)
> > aio_read(iocb1)
> > sendmsg(fd, ...)
> > aio_read(iocb2)
> 
> Sure, that works. But here you limit yourself to one IOCB per aio_read. But 
> aio_read supports multiple IOCBs in one invocation. And this is the issue I 
> am 
> considering.

Not really.  You just lay it out in the same way with lio_listio.
That is, a write followed by read, etc.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 00:17:59 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:10:14PM +0100, Stephan Müller wrote:
> > Each IOCB would transpire into an independent, separate recvmsg invocation
> > without an additional sendmsg/sendpage operation. Thus, in order to
> > support
> > multiple IOCBs, all data the multiple recvmsg invocations will operate on
> > must be injected into the kernel beforehand.
> 
> I don't understand, what's wrong with:
> 
> sendmsg(fd, ...)
> aio_read(iocb1)
> sendmsg(fd, ...)
> aio_read(iocb2)

Sure, that works. But here you limit yourself to one IOCB per aio_read. But 
aio_read supports multiple IOCBs in one invocation. And this is the issue I am 
considering.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:10:14PM +0100, Stephan Müller wrote:
>
> Each IOCB would transpire into an independent, separate recvmsg invocation 
> without an additional sendmsg/sendpage operation. Thus, in order to support 
> multiple IOCBs, all data the multiple recvmsg invocations will operate on 
> must 
> be injected into the kernel beforehand.

I don't understand, what's wrong with:

sendmsg(fd, ...)
aio_read(iocb1)
sendmsg(fd, ...)
aio_read(iocb2)

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Freitag, 13. Januar 2017, 00:07:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:05:03PM +0100, Stephan Müller wrote:
> > Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
> > 
> > Hi Herbert,
> > 
> > > That would mean that we would only support one IOCB.
> > 
> > As we also need to be standards compliant, would it be appropriate to only
> > support one IOCB? I think this is a significant difference to other AIO
> > operations like for networking.
> 
> Why would we be limited to one IOCB?

Each IOCB would transpire into an independent, separate recvmsg invocation 
without an additional sendmsg/sendpage operation. Thus, in order to support 
multiple IOCBs, all data the multiple recvmsg invocations will operate on must 
be injected into the kernel beforehand.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:

Hi Herbert,

> 
> That would mean that we would only support one IOCB.

As we also need to be standards compliant, would it be appropriate to only 
support one IOCB? I think this is a significant difference to other AIO 
operations like for networking.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 23:51:28 CET schrieb Herbert Xu:

Hi Herbert,

> On Sun, Dec 25, 2016 at 06:15:06PM +0100, Stephan Müller wrote:
> > + * The following concept of the memory management is used:
> > + *
> > + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL
> > is
> > + * filled by user space with the data submitted via sendpage/sendmsg.
> > Filling + * up the TX SGL does not cause a crypto operation -- the data
> > will only be + * tracked by the kernel. Upon receipt of one recvmsg call,
> > the caller must + * provide a buffer which is tracked with the RX SGL.
> > + *
> > + * During the processing of the recvmsg operation, the cipher request is
> > + * allocated and prepared. To support multiple recvmsg operations
> > operating + * on one TX SGL, an offset pointer into the TX SGL is
> > maintained. The TX SGL + * that is used for the crypto request is
> > scatterwalk_ffwd by the offset + * pointer to obtain the start address
> > the crypto operation shall use for + * the input data.
> 
> I think this is overcomplicating things.  The async interface
> should be really simple.  It should be exactly the same as the
> sync interface, except that completion is out-of-line.
> 
> So there should be no mixing of SGLs from different requests.
>  Just start with a clean slate after each recvmsg regardless of
> whether it's sync or async.
> 
> The only difference in the async case is that you need to keep a
> reference to the old pages and free them upon completion.  But this
> should in no way interfere with subsequent requests.

That would mean that we would only support one IOCB.

At least with algif_skcipher, having multiple IOCBs would reduce the number of 
system calls user space needs to make for multiple plaintext / ciphertext 
blocks. But then, with the use of IOVECs, user space could provide all input 
data with one system call anyway.

Ok, I will update the patch as suggested.
> 
> Cheers,



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html