Messages in this thread | | | Date | Mon, 12 Dec 2022 10:39:26 -0800 | Subject | Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of cleaning up programs | From | Yonghong Song <> |
| |
On 12/12/22 10:20 AM, Greg KH wrote: > On Mon, Dec 12, 2022 at 09:52:03AM -0800, Yonghong Song wrote: >> >> >> On 12/12/22 9:02 AM, Benjamin Tissoires wrote: >>> On Thu, Nov 3, 2022 at 4:58 PM Benjamin Tissoires >>> <benjamin.tissoires@redhat.com> 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@redhat.com> >>>> >>>> --- >>> >>> 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. > > But now that the kernel does support LTO, how can you be sure this will > always work properly? The code author does not know if LTO will kick in > and optimize this away or not, that's the linker's job.
Ya, that is right. So for in-kernel bpf programs, attaching to global functions are not safe either. For other not-in-kernel bpf programs, it may not work but that is user's responsibility to adjust properly (to different functions based on a particular build, etc.).
> > thanks, > > greg k-h
| |