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 hid-playstation with the current 6.3 kernel: ``` /* * variable that is set to true when steam is opening the device * we might as well use a map instead */ static bool steam_opened = false; /* * HID unique ID used by the device we are monitoring (set during attach) */ static u16 hid_steam_device_id; /* * take the current report descriptor, and extend it by adding more opaque report IDs * for example, if the mouse is reporting on report ID 0x02, we set a new hid collection * with opaque data (const) as report ID 0xf2 */ SEC("fmod_ret/hid_bpf_rdesc_fixup") int BPF_PROG(hid_fix_rdesc, struct hid_bpf_ctx *hctx) { __u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 4096 /* size */); if (!data) return 0; /* EPERM check */ /* append a new report descriptor to the current one */ return old_size + appended; } SEC("fmod_ret/hid_bpf_device_event") int BPF_PROG(disable_when_opened, struct hid_bpf_ctx *hid_ctx) { __u8 *data = hid_bpf_get_data(hid_ctx, 0, 1); if (!data) return 0; /* EPERM check */ if (steam_opened) data[0] += 0xf0; return 0; } /* * completely untested */ int hid_id_from_file(struct inode *inode, struct file *file) { struct hidraw_list *list; struct hidraw *dev; if (!file->private_data) return -EINVAL; list = file->private_data; if (!list->hidraw) return -EINVAL; dev = list->hidraw; return dev->hid->id; } SEC("fexit/hidraw_open") int BPF_PROG(fexit_hidraw_open, struct inode *inode, struct file *file, int ret) { /* exit if any error happened */ if (ret) return 0; if (hid_id_from_file(inode, file) == hid_steam_device_id) steam_opened = true; return 0; } SEC("fenty/hidraw_release") int BPF_PROG(fentry_hidraw_release, struct inode *inode, struct file *file) { struct hidraw_list *dev; if (file->private_data) dev = file->private_data; if (hid_id_from_file(inode, file) == hid_steam_device_id) steam_opened = false; return 0; } ``` Minus the completely untested bit to go from the inode to the HID id (which I already did once or twice IIRC), this basically diverts any incoming report from 0x0N to 0xfN. And then in Steam, or any other client, you can either revert that change or just handle it as if this was a normal report from the device. The bits to reset the kernel driver would consist in detaching the hid report fixup and re-attaching it, which would trigger 2 reprobes of the device from the kernel. This might require some userspace control because we need a syscall bpf type to do so. But OTOH, this might not be needed for some devices, given that you can pretty much design and tailor one BPF program per device type (so you can probably handle the "client disconnected abruptly" case from within the eBPF program itself). You could also handle the "steam tells eBPF to offset the report ID" if we add one other eBPF hook in hid_hw_raw_request(). FWIW, that hook would allow for having a HID firewall to prevent unwanted accesses to the device. Basically steam could call a non-declared feature on the device with a given secret knock, and when the eBPF program sees it, it turns the no-report mode on. (To handle the abruptly disconnected client, you can check for the last known knock, and if it's more than 2 min for instance, reset to the "normal" mode without report offset). Compared to a new hidraw ioctl you get: - immediate support on the existing 6.3 kernel - you can change this in the future without having to wait for a new kernel (and then supporting several kernels with different behaviors) - you can target only tested devices (because some might need a different offset) - you can revise any part without bothering to change the kernel - you will fill the dmesg with a bunch of "Unhandled reportID=0xf2" with the current hid-playstation driver, so this needs to be fixed there... - [for the hid-playstation case, you might just change the report size by adding 1 and setting that last extra value to 0 -> hid-playstation will reject the report if it is not exactly the expected size, but hidraw will forward it properly] > > Does anyone have opinions on how to proceed from here? > Basically, I have the strong belief that any kernel API you can imagine can be implemented in eBPF first, and once this stabilized we could think of making it an official one. But this will have the cost of flexibility, so why really bother stabilizing it? Cheers, Benjamin
