Re: FW: [PATCH net] net/tls: sendfile fails with ktls offload

[rohit maheshwari]

-----Original Message-----
From: Jakub Kicinski <k...@kernel.org>
Sent: Friday, September 25, 2020 3:27 AM
To: Rohit Maheshwari <roh...@chelsio.com>
Cc: netdev@vger.kernel.org; da...@davemloft.net; vakul.g...@nxp.com; secdev 
<sec...@chelsio.com>
Subject: Re: [PATCH net] net/tls: sendfile fails with ktls offload

On Thu, 24 Sep 2020 13:20:25 +0530 Rohit Maheshwari wrote:

At first when sendpage gets called, if there is more data, 'more' in
tls_push_data() gets set which later sets pending_open_record_frags,
but when there is no more data in file left, and last time
tls_push_data() gets called, pending_open_record_frags doesn't get
reset. And later when
2 bytes of encrypted alert comes as sendmsg, it first checks for
pending_open_record_frags, and since this is set, it creates a record
with
0 data bytes to encrypt, meaning record length is prepend_size +
tag_size only, which causes problem.

Agreed, looks like the value in pending_open_record_frags may be stale.

  We should set/reset pending_open_record_frags based on more bit.

I think you implementation happens to work because there is always left over 
data when more is set, but I don't think that has to be the case.

Yes, with small file size, more bit won't be set, and so the existing code
works there. If more is not set, which means this should be the overall
record and so, we can continue putting header and TAG to make it a
complete record.

Also shouldn't we update this field or destroy the record before the break on 
line 478?

If more is set, and payload is lesser than the max size, then we need to
hold on to get next sendpage and continue adding frags in the same record.
So I don't think we need to do any update or destroy the record. Please
correct me if I am wrong here.

Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
Signed-off-by: Rohit Maheshwari <roh...@chelsio.com>
---
  net/tls/tls_device.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index
b74e2741f74f..a02aadefd86e 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -492,11 +492,11 @@ static int tls_push_data(struct sock *sk,
                if (!size) {
  last_record:
                        tls_push_record_flags = flags;
-                       if (more) {
-                               tls_ctx->pending_open_record_frags =
-                                               !!record->num_frags;
+                       /* set/clear pending_open_record_frags based on more */
+                       tls_ctx->pending_open_record_frags = !!more;
+
+                       if (more)
                                break;
-                       }

done = true;

                }