| Message ID | 20220224110828.2168231-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Thu, Feb 24, 2022 at 02:49:21PM +0100, Benjamin Tissoires wrote: > Hi Greg, > > Thanks for the quick answer :) > > On Thu, Feb 24, 2022 at 12:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Feb 24, 2022 at 12:08:22PM +0100, Benjamin Tissoires wrote: > > > Hi there, > > > > > > This series introduces support of eBPF for HID devices. > > > > > > I have several use cases where eBPF could be interesting for those > > > input devices: > > > > > > - simple fixup of report descriptor: > > > > > > In the HID tree, we have half of the drivers that are "simple" and > > > that just fix one key or one byte in the report descriptor. > > > Currently, for users of such devices, the process of fixing them > > > is long and painful. > > > With eBPF, we could externalize those fixups in one external repo, > > > ship various CoRe bpf programs and have those programs loaded at boot > > > time without having to install a new kernel (and wait 6 months for the > > > fix to land in the distro kernel) > > > > Why would a distro update such an external repo faster than they update > > the kernel? Many sane distros update their kernel faster than other > > packages already, how about fixing your distro? :) > > Heh, I'm going to try to dodge the incoming rhel bullet :) > > It's true that thanks to the work of the stable folks we don't have to > wait 6 months for a fix to come in. However, I think having a single > file to drop in a directory would be easier for development/testing > (and distribution of that file between developers/testers) than > requiring people to recompile their kernel. > > Brain fart: is there any chance we could keep the validated bpf > programs in the kernel tree? That would make the most sense to me. And allow "slow" distros to override the HID bpf quirks easily if they need to. If you do that, then most of my objections of the "now the code is in two places that you have to track" goes away :) > > I'm all for the idea of using ebpf for HID devices, but now we have to > > keep track of multiple packages to be in sync here. Is this making > > things harder overall? > > Probably, and this is also maybe opening a can of worms. Vendors will > be able to say "use that bpf program for my HID device because the > firmware is bogus". > > OTOH, as far as I understand, you can not load a BPF program in the > kernel that uses GPL-declared functions if your BPF program is not > GPL. Which means that if firmware vendors want to distribute blobs > through BPF, either it's GPL and they have to provide the sources, or > it's not happening. You can make the new HID bpf api only availble to GPL programs, and if I were you, that's what I would do just to keep any legal issues from coming up. Also bundling it with the kernel makes it easier. > I am not entirely clear on which plan I want to have for userspace. > I'd like to have libinput on board, but right now, Peter's stance is > "not in my garden" (and he has good reasons for it). > So my initial plan is to cook and hold the bpf programs in hid-tools, > which is the repo I am using for the regression tests on HID. Why isn't the hid regression tests in the kernel tree also? That would allow all of the testers out there to test things much easier than having to suck down another test repo (like Linaro and 0-day and kernelci would be forced to do). > I plan on building a systemd intrinsic that would detect the HID > VID/PID and then load the various BPF programs associated with the > small fixes. > Note that everything can not be fixed through eBPF, simply because at > boot we don't always have the root partition mounted. Root partitions are now on HID devices? :) > > > - Universal Stylus Interface (or any other new fancy feature that > > > requires a new kernel API) > > > > > > See [0]. > > > Basically, USI pens are requiring a new kernel API because there are > > > some channels of communication our HID and input stack are not capable > > > of. Instead of using hidraw or creating new sysfs or ioctls, we can rely > > > on eBPF to have the kernel API controlled by the consumer and to not > > > impact the performances by waking up userspace every time there is an > > > event. > > > > How is userspace supposed to interact with these devices in a unified > > way then? This would allow random new interfaces to be created, one > > each for each device, and again, be a pain to track for a distro to keep > > in sync. And how are you going to keep the ebpf interface these > > provides in sync with the userspace program? > > Right now, the idea we have is to export the USI specifics through > dbus. This has a couple of advantages: we are not tied to USI and can > "emulate" those parameters by storing them on disk instead of in the > pen, and this is easily accessible from all applications directly. > > I am trying to push to have one implementation of that dbus service > with the Intel and ChromeOS folks so general linux doesn't have to > recreate it. But if you look at it, with hidraw nothing prevents > someone from writing such a library/daemon in its own world without > sharing it with anybody. > > The main advantage of eBPF compared to hidraw is that you can analyse > the incoming event without waking userspace and only wake it up when > there is something noticeable. That is a very good benefit, and one that many battery-powered devices would like. > In terms of random interfaces, yes, this is a good point. But the way > I see it is that we can provide one kernel API (eBPF for HID) which we > will maintain and not have to maintain forever a badly designed kernel > API for a specific device. Though also note that USI is a HID standard > (I think there is a second one), so AFAICT, the same bpf program > should be able to be generic enough to be cross vendor. So there will > be one provider only for USI. Ok, that's good to know. <good stuff snipped> > Yeah, I completely understand the view. However, please keep in mind > that most of it (though not firewall and some corner cases of tracing) > is already possible to do through hidraw. > One other example of that is SDL. We got Sony involved to create a > nice driver for the DualSense controller (the PS5 one), but they > simply ignore it and use plain HID (through hidraw). They have the > advantage of this being cross-platform and can provide a consistent > experience across platforms. And as a result, in the kernel, we have > to hands up the handling of the device whenever somebody opens a > hidraw node for those devices (Steam is also doing the same FWIW). > > Which reminds me that I also have another use-case: joystick > dead-zone. You can have a small filter that configures the dead zone > and doesn't even wake up userspace for those hardware glitches... hidraw is a big issue, and I understand why vendors use that and prefer it over writing a kernel driver. They can control it and ship it to users and it makes life easier for them. It's also what Windows has been doing for decades now, so it's a comfortable interface for them to write their code in userspace. But, now you are going to ask them to use bpf instead? Why would they switch off of hidraw to use this api? What benefit are you going to provide them here for that? This is why I've delayed doing bpf for USB. Right now we have a nice cross-platform way to write userspace USB drivers using usbfs/libusb. All a bpf api to USB would be doing is much the same thing that libusb does, for almost no real added benefit that I can tell. USB, is a really "simple" networking like protocol (send/recieve packets). So it ties into the bpf model well. But the need to use bpf for USB so far I don't have a real justification. And the same thing here. Yes it is cool, and personally I love it, but what is going to get people off of hidraw to use this instead? You can'd drop hidraw now (just like I can't drop usbfs), so all this means is we have yet-another-way to do something on the system. Is that a good idea? I don't know. Also you mention dbus as the carrier for the HID information to userspace programs. Is dbus really the right thing for sending streams of input data around? Yes it will probably work, but I don't think it was designed for that at all, so the overhead involved might just overshadow any of the improvements you made using bpf. And also, you could do this today with hidraw, right? > Anyway, IOW, I think the bpf approach will allow kernel-like > performances of hidraw applications, and I would be more inclined to > ask people to move their weird issue in userspace thanks to that. I like this from a "everyone should use bpf" point of view, but how are you going to tell people "use bpf over hidraw" in a way that gets them to do so? If you have a good answer for that, I might just steal it for the bpf-USB interface as well :) thanks, greg k-h
On Fri, Feb 25, 2022 at 2:38 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Feb 24, 2022 at 02:49:21PM +0100, Benjamin Tissoires wrote: > > Hi Greg, > > > > Thanks for the quick answer :) > > > > On Thu, Feb 24, 2022 at 12:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Feb 24, 2022 at 12:08:22PM +0100, Benjamin Tissoires wrote: > > > > Hi there, > > > > > > > > This series introduces support of eBPF for HID devices. > > > > > > > > I have several use cases where eBPF could be interesting for those > > > > input devices: > > > > > > > > - simple fixup of report descriptor: > > > > > > > > In the HID tree, we have half of the drivers that are "simple" and > > > > that just fix one key or one byte in the report descriptor. > > > > Currently, for users of such devices, the process of fixing them > > > > is long and painful. > > > > With eBPF, we could externalize those fixups in one external repo, > > > > ship various CoRe bpf programs and have those programs loaded at boot > > > > time without having to install a new kernel (and wait 6 months for the > > > > fix to land in the distro kernel) > > > > > > Why would a distro update such an external repo faster than they update > > > the kernel? Many sane distros update their kernel faster than other > > > packages already, how about fixing your distro? :) > > > > Heh, I'm going to try to dodge the incoming rhel bullet :) > > > > It's true that thanks to the work of the stable folks we don't have to > > wait 6 months for a fix to come in. However, I think having a single > > file to drop in a directory would be easier for development/testing > > (and distribution of that file between developers/testers) than > > requiring people to recompile their kernel. > > > > Brain fart: is there any chance we could keep the validated bpf > > programs in the kernel tree? > > That would make the most sense to me. And allow "slow" distros to > override the HID bpf quirks easily if they need to. If you do that, > then most of my objections of the "now the code is in two places that > you have to track" goes away :) Unfortunately, I don't think we will be able to have all the bpf programs in the kernel. I would say programs that "enable" a device (report descriptor fixup, events override, single key remapping) which are there to make the device fully functional could likely be stored and loaded by the kernel. I have yet to figure out if the suggestion from Yonghong Song allows me to also load the bpf program or if it's just a repo in the tree for the sources. However, for USI pens (or any other functionality that needs a new high level kernel interface) I don't think we will have them here. Unless we want to also bind the public API to dbus (or graybus maybe), the bpf program should live in the userspace program so it can update it and not be tied to the decisions in the kernel (I'll go more into detail later in this reply). So that's going to be a half and half solution :/ > > > > I'm all for the idea of using ebpf for HID devices, but now we have to > > > keep track of multiple packages to be in sync here. Is this making > > > things harder overall? > > > > Probably, and this is also maybe opening a can of worms. Vendors will > > be able to say "use that bpf program for my HID device because the > > firmware is bogus". > > > > OTOH, as far as I understand, you can not load a BPF program in the > > kernel that uses GPL-declared functions if your BPF program is not > > GPL. Which means that if firmware vendors want to distribute blobs > > through BPF, either it's GPL and they have to provide the sources, or > > it's not happening. > > You can make the new HID bpf api only availble to GPL programs, and if I > were you, that's what I would do just to keep any legal issues from > coming up. Also bundling it with the kernel makes it easier. Looking at kernel/bpf/bpf_lsm.c I can confirm that it should be easy to prevent a program from binding to HID if it's not GPL :) > > > I am not entirely clear on which plan I want to have for userspace. > > I'd like to have libinput on board, but right now, Peter's stance is > > "not in my garden" (and he has good reasons for it). > > So my initial plan is to cook and hold the bpf programs in hid-tools, > > which is the repo I am using for the regression tests on HID. > > Why isn't the hid regression tests in the kernel tree also? That would > allow all of the testers out there to test things much easier than > having to suck down another test repo (like Linaro and 0-day and > kernelci would be forced to do). 2 years ago I would have argued that the ease of development of gitlab.fd.o was more suited to a fast moving project. Now... The changes in the core part of the code don't change much so yes, merging it in the kernel might have a lot of benefits outside of what you said. The most immediate one is that I could require fixes to be provided with a test, and merge them together, without having to hold them until Linus releases a new version. If nobody complains of having the regression tests in python with pytest and some Python 3.6+ features, that is definitely something I should look for. > > > I plan on building a systemd intrinsic that would detect the HID > > VID/PID and then load the various BPF programs associated with the > > small fixes. > > Note that everything can not be fixed through eBPF, simply because at > > boot we don't always have the root partition mounted. > > Root partitions are now on HID devices? :) Sorry for not being clear :) I mean that if you need a bpf program to be loaded from userspace at boot to make your keyboard functional, then you need to have the root partition mounted (or put the program in the initrd) so udev can load it. Now if your keyboard is supposed to give the password used to decrypt your root partition but you need a bpf program on that said partition to make it functional, you are screwed :) > > > > > - Universal Stylus Interface (or any other new fancy feature that > > > > requires a new kernel API) > > > > > > > > See [0]. > > > > Basically, USI pens are requiring a new kernel API because there are > > > > some channels of communication our HID and input stack are not capable > > > > of. Instead of using hidraw or creating new sysfs or ioctls, we can rely > > > > on eBPF to have the kernel API controlled by the consumer and to not > > > > impact the performances by waking up userspace every time there is an > > > > event. > > > > > > How is userspace supposed to interact with these devices in a unified > > > way then? This would allow random new interfaces to be created, one > > > each for each device, and again, be a pain to track for a distro to keep > > > in sync. And how are you going to keep the ebpf interface these > > > provides in sync with the userspace program? > > > > Right now, the idea we have is to export the USI specifics through > > dbus. This has a couple of advantages: we are not tied to USI and can > > "emulate" those parameters by storing them on disk instead of in the > > pen, and this is easily accessible from all applications directly. > > > > I am trying to push to have one implementation of that dbus service > > with the Intel and ChromeOS folks so general linux doesn't have to > > recreate it. But if you look at it, with hidraw nothing prevents > > someone from writing such a library/daemon in its own world without > > sharing it with anybody. > > > > The main advantage of eBPF compared to hidraw is that you can analyse > > the incoming event without waking userspace and only wake it up when > > there is something noticeable. > > That is a very good benefit, and one that many battery-powered devices > would like. > > > In terms of random interfaces, yes, this is a good point. But the way > > I see it is that we can provide one kernel API (eBPF for HID) which we > > will maintain and not have to maintain forever a badly designed kernel > > API for a specific device. Though also note that USI is a HID standard > > (I think there is a second one), so AFAICT, the same bpf program > > should be able to be generic enough to be cross vendor. So there will > > be one provider only for USI. > > Ok, that's good to know. > > <good stuff snipped> > > > Yeah, I completely understand the view. However, please keep in mind > > that most of it (though not firewall and some corner cases of tracing) > > is already possible to do through hidraw. > > One other example of that is SDL. We got Sony involved to create a > > nice driver for the DualSense controller (the PS5 one), but they > > simply ignore it and use plain HID (through hidraw). They have the > > advantage of this being cross-platform and can provide a consistent > > experience across platforms. And as a result, in the kernel, we have > > to hands up the handling of the device whenever somebody opens a > > hidraw node for those devices (Steam is also doing the same FWIW). > > > > Which reminds me that I also have another use-case: joystick > > dead-zone. You can have a small filter that configures the dead zone > > and doesn't even wake up userspace for those hardware glitches... > > hidraw is a big issue, and I understand why vendors use that and prefer > it over writing a kernel driver. They can control it and ship it to > users and it makes life easier for them. It's also what Windows has > been doing for decades now, so it's a comfortable interface for them to > write their code in userspace. > > But, now you are going to ask them to use bpf instead? Why would they > switch off of hidraw to use this api? What benefit are you going to > provide them here for that? Again, there are 2 big classes of users of hid-bpf ("you" here is a developer in general): 1. you need to fix your device 2. you need to add a new kernel API 2. can be entirely done with hidraw: - you open the hidraw node - you parse every incoming event - out of that you build up your high level API This is what we are doing in libratbag for instance to support gaming mice that need extra features. We try to not read every single event, but some mice are done in a way we don't have a choice. With bpf, you could: - load the bpf program - have the kernel (bpf program) parse the incoming report without waking up user space - when something changes (a given button is pressed) the bpf program notifies userspace with an event - then userspace builds its own API on top of it (forward that change through dbus for example) As far as 1., the aim is not to replace hidraw but the kernel drivers themselves: instead of having a formal driver loaded in the kernel, you can rely on a bpf program to do whatever needs to be done to make the device working. If the FW is wrong and the report descriptor messes up a button mapping, you can change that with bpf instead of having a specific driver for it. And of course, using hidraw for that just doesn't work because the event stream you get from hidraw is for the process only. In BPF, we can change the event flow for anybody, which allows much more power (but this is scarier too). This class of bpf program should actually reside in the kernel tree so everybody can benefit from it (circling back to your first point). So I am not deprecating hidraw nor I am not asking them to use bpf instead. But when you are interested in just one byte in the report, bpf allows you to speed up your program and save battery. > > This is why I've delayed doing bpf for USB. Right now we have a nice > cross-platform way to write userspace USB drivers using usbfs/libusb. > All a bpf api to USB would be doing is much the same thing that libusb > does, for almost no real added benefit that I can tell. > > USB, is a really "simple" networking like protocol (send/recieve > packets). So it ties into the bpf model well. But the need to use bpf > for USB so far I don't have a real justification. > > And the same thing here. Yes it is cool, and personally I love it, but > what is going to get people off of hidraw to use this instead? You > can'd drop hidraw now (just like I can't drop usbfs), so all this means > is we have yet-another-way to do something on the system. Is that a > good idea? I don't know. > > Also you mention dbus as the carrier for the HID information to > userspace programs. Is dbus really the right thing for sending streams > of input data around? Yes it will probably work, but I don't think it > was designed for that at all, so the overhead involved might just > overshadow any of the improvements you made using bpf. And also, you > could do this today with hidraw, right? Let me go into more details regarding USI. Peter drafted a WIP for the dbus API at https://gitlab.freedesktop.org/whot/usid. Unfortunately that's the only tangent thing I can share right now for it, and it doesn't even have the bpf part ;). Basically the dbus API will export just the USI bits we care about: - Preferred Color: the 8-bit Web color or 24 bit RGB color assigned to the device - Preferred Line Width: the physical width (e.g. 2mm) - Preferred Line Style: One of Ink, Pencil, Highlighter, Chisel Marker, Brush, No Preference. The rest of the event stream will still continue to go through evdev, and not dbus. The bpf program we should have here parses the incoming report and whenever there is a change in those 3 properties above, it raises an event to the dbus daemon which will in turn forward that to its clients. On the other side, when a client needs to have a color change for instance, it sets the dbus property and then the dbus daemon might either store the data on disk or on the pen through bpf and HID. So yes, you can do that with hidraw but that means you need to parse all incoming reports in userspace for events that will likely occur every once in a while. In my mind, the dbus operations here are to replace the kernel API you might want to create instead. In that case, Tero started with 2 in-kernel possibilities: a sysfs entry we could read/write, or a new ioctl attached to the evdev node (it was actually a separate char device). > > > Anyway, IOW, I think the bpf approach will allow kernel-like > > performances of hidraw applications, and I would be more inclined to > > ask people to move their weird issue in userspace thanks to that. > > I like this from a "everyone should use bpf" point of view, but how are > you going to tell people "use bpf over hidraw" in a way that gets them > to do so? If you have a good answer for that, I might just steal it for > the bpf-USB interface as well :) > I don't think we can have a generic answer here unfortunately. It will depend on the use case: - SDL/Steam -> they are interested in the entire stream of events, so we can't really tell them to use bpf, unless if they want to fix a deadzone of a joystick - if the idea is to fix the device (spurious event, wrong key mapping, drift of one coordinate), hidraw doesn't apply, and bpf is IMO better than writing a kernel module as long as we can ship them with the kernel (because of the cost of compiling a driver/kernel and testing is much higher than just inserting a BPF program in the running kernel) - if the idea is to add a new kernel API or functionality to a device (a new haptic FF implementation, some USI properties, something unique enough to not be generic and have a properly defined API), then bpf is a good choice because by tying the bpf program with the high level API from userspace we can ensure we don't have to maintain this kernel API with all its bugs forever. And here, you could use hidraw but if you need to filter a small amount of information in the event stream instead of just using Set/Get reports, then BPF is much more efficient. I don't know enough about the USB use case you have for libusb (I know mostly the input ones ;-P) to be able to give you an answer there. Cheers, Benjamin
On Thu, Feb 24, 2022 at 6:21 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 2/24/22 5:49 AM, Benjamin Tissoires wrote: > > Hi Greg, > > > > Thanks for the quick answer :) > > > > On Thu, Feb 24, 2022 at 12:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >> On Thu, Feb 24, 2022 at 12:08:22PM +0100, Benjamin Tissoires wrote: > >>> Hi there, > >>> > >>> This series introduces support of eBPF for HID devices. > >>> > >>> I have several use cases where eBPF could be interesting for those > >>> input devices: > >>> > >>> - simple fixup of report descriptor: > >>> > >>> In the HID tree, we have half of the drivers that are "simple" and > >>> that just fix one key or one byte in the report descriptor. > >>> Currently, for users of such devices, the process of fixing them > >>> is long and painful. > >>> With eBPF, we could externalize those fixups in one external repo, > >>> ship various CoRe bpf programs and have those programs loaded at boot > >>> time without having to install a new kernel (and wait 6 months for the > >>> fix to land in the distro kernel) > >> > >> Why would a distro update such an external repo faster than they update > >> the kernel? Many sane distros update their kernel faster than other > >> packages already, how about fixing your distro? :) > > > > Heh, I'm going to try to dodge the incoming rhel bullet :) > > > > It's true that thanks to the work of the stable folks we don't have to > > wait 6 months for a fix to come in. However, I think having a single > > file to drop in a directory would be easier for development/testing > > (and distribution of that file between developers/testers) than > > requiring people to recompile their kernel. > > > > Brain fart: is there any chance we could keep the validated bpf > > programs in the kernel tree? > > Yes, see kernel/bpf/preload/iterators/iterators.bpf.c. Thanks. This is indeed interesting. I am not sure the exact usage of it though :) One thing I wonder too while we are on this topic, is it possible to load a bpf program from the kernel directly, in the same way we can request firmwares? Because if we can do that, in my HID use case we could replace simple drivers with bpf programs entirely and reduce the development cycle to a bare minimum. Cheers, Benjamin > > > > >> > >> I'm all for the idea of using ebpf for HID devices, but now we have to > >> keep track of multiple packages to be in sync here. Is this making > >> things harder overall? > > > > Probably, and this is also maybe opening a can of worms. Vendors will > > be able to say "use that bpf program for my HID device because the > > firmware is bogus". > > > > OTOH, as far as I understand, you can not load a BPF program in the > > kernel that uses GPL-declared functions if your BPF program is not > > GPL. Which means that if firmware vendors want to distribute blobs > > through BPF, either it's GPL and they have to provide the sources, or > > it's not happening. > > > > I am not entirely clear on which plan I want to have for userspace. > > I'd like to have libinput on board, but right now, Peter's stance is > > "not in my garden" (and he has good reasons for it). > > So my initial plan is to cook and hold the bpf programs in hid-tools, > > which is the repo I am using for the regression tests on HID. > > > > I plan on building a systemd intrinsic that would detect the HID > > VID/PID and then load the various BPF programs associated with the > > small fixes. > > Note that everything can not be fixed through eBPF, simply because at > > boot we don't always have the root partition mounted. > [...] >
On Fri, Feb 25, 2022 at 05:06:32PM +0100, Benjamin Tissoires wrote: > On Thu, Feb 24, 2022 at 6:21 PM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 2/24/22 5:49 AM, Benjamin Tissoires wrote: > > > Hi Greg, > > > > > > Thanks for the quick answer :) > > > > > > On Thu, Feb 24, 2022 at 12:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > >> > > >> On Thu, Feb 24, 2022 at 12:08:22PM +0100, Benjamin Tissoires wrote: > > >>> Hi there, > > >>> > > >>> This series introduces support of eBPF for HID devices. > > >>> > > >>> I have several use cases where eBPF could be interesting for those > > >>> input devices: > > >>> > > >>> - simple fixup of report descriptor: > > >>> > > >>> In the HID tree, we have half of the drivers that are "simple" and > > >>> that just fix one key or one byte in the report descriptor. > > >>> Currently, for users of such devices, the process of fixing them > > >>> is long and painful. > > >>> With eBPF, we could externalize those fixups in one external repo, > > >>> ship various CoRe bpf programs and have those programs loaded at boot > > >>> time without having to install a new kernel (and wait 6 months for the > > >>> fix to land in the distro kernel) > > >> > > >> Why would a distro update such an external repo faster than they update > > >> the kernel? Many sane distros update their kernel faster than other > > >> packages already, how about fixing your distro? :) > > > > > > Heh, I'm going to try to dodge the incoming rhel bullet :) > > > > > > It's true that thanks to the work of the stable folks we don't have to > > > wait 6 months for a fix to come in. However, I think having a single > > > file to drop in a directory would be easier for development/testing > > > (and distribution of that file between developers/testers) than > > > requiring people to recompile their kernel. > > > > > > Brain fart: is there any chance we could keep the validated bpf > > > programs in the kernel tree? > > > > Yes, see kernel/bpf/preload/iterators/iterators.bpf.c. > > Thanks. This is indeed interesting. > I am not sure the exact usage of it though :) > > One thing I wonder too while we are on this topic, is it possible to > load a bpf program from the kernel directly, in the same way we can > request firmwares? We used to be able to do that, putting bpf programs inside a module. But that might have gotten removed because no one actually used it. I thought it was a nice idea. > Because if we can do that, in my HID use case we could replace simple > drivers with bpf programs entirely and reduce the development cycle to > a bare minimum. How would the development cycle change? You could get rid of many in-kernel hid drivers and replace them with bpf code perhaps? Maybe that's a good use case :) thanks, greg k-h
HID selftests question for now: On Fri, Feb 25, 2022 at 05:00:53PM +0100, Benjamin Tissoires wrote: > > > I am not entirely clear on which plan I want to have for userspace. > > > I'd like to have libinput on board, but right now, Peter's stance is > > > "not in my garden" (and he has good reasons for it). > > > So my initial plan is to cook and hold the bpf programs in hid-tools, > > > which is the repo I am using for the regression tests on HID. > > > > Why isn't the hid regression tests in the kernel tree also? That would > > allow all of the testers out there to test things much easier than > > having to suck down another test repo (like Linaro and 0-day and > > kernelci would be forced to do). > > 2 years ago I would have argued that the ease of development of > gitlab.fd.o was more suited to a fast moving project. > > Now... The changes in the core part of the code don't change much so > yes, merging it in the kernel might have a lot of benefits outside of > what you said. The most immediate one is that I could require fixes to > be provided with a test, and merge them together, without having to > hold them until Linus releases a new version. Yes, having a test be required for a fix is a great idea. Many subsystems do this already and it helps a lot. > If nobody complains of having the regression tests in python with > pytest and some Python 3.6+ features, that is definitely something I > should look for. Look at the tools/testing/selftests/ directory today. We already have python3 tests in there, and as long as you follow the proper TAP output format, all should be fine. The tc-testing python code in the kernel trees seems to do that and no one has complained yet :) thanks, greg k-h
On 2/25/22 8:06 AM, Benjamin Tissoires wrote: > On Thu, Feb 24, 2022 at 6:21 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 2/24/22 5:49 AM, Benjamin Tissoires wrote: >>> Hi Greg, >>> >>> Thanks for the quick answer :) >>> >>> On Thu, Feb 24, 2022 at 12:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: >>>> >>>> On Thu, Feb 24, 2022 at 12:08:22PM +0100, Benjamin Tissoires wrote: >>>>> Hi there, >>>>> >>>>> This series introduces support of eBPF for HID devices. >>>>> >>>>> I have several use cases where eBPF could be interesting for those >>>>> input devices: >>>>> >>>>> - simple fixup of report descriptor: >>>>> >>>>> In the HID tree, we have half of the drivers that are "simple" and >>>>> that just fix one key or one byte in the report descriptor. >>>>> Currently, for users of such devices, the process of fixing them >>>>> is long and painful. >>>>> With eBPF, we could externalize those fixups in one external repo, >>>>> ship various CoRe bpf programs and have those programs loaded at boot >>>>> time without having to install a new kernel (and wait 6 months for the >>>>> fix to land in the distro kernel) >>>> >>>> Why would a distro update such an external repo faster than they update >>>> the kernel? Many sane distros update their kernel faster than other >>>> packages already, how about fixing your distro? :) >>> >>> Heh, I'm going to try to dodge the incoming rhel bullet :) >>> >>> It's true that thanks to the work of the stable folks we don't have to >>> wait 6 months for a fix to come in. However, I think having a single >>> file to drop in a directory would be easier for development/testing >>> (and distribution of that file between developers/testers) than >>> requiring people to recompile their kernel. >>> >>> Brain fart: is there any chance we could keep the validated bpf >>> programs in the kernel tree? >> >> Yes, see kernel/bpf/preload/iterators/iterators.bpf.c. > > Thanks. This is indeed interesting. > I am not sure the exact usage of it though :) > > One thing I wonder too while we are on this topic, is it possible to > load a bpf program from the kernel directly, in the same way we can > request firmwares? Yes. You can. See the example at kernel/bpf/preload directory. The example will pin the link (holding a reference to the program) into bpffs (implemented in kernel/bpf/inode.c). Later on, in userspace, you can grab a fd from bpffs pinned link and use BPF_LINK_UPDATE to update the program if you want. This way, if your driver uses the link to get the program, they will automatically get the new program in the next run. > > Because if we can do that, in my HID use case we could replace simple > drivers with bpf programs entirely and reduce the development cycle to > a bare minimum. > > Cheers, > Benjamin > > >> >>> >>>> >>>> I'm all for the idea of using ebpf for HID devices, but now we have to >>>> keep track of multiple packages to be in sync here. Is this making >>>> things harder overall? >>> >>> Probably, and this is also maybe opening a can of worms. Vendors will >>> be able to say "use that bpf program for my HID device because the >>> firmware is bogus". >>> >>> OTOH, as far as I understand, you can not load a BPF program in the >>> kernel that uses GPL-declared functions if your BPF program is not >>> GPL. Which means that if firmware vendors want to distribute blobs >>> through BPF, either it's GPL and they have to provide the sources, or >>> it's not happening. >>> >>> I am not entirely clear on which plan I want to have for userspace. >>> I'd like to have libinput on board, but right now, Peter's stance is >>> "not in my garden" (and he has good reasons for it). >>> So my initial plan is to cook and hold the bpf programs in hid-tools, >>> which is the repo I am using for the regression tests on HID. >>> >>> I plan on building a systemd intrinsic that would detect the HID >>> VID/PID and then load the various BPF programs associated with the >>> small fixes. >>> Note that everything can not be fixed through eBPF, simply because at >>> boot we don't always have the root partition mounted. >> [...] >> >
On Fri, Feb 25, 2022 at 05:00:53PM +0100, Benjamin Tissoires wrote: > > > I plan on building a systemd intrinsic that would detect the HID > > > VID/PID and then load the various BPF programs associated with the > > > small fixes. > > > Note that everything can not be fixed through eBPF, simply because at > > > boot we don't always have the root partition mounted. > > > > Root partitions are now on HID devices? :) > > Sorry for not being clear :) > > I mean that if you need a bpf program to be loaded from userspace at > boot to make your keyboard functional, then you need to have the root > partition mounted (or put the program in the initrd) so udev can load > it. Now if your keyboard is supposed to give the password used to > decrypt your root partition but you need a bpf program on that said > partition to make it functional, you are screwed :) True, but that's why the HID boot protocol was designed for keyboards and mice, so that they "always" work. Yeah, I know many devices ignore it, oh well... Anyway, the requirement of "if you need it to boot, don't make it a bpf program" is fine, unless you can put the bpf program in a kernel module (see my other response for that.) > > > > > - Universal Stylus Interface (or any other new fancy feature that > > > > > requires a new kernel API) > > > > > > > > > > See [0]. > > > > > Basically, USI pens are requiring a new kernel API because there are > > > > > some channels of communication our HID and input stack are not capable > > > > > of. Instead of using hidraw or creating new sysfs or ioctls, we can rely > > > > > on eBPF to have the kernel API controlled by the consumer and to not > > > > > impact the performances by waking up userspace every time there is an > > > > > event. > > > > > > > > How is userspace supposed to interact with these devices in a unified > > > > way then? This would allow random new interfaces to be created, one > > > > each for each device, and again, be a pain to track for a distro to keep > > > > in sync. And how are you going to keep the ebpf interface these > > > > provides in sync with the userspace program? > > > > > > Right now, the idea we have is to export the USI specifics through > > > dbus. This has a couple of advantages: we are not tied to USI and can > > > "emulate" those parameters by storing them on disk instead of in the > > > pen, and this is easily accessible from all applications directly. > > > > > > I am trying to push to have one implementation of that dbus service > > > with the Intel and ChromeOS folks so general linux doesn't have to > > > recreate it. But if you look at it, with hidraw nothing prevents > > > someone from writing such a library/daemon in its own world without > > > sharing it with anybody. > > > > > > The main advantage of eBPF compared to hidraw is that you can analyse > > > the incoming event without waking userspace and only wake it up when > > > there is something noticeable. > > > > That is a very good benefit, and one that many battery-powered devices > > would like. > > > > > In terms of random interfaces, yes, this is a good point. But the way > > > I see it is that we can provide one kernel API (eBPF for HID) which we > > > will maintain and not have to maintain forever a badly designed kernel > > > API for a specific device. Though also note that USI is a HID standard > > > (I think there is a second one), so AFAICT, the same bpf program > > > should be able to be generic enough to be cross vendor. So there will > > > be one provider only for USI. > > > > Ok, that's good to know. > > > > <good stuff snipped> > > > > > Yeah, I completely understand the view. However, please keep in mind > > > that most of it (though not firewall and some corner cases of tracing) > > > is already possible to do through hidraw. > > > One other example of that is SDL. We got Sony involved to create a > > > nice driver for the DualSense controller (the PS5 one), but they > > > simply ignore it and use plain HID (through hidraw). They have the > > > advantage of this being cross-platform and can provide a consistent > > > experience across platforms. And as a result, in the kernel, we have > > > to hands up the handling of the device whenever somebody opens a > > > hidraw node for those devices (Steam is also doing the same FWIW). > > > > > > Which reminds me that I also have another use-case: joystick > > > dead-zone. You can have a small filter that configures the dead zone > > > and doesn't even wake up userspace for those hardware glitches... > > > > hidraw is a big issue, and I understand why vendors use that and prefer > > it over writing a kernel driver. They can control it and ship it to > > users and it makes life easier for them. It's also what Windows has > > been doing for decades now, so it's a comfortable interface for them to > > write their code in userspace. > > > > But, now you are going to ask them to use bpf instead? Why would they > > switch off of hidraw to use this api? What benefit are you going to > > provide them here for that? > > Again, there are 2 big classes of users of hid-bpf ("you" here is a > developer in general): > 1. you need to fix your device > 2. you need to add a new kernel API > > 2. can be entirely done with hidraw: > - you open the hidraw node > - you parse every incoming event > - out of that you build up your high level API > > This is what we are doing in libratbag for instance to support gaming > mice that need extra features. We try to not read every single event, > but some mice are done in a way we don't have a choice. > > With bpf, you could: > - load the bpf program > - have the kernel (bpf program) parse the incoming report without > waking up user space > - when something changes (a given button is pressed) the bpf program > notifies userspace with an event > - then userspace builds its own API on top of it (forward that change > through dbus for example) > > As far as 1., the aim is not to replace hidraw but the kernel drivers > themselves: > instead of having a formal driver loaded in the kernel, you can rely > on a bpf program to do whatever needs to be done to make the device > working. > > If the FW is wrong and the report descriptor messes up a button > mapping, you can change that with bpf instead of having a specific > driver for it. > And of course, using hidraw for that just doesn't work because the > event stream you get from hidraw is for the process only. In BPF, we > can change the event flow for anybody, which allows much more power > (but this is scarier too). > > This class of bpf program should actually reside in the kernel tree so > everybody can benefit from it (circling back to your first point). > > So I am not deprecating hidraw nor I am not asking them to use bpf instead. > But when you are interested in just one byte in the report, bpf allows > you to speed up your program and save battery. Ah, so really you are using bpf here as a "filter" for the HID events that you care about to send to userspace or act apon in some way. That makes a lot more sense to me, sorry for not realizing it sooner. So yes, I agree, HID control with bpf does make sense. You can fix up and filter out only the events you want without getting userspace involved if it does not match. That should make people's lives easier (hopefully) and based on your example code you provide in this patch series, it doesn't look all that complex. Along this line, now I think I know what we can do for USB with bpf as well. Much the same thing, like a smart filter, which is what bpf was designed for. USB is just a stream of data like a network connection with pipes and the like, so it will work quite well for this. Ok, thanks for the explanations, you've sold me, nice work :) One comment about the patch series. You might want to break the patches up a bit smaller, having the example code in a separate commit from the "add this feature" commit, as it was hard to pick out what was kernel changes, and what was test changes from it. That way I can complain about the example code and tests without having to worry about the kernel patches. thanks, greg k-h
On Thu, Feb 24, 2022 at 3:09 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > HID is a protocol that could benefit from using BPF too. > > This patch implements a net-like use of BPF capability for HID. > Any incoming report coming from the device gets injected into a series > of BPF programs that can modify it or even discard it by setting the > size in the context to 0. > > The kernel/bpf implementation is based on net-namespace.c, with only > the bpf_link part kept, there is no real points in keeping the > bpf_prog_{attach|detach} API. > > The implementation is split into 2 parts: > - the kernel/bpf part which isn't aware of the HID usage, but takes care > of handling the BPF links > - the drivers/hid/hid-bpf.c part which knows about HID > > Note that HID can be compiled in as a module, and so the functions that > kernel/bpf/hid.c needs to call in hid.ko are exported in struct hid_hooks. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/hid/Makefile | 1 + > drivers/hid/hid-bpf.c | 176 ++++++++ > drivers/hid/hid-core.c | 21 +- > include/linux/bpf-hid.h | 87 ++++ > include/linux/bpf_types.h | 4 + > include/linux/hid.h | 16 + > include/uapi/linux/bpf.h | 7 + > include/uapi/linux/bpf_hid.h | 39 ++ > kernel/bpf/Makefile | 3 + > kernel/bpf/hid.c | 437 +++++++++++++++++++ > kernel/bpf/syscall.c | 8 + > samples/bpf/.gitignore | 1 + > samples/bpf/Makefile | 4 + > samples/bpf/hid_mouse_kern.c | 66 +++ > samples/bpf/hid_mouse_user.c | 129 ++++++ > tools/include/uapi/linux/bpf.h | 7 + > tools/lib/bpf/libbpf.c | 7 + > tools/lib/bpf/libbpf.h | 2 + > tools/lib/bpf/libbpf.map | 1 + > tools/testing/selftests/bpf/prog_tests/hid.c | 318 ++++++++++++++ > tools/testing/selftests/bpf/progs/hid.c | 20 + > 21 files changed, 1351 insertions(+), 3 deletions(-) > create mode 100644 drivers/hid/hid-bpf.c > create mode 100644 include/linux/bpf-hid.h > create mode 100644 include/uapi/linux/bpf_hid.h > create mode 100644 kernel/bpf/hid.c > create mode 100644 samples/bpf/hid_mouse_kern.c > create mode 100644 samples/bpf/hid_mouse_user.c > create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c > create mode 100644 tools/testing/selftests/bpf/progs/hid.c Please split kernel changes, libbpf changes, selftests, and sample code into separate patches. > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 6d3e630e81af..08d2d7619937 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -4,6 +4,7 @@ > # > hid-y := hid-core.o hid-input.o hid-quirks.o > hid-$(CONFIG_DEBUG_FS) += hid-debug.o > +hid-$(CONFIG_BPF) += hid-bpf.o > > obj-$(CONFIG_HID) += hid.o > obj-$(CONFIG_UHID) += uhid.o > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > new file mode 100644 > index 000000000000..6c8445820944 > --- /dev/null > +++ b/drivers/hid/hid-bpf.c > @@ -0,0 +1,176 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * BPF in HID support for Linux > + * > + * Copyright (c) 2021 Benjamin Tissoires Maybe 2022? [...] > +static int hid_bpf_run_progs(struct hid_device *hdev, enum bpf_hid_attach_type type, > + struct hid_bpf_ctx *ctx, u8 *data, int size) > +{ > + enum hid_bpf_event event = HID_BPF_UNDEF; > + > + if (type < 0 || !ctx) > + return -EINVAL; > + > + switch (type) { > + case BPF_HID_ATTACH_DEVICE_EVENT: > + event = HID_BPF_DEVICE_EVENT; > + if (size > sizeof(ctx->u.device.data)) > + return -E2BIG; > + break; > + default: > + return -EINVAL; > + } > + > + if (!hdev->bpf.run_array[type]) > + return 0; > + > + memset(ctx, 0, sizeof(*ctx)); > + ctx->hdev = hdev; > + ctx->type = event; > + > + if (size && data) { > + switch (event) { > + case HID_BPF_DEVICE_EVENT: > + memcpy(ctx->u.device.data, data, size); > + ctx->u.device.size = size; > + break; > + default: > + /* do nothing */ > + } > + } > + > + BPF_PROG_RUN_ARRAY(hdev->bpf.run_array[type], ctx, bpf_prog_run); I guess we need "return BPF_PROG_RUN_ARRAY(...)"? > + > + return 0; > +} > + > +u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) > +{ > + int ret; > + > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_DEVICE_EVENT)) > + return data; > + > + ret = hid_bpf_run_progs(hdev, BPF_HID_ATTACH_DEVICE_EVENT, > + hdev->bpf.ctx, data, *size); > + if (ret) > + return data; shall we return ERR_PTR(ret)? > + > + if (!hdev->bpf.ctx->u.device.size) > + return ERR_PTR(-EINVAL); > + > + *size = hdev->bpf.ctx->u.device.size; > + > + return hdev->bpf.ctx->u.device.data; > +} [...] > diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h > new file mode 100644 > index 000000000000..243ac45a253f > --- /dev/null > +++ b/include/uapi/linux/bpf_hid.h > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ > + > +/* > + * HID BPF public headers > + * > + * Copyright (c) 2021 Benjamin Tissoires > + */ > + > +#ifndef _UAPI__LINUX_BPF_HID_H__ > +#define _UAPI__LINUX_BPF_HID_H__ > + > +#include <linux/types.h> > + > +#define HID_BPF_MAX_BUFFER_SIZE 16384 /* 16kb */ > + > +struct hid_device; > + > +enum hid_bpf_event { > + HID_BPF_UNDEF = 0, > + HID_BPF_DEVICE_EVENT, > +}; > + > +/* type is HID_BPF_DEVICE_EVENT */ > +struct hid_bpf_ctx_device_event { > + __u8 data[HID_BPF_MAX_BUFFER_SIZE]; 16kB sounds pretty big to me, do we usually need that much? > + unsigned long size; > +}; > + > +struct hid_bpf_ctx { > + enum hid_bpf_event type; > + struct hid_device *hdev; > + > + union { > + struct hid_bpf_ctx_device_event device; > + } u; > +}; > + > +#endif /* _UAPI__LINUX_BPF_HID_H__ */ [...] > diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c > new file mode 100644 > index 000000000000..d3cb952bfc26 > --- /dev/null > +++ b/kernel/bpf/hid.c [...] > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 9c7a72b65eee..230ca6964a7e 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3,6 +3,7 @@ > */ > #include <linux/bpf.h> > #include <linux/bpf-cgroup.h> > +#include <linux/bpf-hid.h> > #include <linux/bpf_trace.h> > #include <linux/bpf_lirc.h> > #include <linux/bpf_verifier.h> > @@ -2174,6 +2175,7 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > case BPF_PROG_TYPE_CGROUP_SYSCTL: > case BPF_PROG_TYPE_SOCK_OPS: > case BPF_PROG_TYPE_EXT: /* extends any prog */ > + case BPF_PROG_TYPE_HID: Is this net_admin type? > return true; > case BPF_PROG_TYPE_CGROUP_SKB: > /* always unpriv */ > @@ -3188,6 +3190,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) > return BPF_PROG_TYPE_SK_LOOKUP; > case BPF_XDP: > return BPF_PROG_TYPE_XDP; > + case BPF_HID_DEVICE_EVENT: > + return BPF_PROG_TYPE_HID; > default: > return BPF_PROG_TYPE_UNSPEC; > } > @@ -3331,6 +3335,8 @@ static int bpf_prog_query(const union bpf_attr *attr, > case BPF_SK_MSG_VERDICT: > case BPF_SK_SKB_VERDICT: > return sock_map_bpf_prog_query(attr, uattr); > + case BPF_HID_DEVICE_EVENT: > + return bpf_hid_prog_query(attr, uattr); > default: > return -EINVAL; > } > @@ -4325,6 +4331,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr) > ret = bpf_perf_link_attach(attr, prog); > break; > #endif > + case BPF_PROG_TYPE_HID: > + return bpf_hid_link_create(attr, prog); > default: > ret = -EINVAL; > } [...] > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index afe3d0d7f5f2..5978b92cacd3 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -952,6 +952,7 @@ enum bpf_prog_type { > BPF_PROG_TYPE_LSM, > BPF_PROG_TYPE_SK_LOOKUP, > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ > + BPF_PROG_TYPE_HID, > }; > > enum bpf_attach_type { > @@ -997,6 +998,7 @@ enum bpf_attach_type { > BPF_SK_REUSEPORT_SELECT, > BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, > BPF_PERF_EVENT, > + BPF_HID_DEVICE_EVENT, > __MAX_BPF_ATTACH_TYPE > }; > > @@ -1011,6 +1013,7 @@ enum bpf_link_type { > BPF_LINK_TYPE_NETNS = 5, > BPF_LINK_TYPE_XDP = 6, > BPF_LINK_TYPE_PERF_EVENT = 7, > + BPF_LINK_TYPE_HID = 8, > > MAX_BPF_LINK_TYPE, > }; > @@ -5870,6 +5873,10 @@ struct bpf_link_info { > struct { > __u32 ifindex; > } xdp; > + struct { > + __s32 hidraw_ino; > + __u32 attach_type; > + } hid; > }; > } __attribute__((aligned(8))); >
On Fri, Feb 25, 2022 at 8:32 AM Greg KH <gregkh@linuxfoundation.org> wrote: > [...] > > One comment about the patch series. You might want to break the patches > up a bit smaller, having the example code in a separate commit from the > "add this feature" commit, as it was hard to pick out what was kernel > changes, and what was test changes from it. That way I can complain > about the example code and tests without having to worry about the > kernel patches. Echo on this part. Please organize kernel changes, libbpf changes, maybe also bpftool changes, selftests, and samples into separate patches. This would help folks without HID experience understand the design. Thanks, Song
On Sat, Feb 26, 2022 at 8:37 AM Song Liu <song@kernel.org> wrote: > > On Fri, Feb 25, 2022 at 8:32 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > [...] > > > > One comment about the patch series. You might want to break the patches > > up a bit smaller, having the example code in a separate commit from the > > "add this feature" commit, as it was hard to pick out what was kernel > > changes, and what was test changes from it. That way I can complain > > about the example code and tests without having to worry about the > > kernel patches. > > Echo on this part. Please organize kernel changes, libbpf changes, > maybe also bpftool changes, selftests, and samples into separate patches. > This would help folks without HID experience understand the design. > Sure. And thanks for the initial review. I'll send out a splitted v2 early next week then. Cheers, Benjamin
On Fri, 25 Feb 2022, Greg KH wrote: > > I mean that if you need a bpf program to be loaded from userspace at > > boot to make your keyboard functional, then you need to have the root > > partition mounted (or put the program in the initrd) so udev can load > > it. Now if your keyboard is supposed to give the password used to > > decrypt your root partition but you need a bpf program on that said > > partition to make it functional, you are screwed :) > > True, but that's why the HID boot protocol was designed for keyboards > and mice, so that they "always" work. Yeah, I know many devices ignore > it, oh well... That's a very mild statement :) *Most* of the recent modern HW doesn't support it as far as I can say. Thanks,
On Thu, Feb 24, 2022 at 12:41 PM Greg KH <greg@kroah.com> wrote: > > On Thu, Feb 24, 2022 at 12:08:23PM +0100, Benjamin Tissoires wrote: > > index 000000000000..243ac45a253f > > --- /dev/null > > +++ b/include/uapi/linux/bpf_hid.h > > @@ -0,0 +1,39 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ > > + > > +/* > > + * HID BPF public headers > > + * > > + * Copyright (c) 2021 Benjamin Tissoires > > + */ > > + > > +#ifndef _UAPI__LINUX_BPF_HID_H__ > > +#define _UAPI__LINUX_BPF_HID_H__ > > + > > +#include <linux/types.h> > > + > > +#define HID_BPF_MAX_BUFFER_SIZE 16384 /* 16kb */ > > + > > +struct hid_device; > > + > > +enum hid_bpf_event { > > + HID_BPF_UNDEF = 0, > > + HID_BPF_DEVICE_EVENT, > > +}; > > + > > +/* type is HID_BPF_DEVICE_EVENT */ > > +struct hid_bpf_ctx_device_event { > > + __u8 data[HID_BPF_MAX_BUFFER_SIZE]; > > + unsigned long size; > > That's not a valid type to cross the user/kernel boundry, shouldn't it > be "__u64"? But really, isn't __u32 enough here? Fixed locally with a __u16 instead. Will be present in v2. Cheers, Benjamin
On Sat, Feb 26, 2022 at 8:27 AM Song Liu <song@kernel.org> wrote: > > On Thu, Feb 24, 2022 at 3:09 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > HID is a protocol that could benefit from using BPF too. > > > > This patch implements a net-like use of BPF capability for HID. > > Any incoming report coming from the device gets injected into a series > > of BPF programs that can modify it or even discard it by setting the > > size in the context to 0. > > > > The kernel/bpf implementation is based on net-namespace.c, with only > > the bpf_link part kept, there is no real points in keeping the > > bpf_prog_{attach|detach} API. > > > > The implementation is split into 2 parts: > > - the kernel/bpf part which isn't aware of the HID usage, but takes care > > of handling the BPF links > > - the drivers/hid/hid-bpf.c part which knows about HID > > > > Note that HID can be compiled in as a module, and so the functions that > > kernel/bpf/hid.c needs to call in hid.ko are exported in struct hid_hooks. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > drivers/hid/Makefile | 1 + > > drivers/hid/hid-bpf.c | 176 ++++++++ > > drivers/hid/hid-core.c | 21 +- > > include/linux/bpf-hid.h | 87 ++++ > > include/linux/bpf_types.h | 4 + > > include/linux/hid.h | 16 + > > include/uapi/linux/bpf.h | 7 + > > include/uapi/linux/bpf_hid.h | 39 ++ > > kernel/bpf/Makefile | 3 + > > kernel/bpf/hid.c | 437 +++++++++++++++++++ > > kernel/bpf/syscall.c | 8 + > > samples/bpf/.gitignore | 1 + > > samples/bpf/Makefile | 4 + > > samples/bpf/hid_mouse_kern.c | 66 +++ > > samples/bpf/hid_mouse_user.c | 129 ++++++ > > tools/include/uapi/linux/bpf.h | 7 + > > tools/lib/bpf/libbpf.c | 7 + > > tools/lib/bpf/libbpf.h | 2 + > > tools/lib/bpf/libbpf.map | 1 + > > tools/testing/selftests/bpf/prog_tests/hid.c | 318 ++++++++++++++ > > tools/testing/selftests/bpf/progs/hid.c | 20 + > > 21 files changed, 1351 insertions(+), 3 deletions(-) > > create mode 100644 drivers/hid/hid-bpf.c > > create mode 100644 include/linux/bpf-hid.h > > create mode 100644 include/uapi/linux/bpf_hid.h > > create mode 100644 kernel/bpf/hid.c > > create mode 100644 samples/bpf/hid_mouse_kern.c > > create mode 100644 samples/bpf/hid_mouse_user.c > > create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c > > create mode 100644 tools/testing/selftests/bpf/progs/hid.c > > Please split kernel changes, libbpf changes, selftests, and sample code into > separate patches. OK, done locally. > > > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > > index 6d3e630e81af..08d2d7619937 100644 > > --- a/drivers/hid/Makefile > > +++ b/drivers/hid/Makefile > > @@ -4,6 +4,7 @@ > > # > > hid-y := hid-core.o hid-input.o hid-quirks.o > > hid-$(CONFIG_DEBUG_FS) += hid-debug.o > > +hid-$(CONFIG_BPF) += hid-bpf.o > > > > obj-$(CONFIG_HID) += hid.o > > obj-$(CONFIG_UHID) += uhid.o > > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > > new file mode 100644 > > index 000000000000..6c8445820944 > > --- /dev/null > > +++ b/drivers/hid/hid-bpf.c > > @@ -0,0 +1,176 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * BPF in HID support for Linux > > + * > > + * Copyright (c) 2021 Benjamin Tissoires > > Maybe 2022? heh, maybe :) > > [...] > > > +static int hid_bpf_run_progs(struct hid_device *hdev, enum bpf_hid_attach_type type, > > + struct hid_bpf_ctx *ctx, u8 *data, int size) > > +{ > > + enum hid_bpf_event event = HID_BPF_UNDEF; > > + > > + if (type < 0 || !ctx) > > + return -EINVAL; > > + > > + switch (type) { > > + case BPF_HID_ATTACH_DEVICE_EVENT: > > + event = HID_BPF_DEVICE_EVENT; > > + if (size > sizeof(ctx->u.device.data)) > > + return -E2BIG; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (!hdev->bpf.run_array[type]) > > + return 0; > > + > > + memset(ctx, 0, sizeof(*ctx)); > > + ctx->hdev = hdev; > > + ctx->type = event; > > + > > + if (size && data) { > > + switch (event) { > > + case HID_BPF_DEVICE_EVENT: > > + memcpy(ctx->u.device.data, data, size); > > + ctx->u.device.size = size; > > + break; > > + default: > > + /* do nothing */ > > + } > > + } > > + > > + BPF_PROG_RUN_ARRAY(hdev->bpf.run_array[type], ctx, bpf_prog_run); > > I guess we need "return BPF_PROG_RUN_ARRAY(...)"? ack > > > + > > + return 0; > > +} > > + > > +u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) > > +{ > > + int ret; > > + > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_DEVICE_EVENT)) > > + return data; > > + > > + ret = hid_bpf_run_progs(hdev, BPF_HID_ATTACH_DEVICE_EVENT, > > + hdev->bpf.ctx, data, *size); > > + if (ret) > > + return data; > > shall we return ERR_PTR(ret)? I initially wanted to have a bpf_program returning something other than 0 being a signal to silently ignore the report. But the API will be more consistent if we simply return an error in the same way we do for bpf.ctx->u.device.size just below. IOW, will change in v2 > > > + > > + if (!hdev->bpf.ctx->u.device.size) > > + return ERR_PTR(-EINVAL); > > + > > + *size = hdev->bpf.ctx->u.device.size; > > + > > + return hdev->bpf.ctx->u.device.data; > > +} > > [...] > > > diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h > > new file mode 100644 > > index 000000000000..243ac45a253f > > --- /dev/null > > +++ b/include/uapi/linux/bpf_hid.h > > @@ -0,0 +1,39 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ > > + > > +/* > > + * HID BPF public headers > > + * > > + * Copyright (c) 2021 Benjamin Tissoires > > + */ > > + > > +#ifndef _UAPI__LINUX_BPF_HID_H__ > > +#define _UAPI__LINUX_BPF_HID_H__ > > + > > +#include <linux/types.h> > > + > > +#define HID_BPF_MAX_BUFFER_SIZE 16384 /* 16kb */ > > + > > +struct hid_device; > > + > > +enum hid_bpf_event { > > + HID_BPF_UNDEF = 0, > > + HID_BPF_DEVICE_EVENT, > > +}; > > + > > +/* type is HID_BPF_DEVICE_EVENT */ > > +struct hid_bpf_ctx_device_event { > > + __u8 data[HID_BPF_MAX_BUFFER_SIZE]; > > 16kB sounds pretty big to me, do we usually need that much? That's painful but it seems so: see commit 6a0eaf5123e0 ("HID: Increase HID maximum report size to 16KB"). I wanted to have a static definition of the buffer, but maybe I could terminate the struct with `_u8 data[]` and dynamically (re-)alloc the buffer depending on the context. If the verifier doesn't reject that (why would it? given that it should rely on hid_is_valid_access()), I'll implement this in v2. > > > + unsigned long size; > > +}; > > + > > +struct hid_bpf_ctx { > > + enum hid_bpf_event type; > > + struct hid_device *hdev; > > + > > + union { > > + struct hid_bpf_ctx_device_event device; > > + } u; > > +}; > > + > > +#endif /* _UAPI__LINUX_BPF_HID_H__ */ > [...] > > > diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c > > new file mode 100644 > > index 000000000000..d3cb952bfc26 > > --- /dev/null > > +++ b/kernel/bpf/hid.c > > [...] > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 9c7a72b65eee..230ca6964a7e 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -3,6 +3,7 @@ > > */ > > #include <linux/bpf.h> > > #include <linux/bpf-cgroup.h> > > +#include <linux/bpf-hid.h> > > #include <linux/bpf_trace.h> > > #include <linux/bpf_lirc.h> > > #include <linux/bpf_verifier.h> > > @@ -2174,6 +2175,7 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > > case BPF_PROG_TYPE_CGROUP_SYSCTL: > > case BPF_PROG_TYPE_SOCK_OPS: > > case BPF_PROG_TYPE_EXT: /* extends any prog */ > > + case BPF_PROG_TYPE_HID: > > Is this net_admin type? Not really :) I initially copied over from the LIRC2 code, which is something quite similar in terms of abuse of BPF. Maybe I should add an extra patch before introducing is_sys_admin_prog_type() and move over LIRC2 there before adding HID. > > > return true; > > case BPF_PROG_TYPE_CGROUP_SKB: > > /* always unpriv */ > > @@ -3188,6 +3190,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) > > return BPF_PROG_TYPE_SK_LOOKUP; > > case BPF_XDP: > > return BPF_PROG_TYPE_XDP; > > + case BPF_HID_DEVICE_EVENT: > > + return BPF_PROG_TYPE_HID; > > default: > > return BPF_PROG_TYPE_UNSPEC; > > } > > @@ -3331,6 +3335,8 @@ static int bpf_prog_query(const union bpf_attr *attr, > > case BPF_SK_MSG_VERDICT: > > case BPF_SK_SKB_VERDICT: > > return sock_map_bpf_prog_query(attr, uattr); > > + case BPF_HID_DEVICE_EVENT: > > + return bpf_hid_prog_query(attr, uattr); > > default: > > return -EINVAL; > > } > > @@ -4325,6 +4331,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr) > > ret = bpf_perf_link_attach(attr, prog); > > break; > > #endif > > + case BPF_PROG_TYPE_HID: > > + return bpf_hid_link_create(attr, prog); > > default: > > ret = -EINVAL; > > } > > [...] > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > index afe3d0d7f5f2..5978b92cacd3 100644 > > --- a/tools/include/uapi/linux/bpf.h > > +++ b/tools/include/uapi/linux/bpf.h > > @@ -952,6 +952,7 @@ enum bpf_prog_type { > > BPF_PROG_TYPE_LSM, > > BPF_PROG_TYPE_SK_LOOKUP, > > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ > > + BPF_PROG_TYPE_HID, > > }; > > > > enum bpf_attach_type { > > @@ -997,6 +998,7 @@ enum bpf_attach_type { > > BPF_SK_REUSEPORT_SELECT, > > BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, > > BPF_PERF_EVENT, > > + BPF_HID_DEVICE_EVENT, > > __MAX_BPF_ATTACH_TYPE > > }; > > > > @@ -1011,6 +1013,7 @@ enum bpf_link_type { > > BPF_LINK_TYPE_NETNS = 5, > > BPF_LINK_TYPE_XDP = 6, > > BPF_LINK_TYPE_PERF_EVENT = 7, > > + BPF_LINK_TYPE_HID = 8, > > > > MAX_BPF_LINK_TYPE, > > }; > > @@ -5870,6 +5873,10 @@ struct bpf_link_info { > > struct { > > __u32 ifindex; > > } xdp; > > + struct { > > + __s32 hidraw_ino; > > + __u32 attach_type; > > + } hid; > > }; > > } __attribute__((aligned(8))); > > > And thanks for the initial review :) Cheers, Benjamin
Hi there, This series introduces support of eBPF for HID devices. I have several use cases where eBPF could be interesting for those input devices: - simple fixup of report descriptor: In the HID tree, we have half of the drivers that are "simple" and that just fix one key or one byte in the report descriptor. Currently, for users of such devices, the process of fixing them is long and painful. With eBPF, we could externalize those fixups in one external repo, ship various CoRe bpf programs and have those programs loaded at boot time without having to install a new kernel (and wait 6 months for the fix to land in the distro kernel) - Universal Stylus Interface (or any other new fancy feature that requires a new kernel API) See [0]. Basically, USI pens are requiring a new kernel API because there are some channels of communication our HID and input stack are not capable of. Instead of using hidraw or creating new sysfs or ioctls, we can rely on eBPF to have the kernel API controlled by the consumer and to not impact the performances by waking up userspace every time there is an event. - Surface Dial This device is a "puck" from Microsoft, basically a rotary dial with a push button. The kernel already exports it as such but doesn't handle the haptic feedback we can get out of it. Furthermore, that device is not recognized by userspace and so it's a nice paperwight in the end. With eBPF, we can morph that device into a mouse, and convert the dial events into wheel events. Also, we can set/unset the haptic feedback from userspace. The convenient part of BPF makes it that the kernel doesn't make any choice that would need to be reverted because that specific userspace doesn't handle it properly or because that other one expects it to be different. - firewall What if we want to prevent other users to access a specific feature of a device? (think a possibly bonker firmware update entry popint) With eBPF, we can intercept any HID command emitted to the device and validate it or not. This also allows to sync the state between the userspace and the kernel/bpf program because we can intercept any incoming command. - tracing The last usage I have in mind is tracing events and all the fun we can do we BPF to summarize and analyze events. Right now, tracing relies on hidraw. It works well except for a couple of issues: 1. if the driver doesn't export a hidraw node, we can't trace anything (eBPF will be a "god-mode" there, so it might raise some eyebrows) 2. hidraw doesn't catch the other process requests to the device, which means that we have cases where we need to add printks to the kernel to understand what is happening. With that long introduction, here is the v1 of the support of eBPF in HID. I have targeted bpf-next here because the parts that will have the most conflicts are in bpf. There might be a trivial minor conflict in include/linux/hid.h with an other series I have pending[1]. I am relatively new to bpf, so having some feedback would be most very welcome. A couple of notes though: - The series is missing a SEC("hid/driver_event") which would allow to intercept incoming requests to the device from anybody. I left it outside because it's not critical to have it from day one (we are more interested right now by the USI case above) - I am still wondering how to integrate the tracing part: right now, if a bpf program is loaded before we start the tracer, we will see *modified* events in the tracer. However, it might be interesting to decide to see either unmodified (raw events from the device) or modified events. I think a flag might be able to solve that. The flag will control whether we add the new program at the beginning of the list or at the tail, but I am not sure if this is common practice in eBPF or if there is a better way. Cheers, Benjamin [0] https://lore.kernel.org/linux-input/20211215134220.1735144-1-tero.kristo@linux.intel.com/ [1] https://lore.kernel.org/linux-input/20220203143226.4023622-1-benjamin.tissoires@redhat.com/ Benjamin Tissoires (6): HID: initial BPF implementation HID: bpf: allow to change the report descriptor from an eBPF program HID: bpf: add hid_{get|set}_data helpers HID: bpf: add new BPF type to trigger commands from userspace HID: bpf: tests: rely on uhid event to know if a test device is ready HID: bpf: add bpf_hid_raw_request helper function drivers/hid/Makefile | 1 + drivers/hid/hid-bpf.c | 327 +++++++++ drivers/hid/hid-core.c | 31 +- include/linux/bpf-hid.h | 98 +++ include/linux/bpf_types.h | 4 + include/linux/hid.h | 25 + include/uapi/linux/bpf.h | 33 + include/uapi/linux/bpf_hid.h | 56 ++ kernel/bpf/Makefile | 3 + kernel/bpf/hid.c | 653 ++++++++++++++++++ kernel/bpf/syscall.c | 12 + samples/bpf/.gitignore | 1 + samples/bpf/Makefile | 4 + samples/bpf/hid_mouse_kern.c | 91 +++ samples/bpf/hid_mouse_user.c | 129 ++++ tools/include/uapi/linux/bpf.h | 33 + tools/lib/bpf/libbpf.c | 9 + tools/lib/bpf/libbpf.h | 2 + tools/lib/bpf/libbpf.map | 1 + tools/testing/selftests/bpf/prog_tests/hid.c | 685 +++++++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 149 ++++ 21 files changed, 2339 insertions(+), 8 deletions(-) create mode 100644 drivers/hid/hid-bpf.c create mode 100644 include/linux/bpf-hid.h create mode 100644 include/uapi/linux/bpf_hid.h create mode 100644 kernel/bpf/hid.c create mode 100644 samples/bpf/hid_mouse_kern.c create mode 100644 samples/bpf/hid_mouse_user.c create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c create mode 100644 tools/testing/selftests/bpf/progs/hid.c