Re: [PATCH v2 bpf-next 0/9] add new acquire/release BPF kfuncs

[Christian Brauner <brauner@kernel.org>]

On Fri, Mar 08, 2024 at 05:23:30PM -0800, Alexei Starovoitov wrote:
> On Fri, Mar 8, 2024 at 2:36 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> >
> > These exports are specifically for an out-of-tree BPF LSM program that
> > is not accessible to the public. The question in the other mail stands.
> 
> The question was already answered. You just don't like the answer.
> bpf progs are not equivalent to kernel modules.
> They have completely different safety and visibility properties.
> The safety part I already talked about.
> Sounds like the visibility has to be explained.
> Kernel modules are opaque binary blobs.
> bpf programs are fully transparent. The intent is known
> to the verifier and to anyone with understanding
> of bpf assembly.
> Those that cannot read bpf asm can read C source code that is
> embedded in the bpf program in kernel memory.
> It's not the same as "llvm-dwarfdump module.ko" on disk.
> The bpf prog source code is loaded into the kernel
> at program verification time for debugging and visibility reasons.
> If there is a verifier bug and bpf manages to crash the kernel
> vmcore will have relevant lines of program C source code right there.
> 
> Hence out-of-tree or in-tree bpf makes no practical difference.
> The program cannot hide its meaning and doesn't hamper debugging.
> 
> Hence adding EXPORT_SYMBOL == Brace for impact!
> Expect crashes, api misuse and what not.
> 
> While adding bpf_kfunc is a nop for kernel development.
> If kfunc is in the way of code refactoring it can be removed
> (as we demonstrated several times).
> A kfunc won't cause headaches for the kernel code it is
> calling (assuming no verifier bugs).
> If there is a bug it's on us to fix it as we demonstrated in the past.
> For example: bpf_probe_read_kernel().
> It's a wrapper of copy_from_kernel_nofault() and over the years
> bpf users hit various bugs in copy_from_kernel_nofault(),
> reported them, and _bpf developers_ fixed them.
> Though copy_from_kernel_nofault() is as generic as it can get
> and the same bugs could have been reproduced without bpf
> we took care of fixing these parts of the kernel.
> 
> Look at path_put().
> It's EXPORT_SYMBOL and any kernel module can easily screw up
> reference counting, so that sooner or later distro folks
> will experience debug pains due to out-of-tree drivers.
> 
> kfunc that calls path_put() won't have such consequences.
> The verifier will prevent path_put() on a pointer that wasn't
> acquired by the same bpf program. No support pains.
> It's a nop for vfs folks.
> 
> > > First of all, there is no such thing as get_task_fs_pwd/root
> > > in the kernel.
> >
> > Yeah, we'd need specific helpers for a never seen before out-of-tree BPF
> > LSM. I don't see how that's different from an out-of-tree kernel module.
> 
> Sorry, but you don't seem to understand what bpf can and cannot do,
> hence they look similar.

Maybe. On the other hand you seem to ignore what I'm saying. You
currently don't have a clear set of rules for when it's ok for someone
to send patches and request access to bpf kfuncs to implement a new BPF
program. This patchset very much illustrates this point. The safety
properties of bpf don't matter for this. And again, your safety
properties very much didn't protect you from your bpf_d_path() mess.

We're not even clearly told where and how these helper are supposed to be
used. That's not ok and will never be ok. As long as there are no clear
criteria to operate under this is highly problematic. This may be fine
from a bpf perspective and one can even understand why because that's
apparently your model or promise to your users. But there's no reason to
expect the same level of laxness from any of the subsystems you're
requesting kfuncs from.

> > > One can argue that get_mm_exe_file() is not exported,
> > > but it's nothing but rcu_lock-wrap plus get_file_rcu()
> > > which is EXPORT_SYMBOL.
> >
> > Oh, good spot. That's an accident. get_file_rcu() definitely shouldn't
> > be exported. So that'll be removed asap.
> 
> So, just to make a point that
> "Included in that set are functions that aren't currently even
> exported to modules"
> you want to un-export get_file_rcu() ?

No. The reason it was exported was because of the drm subsystem and we
already quite disliked that. But it turned out that's not needed so in
commit 61d4fb0b349e ("file, i915: fix file reference for
mmap_singleton()") they were moved away from this helper.

And then we simply forgot to unexport it.

A helper such as get_file_rcu() is called on a file object that is
subject to SLAB_TYPESAFE_BY_RCU semantics where the caller doesn't hold
a reference. The semantics of that are maybe understood by a couple of
people in the kernel. There is absolutely no way that any userspace will
get access to such low-level helpers. They have zero business to be
involved in the lifetimes of objects on this level just as no module has.

So really, this is an orthogonal cleanup.