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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200928141330.54f06deb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 28 Sep 2020 14:13:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     rohit maheshwari <rohitm@...lsio.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        vakul.garg@....com, secdev <secdev@...lsio.com>
Subject: Re: [PATCH net] net/tls: sendfile fails with ktls offload

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?