Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of cleaning up programs

[Yonghong Song]

On 12/12/22 9:02 AM, Benjamin Tissoires wrote:
> On Thu, Nov 3, 2022 at 4:58 PM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> > Kind of a hack, but works for now:
> >
> > Instead of listening for any close of eBPF program, we now
> > decrement the refcount when we insert it in our internal
> > map of fd progs.
> >
> > This is safe to do because:
> > - we listen to any call of destructor of programs
> > - when a program is being destroyed, we disable it by removing
> >    it from any RCU list used by any HID device (so it will never
> >    be called)
> > - we then trigger a job to cleanup the prog fd map, but we overwrite
> >    the removal of the elements to not do anything on the programs, just
> >    remove the allocated space
> >
> > This is better than previously because we can remove the map of known
> > programs and their usage count. We now rely on the refcount of
> > bpf, which has greater chances of being accurate.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > ---
>
> So... I am a little bit embarrassed, but it turns out that this hack
> is not safe enough.
>
> If I compile the kernel with LLVM=1, the function
> bpf_prog_put_deferred() is optimized in a weird way: if we are not in
> irq, the function is inlined into __bpf_prog_put(), but if we are, the
> function is still kept around as it is called in a scheduled work
> item.
>
> This is something I completely overlooked: I assume that if the
> function would be inlined, the HID entrypoint BPF preloaded object
> would not be able to bind, thus deactivating HID-BPF safely. But if a
> function can be both inlined and not inlined, then I have no
> guarantees that my cleanup call will be called. Meaning that a HID
> device might believe there is still a bpf function to call. And things
> will get messy, with kernel crashes and others.

You should not rely fentry to a static function. This is unstable
as compiler could inline it if that static function is called
directly. You could attach to a global function if it is not
compiled with lto.

> An easy "fix" would be to tag bpf_prog_put_deferred() with "noinline",
> but it just feels wrong to have that for this specific reason.

This is not a right approach just for this purpose.

> AFAICT, gcc is not doing that optimisation, but nothing prevents it
> from doing it, and suddenly that will be a big whole in the kernel.
>
> As much as I wish I had another option, I think for the sake of
> everyone (and for my future holidays) I'll postpone HID-BPF to 6.3.
>
> I actually thought of another way of removing that trampoline call. So
> I'm not entirely going back to the drawing board hopefully.
>
> [a few hours laters]
>
> Just as a preview, I am reusing the bpf_link idea: when we call
> hid_bpf_attach_prog(), this creates a bpf_link, and that link is the
> one that needs to be pinned. Whenever all the references of that link
> are dropped, I get called in the link's ->release() function, and I
> can force the unbinding of the hid-device to the program at that time.
>
> Way safer (no refcount mess up) and no optimisations can interfere,
> now that I am not "tracing" the bpf core code.
>
> Cheers,
> Benjamin