| Message ID | 20200924075025.11626-1-rohitm@chelsio.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net/tls: sendfile fails with ktls offload | expand |
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. Also shouldn't we update this field or destroy the record before the break on line 478? > Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface") > Signed-off-by: Rohit Maheshwari <rohitm@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; > }
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, September 25, 2020 3:27 AM > To: Rohit Maheshwari <rohitm@chelsio.com> > Cc: netdev@vger.kernel.org; davem@davemloft.net; vakul.garg@nxp.com; secdev <secdev@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 <rohitm@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; >> }
On Fri, 25 Sep 2020 19:32:23 +0530 rohit maheshwari wrote: > > -----Original Message----- > > From: Jakub Kicinski <kuba@kernel.org> > > Sent: Friday, September 25, 2020 3:27 AM > > To: Rohit Maheshwari <rohitm@chelsio.com> > > Cc: netdev@vger.kernel.org; davem@davemloft.net; vakul.garg@nxp.com; secdev <secdev@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. Okay. > > 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. Agreed, if more is set we should continue appending. What I'm saying is that we may exit the loop on line 478 or 525 without updating pending_open_record_frags. So if pending_open_record_frags is set, we'd be in a position where there is no data in the record, yet pending_open_record_frags is set. Won't subsequent cmsg send not cause a zero length record to be generated? > >> Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface") > >> Signed-off-by: Rohit Maheshwari <rohitm@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; > >> }
>> > -----Original Message----- >> > From: Jakub Kicinski <kuba@kernel.org> >> > Sent: Friday, September 25, 2020 3:27 AM >> > To: Rohit Maheshwari <rohitm@chelsio.com> >> > Cc: netdev@vger.kernel.org; davem@davemloft.net; >> vakul.garg@nxp.com; secdev <secdev@chelsio.com> >> > Subject: Re: [PATCH net] net/tls: sendfile fails with ktls offload >> > > >> > 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. > > Agreed, if more is set we should continue appending. > > What I'm saying is that we may exit the loop on line 478 or 525 without > updating pending_open_record_frags. So if pending_open_record_frags is > set, we'd be in a position where there is no data in the record, yet > pending_open_record_frags is set. Won't subsequent cmsg send not cause > a zero length record to be generated? > Exit on line 478 can get triggered if sk_page_frag_refill() fails, and > then by Exit on line 478 can get triggered if sk_page_frag_refill() fails, and then by exiting, it will hit line 529 and will return 'rc = orig_size - size', so I am sure we don't need to do anything else there. Exit on line 525 will be, due to do_tcp_sendpage(), and I think pending_open_record_frags won't be set if this is the last record. And if it is not the last record, do_tcp_sendpage will be failing for a complete and correct record, that doesn't need to be destroyed and at this very moment pending_open_record_frags will suggest that there is more data (unrelated to current failing record), which actually is correct. I think, there won't be cmsg if pending_open_record_frags is set.
On Sun, 27 Sep 2020 00:13:31 +0530 rohit maheshwari wrote: > >> > 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. > > > > Agreed, if more is set we should continue appending. > > > > What I'm saying is that we may exit the loop on line 478 or 525 without > > updating pending_open_record_frags. So if pending_open_record_frags is > > set, we'd be in a position where there is no data in the record, yet > > pending_open_record_frags is set. Won't subsequent cmsg send not cause > > a zero length record to be generated? > > Exit on line 478 can get triggered if sk_page_frag_refill() fails, and > > then by > Exit on line 478 can get triggered if sk_page_frag_refill() fails, > and then by exiting, it will hit line 529 and will return 'rc = > orig_size - size', so I am sure we don't need to do anything else > there. What makes sure pending_open_record_frags is up to date on that exit path? > Exit on line 525 will be, due to do_tcp_sendpage(), and I > think pending_open_record_frags won't be set if this is the last > record. And if it is not the last record, do_tcp_sendpage will be > failing for a complete and correct record, that doesn't need to be > destroyed and at this very moment pending_open_record_frags > will suggest that there is more data (unrelated to current failing > record), which actually is correct. pending_open_record_frags does not mean more was set on previous call. It means there is an open record that needs to be closed in case cmsg needs to be sent. > I think, there won't be cmsg if pending_open_record_frags is set. cmsg comes from user space, what do you mean there won't be cmsg?
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; }
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. Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface") Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> --- net/tls/tls_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)