| Message ID | 20191016111810.1799-1-christian.brauner@ubuntu.com |
|---|---|
| Headers | show |
| Series | bpf: switch to new usercopy helpers | expand |
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.
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. The misplaced min check has been moved after copy_struct_from_user() so no non-zero bytes can be missed by copy_struct_from_user(). In v5.4-rc2 we added two new helpers check_zeroed_user() and copy_struct_from_user() including selftests (cf. [1]). It is a generic interface designed to copy a struct from userspace. The helpers will be especially useful for structs versioned by size of which we have quite a few. The most obvious benefit is that this helper lets us get rid of duplicate code. We've already switched over sched_setattr(), perf_event_open(), and clone3(). More importantly it will also help to ensure that users implementing versioning-by-size end up with the same core semantics. This point is especially crucial since we have at least one case where versioning-by-size is used but with slighly different semantics: sched_setattr(), perf_event_open(), and clone3() all do do similar checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects differently-sized struct arguments. This little series switches over bpf codepaths that have hand-rolled implementations of these helpers. /* v1 */ Link: https://lore.kernel.org/r/20191009160907.10981-1-christian.brauner@ubuntu.com /* v2 */ Link: https://lore.kernel.org/r/20191016004138.24845-1-christian.brauner@ubuntu.com - rebase onto bpf-next /* v3 */ Link: https://lore.kernel.org/r/20191016034432.4418-1-christian.brauner@ubuntu.com /* Reference */ [1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper") Christian Brauner (3): bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() bpf: use copy_struct_from_user() in bpf() syscall kernel/bpf/syscall.c | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-)