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

quoted

quoted

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Friday, September 25, 2020 3:27 AM
To: Rohit Maheshwari <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; 

vakul.garg@nxp.com; secdev [off-list ref]

quoted

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

quoted

quoted

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:

quoted

quoted

quoted

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?