Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 16 Oct 2019 11:31:35 -0700
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
Message-ID: <20191016183133.ylo7k47o5qkygbze@ast-mbp>
References: <20191016034432.4418-1-christian.brauner@ubuntu.com>
 <20191016111810.1799-1-christian.brauner@ubuntu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191016111810.1799-1-christian.brauner@ubuntu.com>
User-Agent: NeoMutt/20180223
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.