by Benjamin Tissoires

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 06/17] HID: allow to change the report descriptor from an eBPF program

On Tue, Mar 29, 2022 at 3:53 PM Benjamin Tissoires
<[email protected]> wrote:
>
> On Mon, Mar 28, 2022 at 11:35 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Sun, Mar 27, 2022 at 11:57 PM Benjamin Tissoires
> > <[email protected]> wrote:
> > >
> > > On Fri, Mar 25, 2022 at 6:00 PM Andrii Nakryiko
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Alexei,
> > > > >
> > > > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > + struct hid_bpf_ctx_kern ctx = {
> > > > > > > + .type = HID_BPF_RDESC_FIXUP,
> > > > > > > + .hdev = hdev,
> > > > > > > + .size = *size,
> > > > > > > + };
> > > > > > > +
> > > > > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP))
> > > > > > > + goto ignore_bpf;
> > > > > > > +
> > > > > > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL);
> > > > > > > + if (!ctx.data)
> > > > > > > + goto ignore_bpf;
> > > > > > > +
> > > > > > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE;
> > > > > > > +
> > > > > > > + ret = hid_bpf_run_progs(hdev, &ctx);
> > > > > > > + if (ret)
> > > > > > > + goto ignore_bpf;
> > > > > > > +
> > > > > > > + if (ctx.size > ctx.allocated_size)
> > > > > > > + goto ignore_bpf;
> > > > > > > +
> > > > > > > + *size = ctx.size;
> > > > > > > +
> > > > > > > + if (*size) {
> > > > > > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL);
> > > > > > > + } else {
> > > > > > > + rdesc = NULL;
> > > > > > > + kfree(ctx.data);
> > > > > > > + }
> > > > > > > +
> > > > > > > + return rdesc;
> > > > > > > +
> > > > > > > + ignore_bpf:
> > > > > > > + kfree(ctx.data);
> > > > > > > + return kmemdup(rdesc, *size, GFP_KERNEL);
> > > > > > > +}
> > > > > > > +
> > > > > > > int __init hid_bpf_module_init(void)
> > > > > > > {
> > > > > > > struct bpf_hid_hooks hooks = {
> > > > > > > .hdev_from_fd = hid_bpf_fd_to_hdev,
> > > > > > > .pre_link_attach = hid_bpf_pre_link_attach,
> > > > > > > + .post_link_attach = hid_bpf_post_link_attach,
> > > > > > > .array_detach = hid_bpf_array_detach,
> > > > > > > };
> > > > > > >
> > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > > > > index 937fab7eb9c6..3182c39db006 100644
> > > > > > > --- a/drivers/hid/hid-core.c
> > > > > > > +++ b/drivers/hid/hid-core.c
> > > > > > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device)
> > > > > > > return -ENODEV;
> > > > > > > size = device->dev_rsize;
> > > > > > >
> > > > > > > - buf = kmemdup(start, size, GFP_KERNEL);
> > > > > > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */
> > > > > > > + buf = hid_bpf_report_fixup(device, start, &size);
> > > > > >
> > > > > > Looking at this patch and the majority of other patches...
> > > > > > the code is doing a lot of work to connect HID side with bpf.
> > > > > > At the same time the evolution of the patch series suggests
> > > > > > that these hook points are not quite stable. More hooks and
> > > > > > helpers are being added.
> > > > > > It tells us that it's way too early to introduce a stable
> > > > > > interface between HID and bpf.
> > > > >
> > > > > I understand that you might be under the impression that the interface
> > > > > is changing a lot, but this is mostly due to my poor knowledge of all
> > > > > the arcanes of eBPF.
> > > > > The overall way HID-BPF works is to work on a single array, and we
> > > > > should pretty much be sorted out. There are a couple of helpers to be
> > > > > able to communicate with the device, but the API has been stable in
> > > > > the kernel for those for quite some time now.
> > > > >
> > > > > The variations in the hooks is mostly because I don't know what is the
> > > > > best representation we can use in eBPF for those, and the review
> > > > > process is changing that.
> > > >
> > > > I think such a big feature as this one, especially that most BPF folks
> > > > are (probably) not familiar with the HID subsystem in the kernel,
> > > > would benefit from a bit of live discussion during BPF office hours.
> > > > Do you think you can give a short overview of what you are trying to
> > > > achieve with some background context on HID specifics at one of the
> > > > next BPF office hours? We have a meeting scheduled every week on
> > > > Thursday, 9am Pacific time. But people need to put their topic onto
> > > > the agenda, otherwise the meeting is cancelled. See [0] for
> > > > spreadsheet and links to Zoom meeting, agenda, etc.
> > >
> > > This sounds like a good idea. I just added my topic on the agenda and
> > > will prepare some slides.
> > >
> >
> > Great! Unfortunately I personally have a conflict this week and won't
> > be able to attend, so I'll have to catch up somehow through word of
> > mouth :( Next week's BPF office hours would be best, but I don't want
> > to delay discussions just because of me.
>
> OK. FWIW, I'll have slides publicly available once I'll do a final
> roundup on them. Hopefully that will give you enough context on HID to
> understand the problem.
> If there are too many conflicts we can surely delay by a week, but I
> would rather have the discussion happening sooner :/
>

Follow up on the discussion we had yesterday:

- this patchset should be dropped in its current form, because it's
the "old way" of extending BPF:

The new goal is to extend the BPF core so we work around the
limitations we find in HID so other subsystems can use the same
approach.
This is what Alexei was explaining in his answer in this thread. We
need HID to solely declare which functions are replaced (by abusing
SEC("fentry/function_name") - or fexit, fmod_ret), and also allow HID
to declare its BPF API without having to change a single bit in the
BPF core. This approach should allow other subsystems (USB???) to use
a similar approach without having to mess up with BPF too much.

- being able to attach a SEC("fentry/function") to a particular HID
device requires some changes in the BPF core

We can live without it, but it would be way cleaner to selectively
attach those trampolines to only the selected devices without having
to hand that decision to the BPF programmers

- patch 12 and 13 (the bits access helper) should be dropped

This should be done entirely in BPF, with a BPF helper as a .h that
users include instead of having to maintain yet another UAPI

- Daniel raised the question of a flag which tells other BPF that a bug is fixed

In the case we drop in the filesystem a BPF program to fix a
particular device, and then we include that same program in the
kernel, how can we know that the feature is already fixed?
It's still an open question.

- including BPF objects in the kernel is definitively doable through lskel

But again, the question is how do we attach those programs to a
particular device?

- to export a new UAPI BPF helper, we should rely on kfunc as
explained in Alexei's email

However, the current kfunc and ALLOW_ERROR_INJECTION API doesn't
clearly allow to define which function is sleepable or not, making it
currently hard to define that behavior.
Once the BPF kAPI allows to mark kfunc and ALLOW_ERROR_INJECTION
(_weak) functions to be sleepable or not, we will be able to define
the BPF hooks directly in HID without having to touch the BPF core.

- running a SEC("fentry/...") BPF program as a standalone from
userspace through a syscall is possible

- When defining a SEC("fentry/..."), all parameters are currently read-only

we either need a hid_bpf_get_data() hooks where we can mark the output
as being read-write, or we need to be able to tag the target function
as read-write for (part of) its arguments

(I would lean toward the extra helper that might be much easier to declare IMO).

I think that's it from yesterday's discussion.

Many thanks for listening to me and proposing a better solution.
Now I have a lot to work on.

Cheers,
Benjamin