Date:   Mon, 12 Dec 2022 09:52:03 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jiri Kosina <jikos@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Tero Kristo <tero.kristo@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of
 cleaning up programs



On 12/12/22 9:02 AM, Benjamin Tissoires wrote:
> On Thu, Nov 3, 2022 at 4:58 PM Benjamin Tissoires
> <benjamin.tissoires@...hat.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@...hat.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.

> 
> 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
> 

Powered by blists - more mailing lists