On Tue, 19 May 2020 02:55:16 +0300 Vadim Fedorenko wrote:
> 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.
Yeah, as a matter of fact checking if copied is negative is just
papering over the issue. Cleaning up the record so it can be
re-submitted again would be better.
> > 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.
Cool, thanks!
