From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 15751C433FE for ; Fri, 1 Apr 2022 13:22:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346275AbiDANXr (ORCPT ); Fri, 1 Apr 2022 09:23:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44960 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234577AbiDANXp (ORCPT ); Fri, 1 Apr 2022 09:23:45 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 861A827CE34 for ; Fri, 1 Apr 2022 06:21:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1648819312; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wvRTn/VN+9CYa+qxYCy3KNgmaSb0Q38arPgawL5F/Fs=; b=ihRPVyH9RwvjQyFMF3w9aB/XILVsjqjVGour5vdM0/aTsawDbsn3CCsYEa2FerdJWprfFQ YLVZPJmLE7jcPcFhZkpfjBfpTrikmfxRVti2ylbUcEHCVU9V9QaQ73TqaOMaQfGzl6jsF7 fHT6U2mfPhcVV3HwkBdGDXafeWdBFz4= Received: from mail-pl1-f199.google.com (mail-pl1-f199.google.com [209.85.214.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-192-YXTLti3-M6O8jRZE-Ekf2w-1; Fri, 01 Apr 2022 09:21:51 -0400 X-MC-Unique: YXTLti3-M6O8jRZE-Ekf2w-1 Received: by mail-pl1-f199.google.com with SMTP id u8-20020a170903124800b0015195a5826cso1513049plh.4 for ; Fri, 01 Apr 2022 06:21:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wvRTn/VN+9CYa+qxYCy3KNgmaSb0Q38arPgawL5F/Fs=; b=I1JVx/LJiEwK0BWOnJjzsJbukiFWS13W9D5z4Ilh1B/kaSqc6IN+d/9llRWmu75XmQ 0QjksDRtc3xbre+Ag4w99ENRQ8K6n3jsqO305rO4ekYXJVHDaYmsD/lKlQicyrUv7Vgw OaOeQ2yPmvjHZuM++fjz38bh6CsYJNU7lSlmonVSDfbhPvRrDn4Pu+U7yWlo1uM+hqo5 01mNUvIFzvk5Ig7a1xMFLLRr3dotXLcSXNJJyVnB0z4nOzjQK0pVA68DQZSiwECuBMAG sdbgBYCRws17Qen8kPLs8qTOGULTpB5LOvZ0FJ/4dBnidEHiSInFjIfe8OgUyypQNNtd 48jQ== X-Gm-Message-State: AOAM530cJJ3cxDW2ZY9MBX3coVfKHZZP8MJDEULCJRAnvk4qe28zvwx4 AiD65Lb9ZuIXp8Y7lEW/KeXTUBk6HJGrNR8/pzS2jDF5/IVD5GkMdpg3rZz379/s2/JPxsxt8R2 qDNrNW+idk9IbSQe7+MOIZUqvGoQh4TPpJDYgvnFb X-Received: by 2002:a17:90b:224b:b0:1c6:f027:90b1 with SMTP id hk11-20020a17090b224b00b001c6f02790b1mr11883572pjb.173.1648819310118; Fri, 01 Apr 2022 06:21:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyE4l2nE/NDBvRR2VtZNGdt+TrsCCIiMnKdIPRWwDuOD18HLkmLN1hyHYY6tzbZPXj2h1mCJBeXw+5oi4DPg88= X-Received: by 2002:a17:90b:224b:b0:1c6:f027:90b1 with SMTP id hk11-20020a17090b224b00b001c6f02790b1mr11883534pjb.173.1648819309710; Fri, 01 Apr 2022 06:21:49 -0700 (PDT) MIME-Version: 1.0 References: <20220318161528.1531164-1-benjamin.tissoires@redhat.com> <20220318161528.1531164-7-benjamin.tissoires@redhat.com> In-Reply-To: From: Benjamin Tissoires Date: Fri, 1 Apr 2022 15:21:38 +0200 Message-ID: Subject: Re: [PATCH bpf-next v3 06/17] HID: allow to change the report descriptor from an eBPF program To: Andrii Nakryiko , Alexei Starovoitov Cc: Greg KH , Jiri Kosina , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Shuah Khan , Dave Marchevsky , Joe Stringer , Jonathan Corbet , Tero Kristo , LKML , "open list:HID CORE LAYER" , Network Development , bpf , "open list:KERNEL SELFTEST FRAMEWORK" , "open list:DOCUMENTATION" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 29, 2022 at 3:53 PM Benjamin Tissoires wrote: > > On Mon, Mar 28, 2022 at 11:35 PM Andrii Nakryiko > wrote: > > > > On Sun, Mar 27, 2022 at 11:57 PM Benjamin Tissoires > > wrote: > > > > > > On Fri, Mar 25, 2022 at 6:00 PM Andrii Nakryiko > > > wrote: > > > > > > > > On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires > > > > wrote: > > > > > > > > > > Hi Alexei, > > > > > > > > > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov > > > > > wrote: > > > > > > > > > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > > > > > > 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