On Tue, Dec 13, 2022 at 7:28 AM Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Dec 12, 2022 at 10:39:26AM -0800, Yonghong Song wrote:
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.).
So if in-kernel bpf programs will not work or are not safe, how will in-kernel bpf programs properly attach?
Sorry if that wasn't clear. Loading a bpf program from the kernel is fine and safe. But it was the use of it that wasn't.
In my case, HID-BPF to fix devices is safe (whether the program is loaded from the kernel or from userspace): the bpf JIT/verifier ensures that there are no out of bound read/write and the API is properly defined. But the problem I am facing with the generic bpf implementation is that it is made to be a global processing and to attach to one given function, when I wanted to have a couple function + device.
So in this patch, I actually abused BPF to get free event notifications when the bpf program was released. The first implementation (HID: initial BPF implementation) was safer than this patch because I was using BPF for notifications of my internals but I wasn't messing up with the reference count. So if I did not get the events, I wouldn't decrement the bpf_prog and the end result means that the bpf program would stay forever attached to the device. Not user friendly but it doesn't introduce a kernel crash.
However, this patch is messing with reference counting of an internal kernel object assuming I would always get the event. This is not the case and so I get read after free errors.
TL;DR: (ab)using BPF internally for kernel introspection to manage kernel structures is just plain wrong. Mea culpa.
Cheers, Benjamin