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

[Yonghong Song <yhs@meta.com>]

On 12/12/22 10:28 PM, Greg KH 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?

Currently, there is no tracepoint in bpf subsystem to avoid various
'recursion' issues. So the best approach might be providing a
callback call in related func. e.g., bpf_prog_put_deferred(),
and this callback call will provide a facility to invoke an
hid in-kernel bpf programs. This is similar to tracepoint, but it is
not exposed as a tracepoint and only available to kernel internal users.

But I didn't study the bpf usage in this patch set, but typically
bpf prog is hooked to run in a particular kernel place with bpf prog
reference count inceased so the program won't go away. So I think
once hid layer explicit removes bpf program from that hook and
decrease the reference count, hid layer might do some cleanup
work as prog might be freed anyway already or could be due to
other factors like user space closing prog fd.

> 
> confused,
> 
> greg k-h