bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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.


      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).