netdev - Re: [bpf PATCH 1/5] bpf, sockmap: fix partial copy_page_to

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: <5fad936eeee38_2a6120874@john-XPS-13-9370.notmuch>
Date:   Thu, 12 Nov 2020 11:56:30 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>, ast@...nel.org,
        jakub@...udflare.com
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [bpf PATCH 1/5] bpf, sockmap: fix partial copy_page_to_iter so
 progress can still be made

Daniel Borkmann wrote:
> On 11/7/20 8:37 PM, John Fastabend wrote:
> > If copy_page_to_iter() fails or even partially completes, but with fewer
> > bytes copied than expected we currently reset sg.start and return EFAULT.
> > This proves problematic if we already copied data into the user buffer
> > before we return an error. Because we leave the copied data in the user
> > buffer and fail to unwind the scatterlist so kernel side believes data
> > has been copied and user side believes data has _not_ been received.

[...]

> > +			if (!copy) {
> > +				return copied ? copied : -EFAULT;
> >   			}
> 
> nit: no need for {}
> 
> >   
> >   			copied += copy;
> > @@ -56,6 +55,11 @@ int __tcp_bpf_recvmsg(struct sock *sk, struct sk_psock *psock,
> >   						put_page(page);
> >   				}
> >   			} else {
> > +				/* Lets not optimize peek case if copy_page_to_iter
> > +				 * didn't copy the entire length lets just break.
> > +				 */
> > +				if (copy != sge->length)
> > +					goto out;
> 
> nit: return copied;
> 
> Rest lgtm for this one.

Great, thanks for the review will fixup in v2.