From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: ast@kernel.org, bpf@vger.kernel.org, daniel@iogearbox.net,
kafai@fb.com, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, songliubraving@fb.com, yhs@fb.com
Subject: Re: [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers
Date: Wed, 16 Oct 2019 11:31:35 -0700 [thread overview]
Message-ID: <20191016183133.ylo7k47o5qkygbze@ast-mbp> (raw)
In-Reply-To: <20191016111810.1799-1-christian.brauner@ubuntu.com>
On Wed, Oct 16, 2019 at 01:18:07PM +0200, Christian Brauner wrote:
> Hey everyone,
>
> This is v4. If you still feel that I should leave this code alone then
> simply ignore it. I won't send another version. Relevant tests pass and
> I've verified that other failures were already present without this
> patch series applied.
I'm looking at it the following way:
- v1 was posted with zero testing. Obviously broken patches.
- v[23] was claimed to be tested yet there were serious bugs.
Means you folks ran only the tests that I pointed out in v1.
- in v4 patch 3 now has imbalanced copy_to_user. Previously there was:
bpf_check_tail_zero+copy_from+copy_to. Now it's copy_struct_from_user+copy_to.
It's puzzling to read that code.
More so the patch removes actual_size > PAGE_SIZE check.
It's a change in behavior that commit log doesn't talk about.
- so even v4 is not ready to be merged.
- the copy_struct_from_user api was implemented by the same people who
sent buggy patches. When you guys came up with this 'generic' api
you didn't consider bpf usage and bpf_check_uarg_tail_zero() is still necessary.
- few places that were converted to copy_struct_from_user() still have
size > PAGE_SIZE. Why wasn't it part of generic?
It means that the api likely will be refactored again, but looking at the way
the patches were crafted I have no confidence that it will be thoroughly tested.
- and if I accept this set the future refactoring may break bpf side silently.
- what check_zeroed_user() is actually doing? imo it's a premature
optimization with complex implementation. Most of the time the user space passes
the size that is the same as kernel expects or smaller. Rarely user space
libs are newer than the kernel. In such case they should probe the kernel
once for new features (like libbpf does) and should not be calling kernel api
again and again to receive the same E2BIG again and again. So the fancy long read
optimization is used once in real life. Yet it's much more complex than
simple byte loop we do in bpf_check_uarg_tail_zero.
- so no, I'm not applying this. Instead I'm taking bets when this shiny new thing
will cause issues to other subsystems.
prev parent reply other threads:[~2019-10-16 18:31 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-10 10:50 ` Aleksa Sarai
2019-10-09 16:09 ` [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-10 10:51 ` Aleksa Sarai
2019-10-09 16:09 ` [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-10 10:51 ` Aleksa Sarai
2019-10-09 23:06 ` [PATCH 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
2019-10-10 9:26 ` Christian Brauner
2019-10-15 22:45 ` Alexei Starovoitov
2019-10-15 22:55 ` Christian Brauner
2019-10-15 23:02 ` Alexei Starovoitov
2019-10-15 23:08 ` Christian Brauner
2019-10-16 0:41 ` [PATCH v2 " Christian Brauner
2019-10-16 0:41 ` [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16 0:41 ` [PATCH v2 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16 0:41 ` [PATCH v2 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16 0:48 ` [PATCH v2 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-16 2:14 ` Alexei Starovoitov
2019-10-16 3:31 ` Christian Brauner
2019-10-16 3:44 ` [PATCH v3 " Christian Brauner
2019-10-16 3:44 ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16 5:23 ` Alexei Starovoitov
2019-10-16 3:44 ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16 5:25 ` Alexei Starovoitov
2019-10-16 7:30 ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()y Christian Brauner
2019-10-16 7:43 ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16 3:44 ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16 11:18 ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-16 11:18 ` [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16 11:18 ` [PATCH bpf-next v4 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16 11:18 ` [PATCH bpf-next v4 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16 18:31 ` Alexei Starovoitov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191016183133.ylo7k47o5qkygbze@ast-mbp \
--to=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=christian.brauner@ubuntu.com \
--cc=daniel@iogearbox.net \
--cc=kafai@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).