Hi Benjamin, On 4/25/23 07:52, Benjamin Tissoires wrote: > On Mon, Apr 24, 2023 at 3:07 PM Benjamin Tissoires > <benjamin.tissoires@xxxxxxxxxx> wrote: >> >> Hi Vicki, >> >> On Fri, Apr 21, 2023 at 4:36 AM Vicki Pfau <vi@xxxxxxxxxxx> wrote: >>> >>> Hello, >>> >>> Following up on a conversation from last year (cf. https://lore.kernel.org/linux-input/CAO-hwJLfY+D0NyCUCncrjcXETKwOBqj1CuHtB_mtGhYfKV0Bww@xxxxxxxxxxxxxx/) about how to approach differing opinions about how drivers should work between userspace and kernel, we're looking for a way to effectively stop the kernel from doing anything "smart" with a a HID device, e.g. a hid-sony device, when the associated hidraw is opened. At the moment, we have a specialized daemon that will find the mouse evdev associated with the controller when it's created and inhibit it, but this is anything but ideal and only handles the mouse itself, not the kernel logic in general. You can also see the hackish way this is implemented in the hid-steam driver, which uses an intermediary hid ll_driver to intercept the open and close commands and stop passing data if the hidraw gets opened. I consider this implementation to be very much a hack and an anti-pattern, and I think other HID implementers would agree, but I'm not sure there's a cleaner way to detect this in the kernel at the moment. >>> >>> I see a bit of a difference of opinion as to what should be happening here between developers on our end and the kernel end. Our position is "we have a userspace driver that does everything we want, we'd like the kernel to stop trying to be smart when our driver is active", and I expect the kernel developer opinion is "why should you have a userspace driver at all when our kernel driver is fine, and if it's not you can upstream patches?", so there's probably some need to find a middle ground if we want things to work well for users in the end. >> >> The main problem we have here, is that if userspace is actively using >> hidraw to configure the device, the kernel is not aware, and thus its >> state will be wrong. So my initial reaction is "we need to have ad-hoc >> and opt-in functionality for these, because the states the kernel >> needs to monitor will differ from one device to one other". But this >> opt-in could (should) be controlled from userspace through eBPF. >> >> OTOH, maybe the simplest solution would be to entirely unbind the >> device from the kernel driver when steam opens and rebinds it when it >> closes. This way the kernel will reinitialize the device itself >> properly and keep its state coherent. We can think of multiple >> solutions for disabling the driver and still keeping hidraw open, and >> guess what, eBPF is one of them :) >> >> Note though that disabling the kernel module might require some >> changes in the kernel driver itself, so maybe that's not the best path >> too. (eBPF can not change any quirks on a given device, but maybe that >> is something we can think of). >> >>> >>> We'd previously discussed an ioctl for the hidraw, or perhaps an eBPF approach, though I think at the time of that discussion, eBPF wasn't mature for the HID subsystem. I don't know the current state of that, or if it's possible to do this with just write access to the hidraw device (the ideal case for how we want to handle this--root is pretty much out of the question for an "ideal case"). Since we still don't have a good answer for this, as far as I'm aware, I'd like to try to reach an approach that's amicable for both sides. >> >> Good timing. Kernel 6.3 was out just yesterday, and HID-BPF is in :) >> (well, not all use cases I envisioned are implemented, but at least >> the bulk is there). >> >>> >>> I know that the ioctl approach meant having to introduce logic for programs in userspace, but in this specific case, and possibly others, that's actually what we're looking for. Further, the reason root is out is because Steam isn't the OS (in most cases), and we want this to only happen when Steam is running. Having to run a daemon as root underneath Steam would require something like a setuid binary or custom sudo/polkit rules, which of course need root to set up anyway. So while eBPF may fit some use cases, e.g. full control over the system via DE or systemd, etc, it's not what we're looking for here. >> >> I think you are dismissing eBPF too quickly :) >> >> Basically, eBPF doesn't require you to have a daemon loaded: you can >> load the BPF program, attach it to the device, pin it in the kernel >> and close your eBPF loader. So all you need is a new udev rule that >> calls for a program to load the eBPF program just at plugging time. >> Eventually, for such "no-daemon" programs, the kernel itself will >> load those programs, and userspace can just override them if they are >> not up to date. >> >> Which means that when you load the eBPF program, you just "patched" >> the kernel with your current API. >> >> [Side note: my push for eBPF is because this is exactly the kind of >> use case I envisioned for it: an app (or set of apps) wants to tweak >> the kernel behavior only when it is started, and wants to be in >> control. If this app wants to change some behaviour, fixing the eBPF >> program is way simpler and can be done in sync with the app update >> rather than updating the API introduced in the kernel] >> >> I think the following (in pseudo-ish-code) would work for > [...] > > I got a working WIP at > https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/tree/hidraw-close > > I need a couple of kernel changes to allow for the introspection of > the hidraw device from the struct hid_device and to let > hid-playstation go with the events. > > We probably still want the opt-in capability, so we'd also need the > hid_hw_request() BPF hook set up, but this should give you an idea > that BPF could be a simple and elegant solution. > > Cheers, > Benjamin > Thanks for preparing this proof of concept. Does it require kernel 6.3 to work? For the time being I'm trying to get something going on 6.1, and am relying on a user space daemon to manage the `inhibited` node directly. I was hoping to switch over to a more crash-resistant method of doing this, but if it requires 6.2 or 6.3, I will probably have to stick with this for the time being. I'll switch over to an approach like this when possible, but I'm not sure when that will be. Thanks, Vicki
