| Message ID | 20220421140740.459558-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices (new attempt) | expand |
On Thu, Apr 21, 2022 at 04:07:33PM +0200, Benjamin Tissoires wrote: > Hi, > > so after the reviews from v3, and some discussion with Alexei, I am > back with a new version of HID-BPF. > > This version is not complete (thus the RFC), but I'd like to share > it now to get initial feedback, in case I am too far from the actual > goal. > > FTR, the goal is to provide some changes in the core verifier/btf so > that we can plug in HID-BPF independently from BPF core. This way we can > extend it without having to care about bpf-next. Overall looks great. imo much cleaner, simpler and more extensible than the earlier versions. The bpf core extensions are nicely contained and HID side can be worked on in parallel. > The things I am not entirely sure are: > - do we need only fentry/fexit/fmod_ret BPF program types or should > programs that modify the data stream use a different kind? Probably not. I'll reply in patch 2. > - patch 3/7 is probably not the correct approach (see comments in the > patch itself) > > We are missing quite a few bits here: > - selftests for patches 1 to 4 > - add the ability to attach a program to a struct device, and run that > program only for that struct device yes. That is still to be figured out. > - when running through bpf_prog_test_run_opts, how can we ensure we are > talking to the correct device? (I have a feeling this is linked to the > previous point) > - how can we reconnect the device when a report descriptor fixup BPF > program is loaded (would it make sense to allow some notifications on > when a BPF program is attached/detached to a device, and which > function have been traced?) Not sure I follow. What kind of notification do you have in mind? To user space?
On Tue, Apr 26, 2022 at 6:03 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 21, 2022 at 04:07:33PM +0200, Benjamin Tissoires wrote: > > Hi, > > > > so after the reviews from v3, and some discussion with Alexei, I am > > back with a new version of HID-BPF. > > > > This version is not complete (thus the RFC), but I'd like to share > > it now to get initial feedback, in case I am too far from the actual > > goal. > > > > FTR, the goal is to provide some changes in the core verifier/btf so > > that we can plug in HID-BPF independently from BPF core. This way we can > > extend it without having to care about bpf-next. > > Overall looks great. imo much cleaner, simpler and more extensible > than the earlier versions. > The bpf core extensions are nicely contained and HID side can be > worked on in parallel. \o/ > > > The things I am not entirely sure are: > > - do we need only fentry/fexit/fmod_ret BPF program types or should > > programs that modify the data stream use a different kind? > > Probably not. I'll reply in patch 2. > > > - patch 3/7 is probably not the correct approach (see comments in the > > patch itself) > > > > We are missing quite a few bits here: > > - selftests for patches 1 to 4 > > - add the ability to attach a program to a struct device, and run that > > program only for that struct device > > yes. That is still to be figured out. I spent some time on that, and I don't think it makes a lot of sense to use the current trampoline approach if we want to keep on using fentry/fexit... - the trampoline is pretty nice, but it adds instructions before calling the actual function, meaning that adding a check on struct device will be quite hard to do ()we have no idea where the struct device is in the arguments) and will take more space on the trampoline itself - there is a limit on how many functions can be attached to a trampoline (38 IIRC), and we probably will explode that number quickly enough when we get more BPF programs to support HID devices. So my chain of thoughts from yesterday was the following (completely untested of course): - instead of writing a new BPF API that might move in the future while things are settling, I can actually simply load a tracer BPF program from HID that monitors the BPF programs that are attached to a given function - I can also add a new API (a kfunc likely) that "registers" a given BPF program (through its fd) to a given HID device - when a device sends data, it hits hid_bpf_device_event() which will have a default BPF program (loaded by the kernel) that dispatches the registered BPF programs based on the HID device. This would solve the 2 issues above IMO, except that the kfunc to register a HID BPF program will suddenly be not standard. > > > - when running through bpf_prog_test_run_opts, how can we ensure we are > > talking to the correct device? (I have a feeling this is linked to the > > previous point) > > - how can we reconnect the device when a report descriptor fixup BPF > > program is loaded (would it make sense to allow some notifications on > > when a BPF program is attached/detached to a device, and which > > function have been traced?) > > Not sure I follow. What kind of notification do you have in mind? > To user space? > No, this is in-kernel notifications. What I want to do, is when I load a BPF program that changes the HID report descriptor, hid-core detects that and reconnects the attached device. But after a couple of days of thinking, and with the above approach where HID would preload a BPF program, I should be able to achieve that with the "register BPF through a HID kfunc call". When I see that we are attaching a HID report descriptor fixup to a given HID device, I can then reconnect the matching device. It would certainly be cleaner to have a general "notify me when a tracer is attached to this particular function", but we can hide that right now with a preloaded BPF program :) Cheers, Benjamin
On Tue, Apr 26, 2022 at 12:20 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Apr 26, 2022 at 6:03 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Apr 21, 2022 at 04:07:33PM +0200, Benjamin Tissoires wrote: > > > Hi, > > > > > > so after the reviews from v3, and some discussion with Alexei, I am > > > back with a new version of HID-BPF. > > > > > > This version is not complete (thus the RFC), but I'd like to share > > > it now to get initial feedback, in case I am too far from the actual > > > goal. > > > > > > FTR, the goal is to provide some changes in the core verifier/btf so > > > that we can plug in HID-BPF independently from BPF core. This way we can > > > extend it without having to care about bpf-next. > > > > Overall looks great. imo much cleaner, simpler and more extensible > > than the earlier versions. > > The bpf core extensions are nicely contained and HID side can be > > worked on in parallel. > > \o/ > > > > > > The things I am not entirely sure are: > > > - do we need only fentry/fexit/fmod_ret BPF program types or should > > > programs that modify the data stream use a different kind? > > > > Probably not. I'll reply in patch 2. > > > > > - patch 3/7 is probably not the correct approach (see comments in the > > > patch itself) > > > > > > We are missing quite a few bits here: > > > - selftests for patches 1 to 4 > > > - add the ability to attach a program to a struct device, and run that > > > program only for that struct device > > > > yes. That is still to be figured out. > > I spent some time on that, and I don't think it makes a lot of sense > to use the current trampoline approach if we want to keep on using > fentry/fexit... > - the trampoline is pretty nice, but it adds instructions before > calling the actual function, meaning that adding a check on struct > device will be quite hard to do ()we have no idea where the struct > device is in the arguments) and will take more space on the trampoline > itself > - there is a limit on how many functions can be attached to a > trampoline (38 IIRC), and we probably will explode that number quickly > enough when we get more BPF programs to support HID devices. Ohh. This is an obsolete limitation. 38 was the number since we used half page optimization for bpf trampoline. It's gone now. We can easily lift this max. > So my chain of thoughts from yesterday was the following (completely > untested of course): > - instead of writing a new BPF API that might move in the future while > things are settling, I can actually simply load a tracer BPF program > from HID that monitors the BPF programs that are attached to a given > function > - I can also add a new API (a kfunc likely) that "registers" a given > BPF program (through its fd) to a given HID device > - when a device sends data, it hits hid_bpf_device_event() which will > have a default BPF program (loaded by the kernel) that dispatches the > registered BPF programs based on the HID device. > > This would solve the 2 issues above IMO, except that the kfunc to > register a HID BPF program will suddenly be not standard. Could you add more details to these ideas? I thought you wanted bpf prog writers to be independent of each other. They would tell some framework HID device id/pcie id that they need and the rest would be automatic. Maybe we can achieve that by adding another layer before libbpf that would accept (bpf_prog, hid_id) tuple and insert if (hid->id != hid_id) return -E..; as the first insn into bpf_prog before loading into the kernel. All such progs will be kfunc-s attached to the same hook. The kernel will execute them sequentially. The framework will provide demux by auto-inserting this 'if'. This 'if (hid)' could be a part of sample code too. We can simply ask prog writers to follow this style. Another idea would be to do something like libxdp. Attach a "dispatcher" bpf prog to the kfunc hook and use some library to attach hid-specific progs as "freplace" kind of programs. It's a more involved solution. Another option is to use tail_calls. If hid_id is a relatively small number. The "dispatcher" bpf prog can do bpf_tail_call(prog_array, hid_id) while hid specific progs insert itself into prog_array instead of attaching to kfunc. > > > > > - when running through bpf_prog_test_run_opts, how can we ensure we are > > > talking to the correct device? (I have a feeling this is linked to the > > > previous point) > > > - how can we reconnect the device when a report descriptor fixup BPF > > > program is loaded (would it make sense to allow some notifications on > > > when a BPF program is attached/detached to a device, and which > > > function have been traced?) > > > > Not sure I follow. What kind of notification do you have in mind? > > To user space? > > > > No, this is in-kernel notifications. > What I want to do, is when I load a BPF program that changes the HID > report descriptor, hid-core detects that and reconnects the attached > device. > > But after a couple of days of thinking, and with the above approach > where HID would preload a BPF program, I should be able to achieve > that with the "register BPF through a HID kfunc call". When I see that > we are attaching a HID report descriptor fixup to a given HID device, > I can then reconnect the matching device. > > It would certainly be cleaner to have a general "notify me when a > tracer is attached to this particular function", but we can hide that > right now with a preloaded BPF program :) There are few lsm hooks in bpf core. It probably wwill be eird for hid core to hook into them. We can add a few tracepoints at attach functions if that helps.
On Sat, Apr 30, 2022 at 5:01 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Apr 26, 2022 at 12:20 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Tue, Apr 26, 2022 at 6:03 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Apr 21, 2022 at 04:07:33PM +0200, Benjamin Tissoires wrote: > > > > Hi, > > > > > > > > so after the reviews from v3, and some discussion with Alexei, I am > > > > back with a new version of HID-BPF. > > > > > > > > This version is not complete (thus the RFC), but I'd like to share > > > > it now to get initial feedback, in case I am too far from the actual > > > > goal. > > > > > > > > FTR, the goal is to provide some changes in the core verifier/btf so > > > > that we can plug in HID-BPF independently from BPF core. This way we can > > > > extend it without having to care about bpf-next. > > > > > > Overall looks great. imo much cleaner, simpler and more extensible > > > than the earlier versions. > > > The bpf core extensions are nicely contained and HID side can be > > > worked on in parallel. > > > > \o/ > > > > > > > > > The things I am not entirely sure are: > > > > - do we need only fentry/fexit/fmod_ret BPF program types or should > > > > programs that modify the data stream use a different kind? > > > > > > Probably not. I'll reply in patch 2. > > > > > > > - patch 3/7 is probably not the correct approach (see comments in the > > > > patch itself) > > > > > > > > We are missing quite a few bits here: > > > > - selftests for patches 1 to 4 > > > > - add the ability to attach a program to a struct device, and run that > > > > program only for that struct device > > > > > > yes. That is still to be figured out. > > > > I spent some time on that, and I don't think it makes a lot of sense > > to use the current trampoline approach if we want to keep on using > > fentry/fexit... > > - the trampoline is pretty nice, but it adds instructions before > > calling the actual function, meaning that adding a check on struct > > device will be quite hard to do ()we have no idea where the struct > > device is in the arguments) and will take more space on the trampoline > > itself > > - there is a limit on how many functions can be attached to a > > trampoline (38 IIRC), and we probably will explode that number quickly > > enough when we get more BPF programs to support HID devices. > > Ohh. This is an obsolete limitation. > 38 was the number since we used half page optimization > for bpf trampoline. > It's gone now. We can easily lift this max. Oh, interesting. I have been working this week with that limitation in place, and I just managed to get something working yesterday night. See below. > > > So my chain of thoughts from yesterday was the following (completely > > untested of course): > > - instead of writing a new BPF API that might move in the future while > > things are settling, I can actually simply load a tracer BPF program > > from HID that monitors the BPF programs that are attached to a given > > function > > - I can also add a new API (a kfunc likely) that "registers" a given > > BPF program (through its fd) to a given HID device > > - when a device sends data, it hits hid_bpf_device_event() which will > > have a default BPF program (loaded by the kernel) that dispatches the > > registered BPF programs based on the HID device. > > > > This would solve the 2 issues above IMO, except that the kfunc to > > register a HID BPF program will suddenly be not standard. > > Could you add more details to these ideas? That's basically your last suggestion below. > I thought you wanted bpf prog writers to be independent of each other. > They would tell some framework HID device id/pcie id that they need > and the rest would be automatic. > Maybe we can achieve that by adding another layer before libbpf > that would accept (bpf_prog, hid_id) tuple and insert > if (hid->id != hid_id) return -E..; > as the first insn into bpf_prog before loading into the kernel. > All such progs will be kfunc-s attached to the same hook. > The kernel will execute them sequentially. > The framework will provide demux by auto-inserting this 'if'. That's an interesting approach. Much less convoluted than the one I have right now. The only downside I see is that every program will be called for every event, which might not be ideal (though the event stream from HID is way smaller than network). > This 'if (hid)' could be a part of sample code too. > We can simply ask prog writers to follow this style. Not a big fan of that... > > Another idea would be to do something like libxdp. > Attach a "dispatcher" bpf prog to the kfunc hook and use > some library to attach hid-specific progs as "freplace" kind of > programs. It's a more involved solution. freplace might not work for me because I want to attach more than one program to a given HID device. Not so sure BPF_PROG_TYPE_REPLACE will do something enough there. > > Another option is to use tail_calls. > If hid_id is a relatively small number. The "dispatcher" bpf prog > can do bpf_tail_call(prog_array, hid_id) > while hid specific progs insert itself into prog_array > instead of attaching to kfunc. This is roughly what I have now: - hid-core is not aware of BPF except for a few __weak ALLOW_ERROR_INJECTION hooks (dispatch_hid_bpf_device_event for example) - I have a separate hid-bpf module that attaches BPF traces to these hooks and calls a "dispatch" kfunc within hid-bpf - the dispatch function then do a succession of BPF calls to programs attached to it by using bpf_tail_call(prog_array, hid_id) - for the clients, they define one or more SEC("fmod_ret/hid_bpf_device_event"). That __weak hook is declared in the kernel by hid-bpf but is never called, it's just an API declaration - then clients call in a SEC("syscall") hid_bpf_attach_prog(ctx->prog_fd, ctx->hid_id, ctx->flags); - hid_bpf_attach_prog is a kfunc that takes a ref on the struct bpf_prog*, and stores that program in the correct struct bpf_map *for the given attached_btf_id (hid_bpf_device_event in our case) And that's about it. I still need to handle automatic release of the bpf prog when there is no userspace open fd on it unless it's pinned but I think this should be working fine. I also probably need to pin some SEC("syscall") (hid_bpf_attach_prog and hid_bpf_dettach_prog) so users don't have to write them down and can just use the ones provided by the kernel. The nice thing is that I can define my own API for the attach call without dealing with bpf core. I can thus add a priority flag that is relevant here because the data coming through the bpf program can be modified. The other thing is that now, I don't care which function we are in to decide if a RET_PTR_MEM is read only or not. I can deal with that by either playing with the flags or even replacing entirely the dispatch trace prog from userspace if I want to access the raw events. However, the downsides are: - I need to also define kfuncs for BPF_PROG_TYPE_SYSCALL (I don't think It'll be a big issue) - The only way I could store the bpf_prog into the map was to hack around the map ops, because the fd of the map in the skel is not available while doing a SEC("syscall") from a different process. Also, I wonder if we should not have some way to namespace kfuncs. Ideally, I would like to prevent the usage of those kfuncs outside of some helpers that I define in HID so I don't have to worry too much about other trace programs fuzzing and segfaulting the kernel. > > > > > > > > - when running through bpf_prog_test_run_opts, how can we ensure we are > > > > talking to the correct device? (I have a feeling this is linked to the > > > > previous point) > > > > - how can we reconnect the device when a report descriptor fixup BPF > > > > program is loaded (would it make sense to allow some notifications on > > > > when a BPF program is attached/detached to a device, and which > > > > function have been traced?) > > > > > > Not sure I follow. What kind of notification do you have in mind? > > > To user space? > > > > > > > No, this is in-kernel notifications. > > What I want to do, is when I load a BPF program that changes the HID > > report descriptor, hid-core detects that and reconnects the attached > > device. > > > > But after a couple of days of thinking, and with the above approach > > where HID would preload a BPF program, I should be able to achieve > > that with the "register BPF through a HID kfunc call". When I see that > > we are attaching a HID report descriptor fixup to a given HID device, > > I can then reconnect the matching device. > > > > It would certainly be cleaner to have a general "notify me when a > > tracer is attached to this particular function", but we can hide that > > right now with a preloaded BPF program :) > > There are few lsm hooks in bpf core. It probably wwill be eird > for hid core to hook into them. We can add a few tracepoints > at attach functions if that helps. > With the above, I actually don't need those notifications. Users are calling my own "syscall" when attaching their BPF prog, and they are also not required to attach the program to the actual BPF trace (through BPF_PROG_ATTACH), so this request is now gone :) Cheers, Benjamin
On Sat, Apr 30, 2022 at 9:12 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > [...] > This is roughly what I have now: > > - hid-core is not aware of BPF except for a few __weak > ALLOW_ERROR_INJECTION hooks (dispatch_hid_bpf_device_event for > example) > - I have a separate hid-bpf module that attaches BPF traces to these > hooks and calls a "dispatch" kfunc within hid-bpf > - the dispatch function then do a succession of BPF calls to programs > attached to it by using bpf_tail_call(prog_array, hid_id) > > - for the clients, they define one or more > SEC("fmod_ret/hid_bpf_device_event"). That __weak hook is declared in > the kernel by hid-bpf but is never called, it's just an API > declaration > - then clients call in a SEC("syscall") > hid_bpf_attach_prog(ctx->prog_fd, ctx->hid_id, ctx->flags); > - hid_bpf_attach_prog is a kfunc that takes a ref on the struct > bpf_prog*, and stores that program in the correct struct bpf_map *for > the given attached_btf_id (hid_bpf_device_event in our case) > > And that's about it. > I still need to handle automatic release of the bpf prog when there is > no userspace open fd on it unless it's pinned but I think this should > be working fine. > > I also probably need to pin some SEC("syscall") (hid_bpf_attach_prog > and hid_bpf_dettach_prog) so users don't have to write them down and > can just use the ones provided by the kernel. > > The nice thing is that I can define my own API for the attach call > without dealing with bpf core. I can thus add a priority flag that is > relevant here because the data coming through the bpf program can be > modified. > > The other thing is that now, I don't care which function we are in to > decide if a RET_PTR_MEM is read only or not. I can deal with that by > either playing with the flags or even replacing entirely the dispatch > trace prog from userspace if I want to access the raw events. > > However, the downsides are: > - I need to also define kfuncs for BPF_PROG_TYPE_SYSCALL (I don't > think It'll be a big issue) > - The only way I could store the bpf_prog into the map was to hack > around the map ops, because the fd of the map in the skel is not > available while doing a SEC("syscall") from a different process. Update on this side: I realized that I could use the syscall BPF_MAP_GET_FD_BY_ID instead to get an fd for the current task. However, I've been bitten quite hard today because I was using bpf_map_get() instead of bpf_map_get_with_uref(), and so every time I closed the fd in the syscall the map was cleared... But now I would like to have more than one program of a type per hid device, meaning that I can not have only one bpf_map of type BPF_MAP_TYPE_PROG_ARRAY. I have explored BPF_MAP_TYPE_HASH_OF_MAPS, but we can not have BPF_MAP_TYPE_PROG_ARRAY as inner maps with the current code. And I'd need 2 levels of nesting (which is not authorized today): - hid_jmp_table (key: HID id) - array of different program type per HID device (key: HID_BPF_PROG_TYPE) - BPF_MAP_TYPE_PROG_ARRAY with the actual progs (key: int) The other solution would be to be able to create a map when needed, store it in struct hid_device, and then call bpf_tail_call on this map. The problem is that I need a way to teach the verifier that the struct bpf_map pointer I have in the context is a true bpf_map... Any input would be appreciated :) Cheers, Benjamin > > Also, I wonder if we should not have some way to namespace kfuncs. > Ideally, I would like to prevent the usage of those kfuncs outside of > some helpers that I define in HID so I don't have to worry too much > about other trace programs fuzzing and segfaulting the kernel. > [...]
On Mon, May 02, 2022 at 11:43:51PM +0200, Benjamin Tissoires wrote: > On Sat, Apr 30, 2022 at 9:12 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > [...] > > This is roughly what I have now: > > > > - hid-core is not aware of BPF except for a few __weak > > ALLOW_ERROR_INJECTION hooks (dispatch_hid_bpf_device_event for > > example) > > - I have a separate hid-bpf module that attaches BPF traces to these > > hooks and calls a "dispatch" kfunc within hid-bpf > > - the dispatch function then do a succession of BPF calls to programs > > attached to it by using bpf_tail_call(prog_array, hid_id) > > > > - for the clients, they define one or more > > SEC("fmod_ret/hid_bpf_device_event"). That __weak hook is declared in > > the kernel by hid-bpf but is never called, it's just an API > > declaration > > - then clients call in a SEC("syscall") > > hid_bpf_attach_prog(ctx->prog_fd, ctx->hid_id, ctx->flags); > > - hid_bpf_attach_prog is a kfunc that takes a ref on the struct > > bpf_prog*, and stores that program in the correct struct bpf_map *for > > the given attached_btf_id (hid_bpf_device_event in our case) > > > > And that's about it. > > I still need to handle automatic release of the bpf prog when there is > > no userspace open fd on it unless it's pinned but I think this should > > be working fine. > > > > I also probably need to pin some SEC("syscall") (hid_bpf_attach_prog > > and hid_bpf_dettach_prog) so users don't have to write them down and > > can just use the ones provided by the kernel. > > > > The nice thing is that I can define my own API for the attach call > > without dealing with bpf core. I can thus add a priority flag that is > > relevant here because the data coming through the bpf program can be > > modified. > > > > The other thing is that now, I don't care which function we are in to > > decide if a RET_PTR_MEM is read only or not. I can deal with that by > > either playing with the flags or even replacing entirely the dispatch > > trace prog from userspace if I want to access the raw events. > > > > However, the downsides are: > > - I need to also define kfuncs for BPF_PROG_TYPE_SYSCALL (I don't > > think It'll be a big issue) > > - The only way I could store the bpf_prog into the map was to hack > > around the map ops, because the fd of the map in the skel is not > > available while doing a SEC("syscall") from a different process. > > Update on this side: I realized that I could use the syscall > BPF_MAP_GET_FD_BY_ID instead to get an fd for the current task. > However, I've been bitten quite hard today because I was using > bpf_map_get() instead of bpf_map_get_with_uref(), and so every time I > closed the fd in the syscall the map was cleared... > > But now I would like to have more than one program of a type per hid > device, meaning that I can not have only one bpf_map of type > BPF_MAP_TYPE_PROG_ARRAY. > I have explored BPF_MAP_TYPE_HASH_OF_MAPS, but we can not have > BPF_MAP_TYPE_PROG_ARRAY as inner maps with the current code. And I'd > need 2 levels of nesting (which is not authorized today): > - hid_jmp_table (key: HID id) > - array of different program type per HID device (key: HID_BPF_PROG_TYPE) > - BPF_MAP_TYPE_PROG_ARRAY with the actual progs (key: int) > > The other solution would be to be able to create a map when needed, > store it in struct hid_device, and then call bpf_tail_call on this > map. The problem is that I need a way to teach the verifier that the > struct bpf_map pointer I have in the context is a true bpf_map... We have kptr feature now. So bpf progs can store pointers to specific kernel data structures inside map values. Storing 'struct bpf_map *' in a map value would be something :) Circular dependency issues to address. Maybe it's doable. Would hash based prog_array work ? Then the key can be an arbitrary combination. There is fd_htab logic. It's used for map-in-map. We can tweak it to store progs in a hash map.
On Sat, Apr 30, 2022 at 09:12:09AM +0200, Benjamin Tissoires wrote: > > Also, I wonder if we should not have some way to namespace kfuncs. > Ideally, I would like to prevent the usage of those kfuncs outside of > some helpers that I define in HID so I don't have to worry too much > about other trace programs fuzzing and segfaulting the kernel. That would be a great feature to have. Other folks expressed the same interest. Just grouping them by prog type is not flexible enough. It feels kfuncs could be scoped by (prog_type, attach_btf_id or attach_hook) pair. What are your thoughts?
On Thu, May 12, 2022 at 6:16 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, May 02, 2022 at 11:43:51PM +0200, Benjamin Tissoires wrote: > > On Sat, Apr 30, 2022 at 9:12 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > [...] > > > This is roughly what I have now: > > > > > > - hid-core is not aware of BPF except for a few __weak > > > ALLOW_ERROR_INJECTION hooks (dispatch_hid_bpf_device_event for > > > example) > > > - I have a separate hid-bpf module that attaches BPF traces to these > > > hooks and calls a "dispatch" kfunc within hid-bpf > > > - the dispatch function then do a succession of BPF calls to programs > > > attached to it by using bpf_tail_call(prog_array, hid_id) > > > > > > - for the clients, they define one or more > > > SEC("fmod_ret/hid_bpf_device_event"). That __weak hook is declared in > > > the kernel by hid-bpf but is never called, it's just an API > > > declaration > > > - then clients call in a SEC("syscall") > > > hid_bpf_attach_prog(ctx->prog_fd, ctx->hid_id, ctx->flags); > > > - hid_bpf_attach_prog is a kfunc that takes a ref on the struct > > > bpf_prog*, and stores that program in the correct struct bpf_map *for > > > the given attached_btf_id (hid_bpf_device_event in our case) > > > > > > And that's about it. > > > I still need to handle automatic release of the bpf prog when there is > > > no userspace open fd on it unless it's pinned but I think this should > > > be working fine. > > > > > > I also probably need to pin some SEC("syscall") (hid_bpf_attach_prog > > > and hid_bpf_dettach_prog) so users don't have to write them down and > > > can just use the ones provided by the kernel. > > > > > > The nice thing is that I can define my own API for the attach call > > > without dealing with bpf core. I can thus add a priority flag that is > > > relevant here because the data coming through the bpf program can be > > > modified. > > > > > > The other thing is that now, I don't care which function we are in to > > > decide if a RET_PTR_MEM is read only or not. I can deal with that by > > > either playing with the flags or even replacing entirely the dispatch > > > trace prog from userspace if I want to access the raw events. > > > > > > However, the downsides are: > > > - I need to also define kfuncs for BPF_PROG_TYPE_SYSCALL (I don't > > > think It'll be a big issue) > > > - The only way I could store the bpf_prog into the map was to hack > > > around the map ops, because the fd of the map in the skel is not > > > available while doing a SEC("syscall") from a different process. > > > > Update on this side: I realized that I could use the syscall > > BPF_MAP_GET_FD_BY_ID instead to get an fd for the current task. > > However, I've been bitten quite hard today because I was using > > bpf_map_get() instead of bpf_map_get_with_uref(), and so every time I > > closed the fd in the syscall the map was cleared... > > > > But now I would like to have more than one program of a type per hid > > device, meaning that I can not have only one bpf_map of type > > BPF_MAP_TYPE_PROG_ARRAY. > > I have explored BPF_MAP_TYPE_HASH_OF_MAPS, but we can not have > > BPF_MAP_TYPE_PROG_ARRAY as inner maps with the current code. And I'd > > need 2 levels of nesting (which is not authorized today): > > - hid_jmp_table (key: HID id) > > - array of different program type per HID device (key: HID_BPF_PROG_TYPE) > > - BPF_MAP_TYPE_PROG_ARRAY with the actual progs (key: int) > > > > The other solution would be to be able to create a map when needed, > > store it in struct hid_device, and then call bpf_tail_call on this > > map. The problem is that I need a way to teach the verifier that the > > struct bpf_map pointer I have in the context is a true bpf_map... > > We have kptr feature now. > So bpf progs can store pointers to specific kernel data structures > inside map values. > Storing 'struct bpf_map *' in a map value would be something :) > Circular dependency issues to address. Maybe it's doable. > > Would hash based prog_array work ? > Then the key can be an arbitrary combination. > There is fd_htab logic. It's used for map-in-map. > We can tweak it to store progs in a hash map. > In the end, I am just using a global prog_array map, and handling the indexes myself. It is probably not the cleaner and the most reusable, but it allows me at least to move forward. FWIW, I should be able to send v5 next week. I am almost done reimplementing everything I had in v3, and I am now fighting with hid.ko as a module (should be solved soon enough). Cheers, Benjamin
On Thu, May 12, 2022 at 6:23 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Apr 30, 2022 at 09:12:09AM +0200, Benjamin Tissoires wrote: > > > > Also, I wonder if we should not have some way to namespace kfuncs. > > Ideally, I would like to prevent the usage of those kfuncs outside of > > some helpers that I define in HID so I don't have to worry too much > > about other trace programs fuzzing and segfaulting the kernel. > > That would be a great feature to have. Other folks expressed the same interest. > Just grouping them by prog type is not flexible enough. > It feels kfuncs could be scoped by (prog_type, attach_btf_id or attach_hook) pair. > What are your thoughts? > Scoping by attach_btf_id is very appealing to me (attach_hook less TBH): I have internal functions I do not want normal users to use, and also it would also restrict who can call what in the more general case. However, I don't think I'll put that effort in v5. It is a nice to have feature IMO, but not really required ATM. Cheers, Benjamin
On Fri, May 13, 2022 at 10:02 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Thu, May 12, 2022 at 6:23 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Sat, Apr 30, 2022 at 09:12:09AM +0200, Benjamin Tissoires wrote: > > > > > > Also, I wonder if we should not have some way to namespace kfuncs. > > > Ideally, I would like to prevent the usage of those kfuncs outside of > > > some helpers that I define in HID so I don't have to worry too much > > > about other trace programs fuzzing and segfaulting the kernel. > > > > That would be a great feature to have. Other folks expressed the same interest. > > Just grouping them by prog type is not flexible enough. > > It feels kfuncs could be scoped by (prog_type, attach_btf_id or attach_hook) pair. > > What are your thoughts? > > > > Scoping by attach_btf_id is very appealing to me (attach_hook less TBH): > I have internal functions I do not want normal users to use, and also > it would also restrict who can call what in the more general case. > > However, I don't think I'll put that effort in v5. It is a nice to > have feature IMO, but not really required ATM. Great. Looking forward to it.
Hi, so after the reviews from v3, and some discussion with Alexei, I am back with a new version of HID-BPF. This version is not complete (thus the RFC), but I'd like to share it now to get initial feedback, in case I am too far from the actual goal. FTR, the goal is to provide some changes in the core verifier/btf so that we can plug in HID-BPF independently from BPF core. This way we can extend it without having to care about bpf-next. The things I am not entirely sure are: - do we need only fentry/fexit/fmod_ret BPF program types or should programs that modify the data stream use a different kind? - patch 3/7 is probably not the correct approach (see comments in the patch itself) We are missing quite a few bits here: - selftests for patches 1 to 4 - add the ability to attach a program to a struct device, and run that program only for that struct device - when running through bpf_prog_test_run_opts, how can we ensure we are talking to the correct device? (I have a feeling this is linked to the previous point) - how can we reconnect the device when a report descriptor fixup BPF program is loaded (would it make sense to allow some notifications on when a BPF program is attached/detached to a device, and which function have been traced?) Cheers, Benjamin Benjamin Tissoires (7): bpf/btf: also allow kfunc in tracing programs bpf/verifier: allow kfunc to return an allocated mem error-inject: add new type that carries if the function is non sleepable btf: Add a new kfunc set which allows to mark a function to be sleepable HID: initial BPF new way implementation samples/bpf: add new hid_mouse example selftests/bpf: add tests for the HID-bpf initial implementation drivers/hid/hid-core.c | 115 +++++ include/asm-generic/error-injection.h | 1 + include/linux/btf.h | 8 + include/linux/hid_bpf.h | 29 ++ kernel/bpf/btf.c | 50 +- kernel/bpf/verifier.c | 76 ++- lib/error-inject.c | 2 + samples/bpf/.gitignore | 1 + samples/bpf/Makefile | 23 + samples/bpf/hid_mouse.bpf.c | 59 +++ samples/bpf/hid_mouse.c | 131 +++++ tools/testing/selftests/bpf/config | 3 + tools/testing/selftests/bpf/prog_tests/hid.c | 482 +++++++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 32 ++ 14 files changed, 987 insertions(+), 25 deletions(-) create mode 100644 include/linux/hid_bpf.h create mode 100644 samples/bpf/hid_mouse.bpf.c create mode 100644 samples/bpf/hid_mouse.c create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c create mode 100644 tools/testing/selftests/bpf/progs/hid.c