Message ID | 20201005184313.3887-1-rohitm@chelsio.com |
---|---|
State | New |
Headers | show |
Series | [net,v2] net/tls: sendfile fails with ktls offload | expand |
On Tue, 6 Oct 2020 00:13:13 +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. > We should set/reset pending_open_record_frags based on more bit. > > Also incase if tls_do_allocation() fails, and if record len is only > prepend_size, then destroy the record. > > v1->v2: > - handle tls_do_allocation() failure handling. > > Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> > --- > net/tls/tls_device.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > index b74e2741f74f..f3efd53e31cf 100644 > --- a/net/tls/tls_device.c > +++ b/net/tls/tls_device.c > @@ -463,17 +463,16 @@ static int tls_push_data(struct sock *sk, > if (!record) > break; > handle_error: > - if (record_type != TLS_RECORD_TYPE_DATA) { > - /* avoid sending partial > - * record with type != > - * application_data > - */ > - size = orig_size; > - destroy_record(record); > - ctx->open_record = NULL; > - } else if (record->len > prot->prepend_size) { > + /* avoid sending partial record with type != > + * application_data > + */ > + if (record_type == TLS_RECORD_TYPE_DATA && > + record->len > prot->prepend_size) > goto last_record; > - } > + > + size = orig_size; > + destroy_record(record); > + ctx->open_record = NULL; Yet, this still does not update pending_open_record_frags... > break; > } > @@ -492,11 +491,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; > } Maybe I'm misunderstanding what you're fixing but I think you should just do this: diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index b74e2741f74f..674964d5684b 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -418,7 +418,6 @@ static int tls_push_data(struct sock *sk, struct tls_context *tls_ctx = tls_get_ctx(sk); struct tls_prot_info *prot = &tls_ctx->prot_info; struct tls_offload_context_tx *ctx = tls_offload_ctx_tx(tls_ctx); - int more = flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE); struct tls_record_info *record = ctx->open_record; int tls_push_record_flags; struct page_frag *pfrag; @@ -426,6 +425,7 @@ static int tls_push_data(struct sock *sk, u32 max_open_record_len; int copy, rc = 0; bool done = false; + bool more = false; long timeo; if (flags & @@ -492,9 +492,8 @@ 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; + if (flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE)) { + more = true; break; } @@ -526,6 +525,8 @@ static int tls_push_data(struct sock *sk, } } while (!done); + tls_ctx->pending_open_record_frags = more; + if (orig_size - size > 0) rc = orig_size - size;
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index b74e2741f74f..f3efd53e31cf 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -463,17 +463,16 @@ static int tls_push_data(struct sock *sk, if (!record) break; handle_error: - if (record_type != TLS_RECORD_TYPE_DATA) { - /* avoid sending partial - * record with type != - * application_data - */ - size = orig_size; - destroy_record(record); - ctx->open_record = NULL; - } else if (record->len > prot->prepend_size) { + /* avoid sending partial record with type != + * application_data + */ + if (record_type == TLS_RECORD_TYPE_DATA && + record->len > prot->prepend_size) goto last_record; - } + + size = orig_size; + destroy_record(record); + ctx->open_record = NULL; break; } @@ -492,11 +491,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; }
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. We should set/reset pending_open_record_frags based on more bit. Also incase if tls_do_allocation() fails, and if record len is only prepend_size, then destroy the record. v1->v2: - handle tls_do_allocation() failure handling. Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> --- net/tls/tls_device.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)