On Wed, 20 May 2020 15:56:56 -0400 Pooja Trivedi wrote:
> On Tue, May 19, 2020 at 5:43 PM Jakub Kicinski <k...@kernel.org> wrote:
> >
> > On Tue, 19 May 2020 13:21:56 -0400 Pooja Trivedi wrote:
> > > On Mon, May 18, 2020 at 6:50 PM Jakub Kicinski <k...@kernel.org> wrote:
> > > > On Sun, 17 May 2020 16:26:36 +0000 Pooja Trivedi wrote:
> > > > > In pure sw ktls(AES-NI), -EAGAIN from tcp layer (do_tcp_sendpages for
> > > > > encrypted record) gets treated as error, subtracts the offset, and
> > > > > returns to application. Because of this, application sends data from
> > > > > subtracted offset, which leads to data integrity issue. Since record
> > > > > is
> > > > > already encrypted, ktls module marks it as partially sent and pushes
> > > > > the
> > > > > packet to tcp layer in the following iterations (either from bottom
> > > > > half
> > > > > or when pushing next chunk). So returning success in case of EAGAIN
> > > > > will fix the issue.
> > > > >
> > > > > Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
> > > > > Signed-off-by: Pooja Trivedi <pooja.triv...@stackpath.com>
> > > > > Reviewed-by: Mallesham Jatharkonda
> > > > > <mallesham.jatharko...@oneconvergence.com>
> > > > > Reviewed-by: Josh Tway <josh.t...@stackpath.com>
> > > >
> > > > This looks reasonable, I think. Next time user space calls if no new
> > > > buffer space was made available it will get a -EAGAIN, right?
> > >
> > > Yes, this fix should only affect encrypted record. Plain text calls from
> > > user space should be unaffected.
> >
> > AFAICS if TCP layer is full next call from user space should hit
> > sk_stream_wait_memory() immediately and if it has MSG_DONTWAIT set
> > exit with EAGAIN. Which I believe to be correct behavior.
> >
>
> The flow is tls_sw_sendmsg/tls_sw_do_sendpage --> bpf_exec_tx_verdict -->
> tls_push_record --> tls_tx_records --> tls_push_sg --> do_tcp_sendpages
>
> do_tcp_sendpages() sends partial record, 'retry:' label is exercised wherein
> do_tcp_sendpages gets called again and returns -EAGAIN.
> tls_push_sg sets partially_sent_record/partially_sent_offset and
> returns -EAGAIN. -EAGAIN bubbles up to bpf_exec_tx_verdict.
> In bpf_exec_tx_verdict, the following code causes 'copied' variable to
> get updated to a negative value and returns -EAGAIN.
>
> err = tls_push_record(sk, flags, record_type);
> if (err && err != -EINPROGRESS) {
> *copied -= sk_msg_free(sk, msg);
> tls_free_open_rec(sk);
> }
> return err;
>
> -EAGAIN returned by bpf_exec_tx_verdict causes
> tls_sw_sendmsg/tls_sw_do_sendpage to 'continue' in the while loop and
> call sk_stream_wait_memory(). sk_stream_wait_memory returns -EAGAIN
> also and control reaches the 'send_end:' label. The following return
> statement causes a negative 'copied' variable value to be returned to the
> user space.
>
> return copied ? copied : ret;
>
> User space applies this negative value as offset for the next send, causing
> part of the record that was already sent to be pushed again.
>
> Hope this clarifies it.
Oh yes, sorry I was talking about the behavior _after_ your patch, on
the _next_ sendmsg/sendpage call.
It should now work like this:
bpf_exec_tx_verdict() returns success, next iteration of the
sendmsg/sendpage loop hits sk_stream_wait_memory(), we return
positive copied which is counts the entire record, even though
some of it is still in partially_sent_record. If user space
calls sendmsg again we will hit sk_stream_wait_memory() ->
send_end -> this time copied is 0, so user space will see
-EAGAIN.
If I'm still not making sense don't worry about it, I think it should
be easy to explain based on the selftest.
