On 19.05.2020 02:23, Jakub Kicinski wrote:
On Tue, 19 May 2020 02:05:29 +0300 Vadim Fedorenko wrote:
On 19.05.2020 01:30, Jakub Kicinski wrote:
tls_push_record can return -EAGAIN because of tcp layer. In that
case open_rec is already in the tx_record list and should not be
freed.
Also the record size can be more than the size requested to write
in tls_sw_do_sendpage(). That leads to overflow of copied variable
and wrong return code.
Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
Signed-off-by: Vadim Fedorenko <vfedore...@novek.ru>
Doesn't this return -EAGAIN back to user space? Meaning even tho we
queued the user space will try to send it again?
Before patch it was sending negative value back to user space.
After patch it sends the amount of data encrypted in last call. It is checked
by:
return (copied > 0) ? copied : ret;
and returns -EAGAIN only if data is not sent to open record.
I see, you're fixing two different bugs in one patch. Could you please
split the fixes into two? (BTW no need for parenthesis around the
condition in the ternary operator.) I think you need more fixes tags,
too. Commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
already added one instance of the problem, right?
Sure, will split it into two. Also the problem with overflow is possible in
tls_sw_sendmsg(). But I'm not sure about correctness of freeing whole open
record in bpf_exec_tx_verdict.
What do you think about Pooja's patch to consume the EAGAIN earlier?
There doesn't seem to be anything reasonable we can do with the error
anyway, not sure there is a point checking for it..
Yes, it's a good idea to consume this error earlier. I think it's better to fix
tls_push_record() instead of dealing with it every possible caller.
So I suggest to accept Pooja's patch and will resend only ssize_t checking fix.
