by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops

Hi Benjamin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 70ec81c2e2b4005465ad0d042e90b36087c36104]

url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/HID-rename-struct-hid_bpf_ops-into-hid_ops/20240528-212222
base: 70ec81c2e2b4005465ad0d042e90b36087c36104
patch link: https://lore.kernel.org/r/20240528-hid_bpf_struct_ops-v1-3-8c6663df27d8%40kernel.org
patch subject: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops
config: i386-buildonly-randconfig-002-20240531 (https://download.01.org/0day-ci/archive/20240531/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240531/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from include/linux/bpf_verifier.h:7:0,
from drivers/hid/bpf/hid_bpf_struct_ops.c:10:
drivers/hid/bpf/hid_bpf_struct_ops.c: In function 'hid_bpf_struct_ops_init':
>> include/linux/bpf.h:1844:50: warning: statement with no effect [-Wunused-value]
#define register_bpf_struct_ops(st_ops, type) ({ (void *)(st_ops); 0; })
^~~~~~~~~~~~~~~~
drivers/hid/bpf/hid_bpf_struct_ops.c:244:9: note: in expansion of macro 'register_bpf_struct_ops'
return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
^~~~~~~~~~~~~~~~~~~~~~~

vim +1844 include/linux/bpf.h

c196906d50e360d Hou Tao 2021-10-25 1834
c196906d50e360d Hou Tao 2021-10-25 1835 int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
c196906d50e360d Hou Tao 2021-10-25 1836 union bpf_attr __user *uattr);
c196906d50e360d Hou Tao 2021-10-25 1837 #endif
f6be98d19985411 Kui-Feng Lee 2024-01-19 1838 int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
f6be98d19985411 Kui-Feng Lee 2024-01-19 1839 struct btf *btf,
f6be98d19985411 Kui-Feng Lee 2024-01-19 1840 struct bpf_verifier_log *log);
1338b93346587a2 Kui-Feng Lee 2024-01-19 1841 void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map);
1611603537a4b88 Kui-Feng Lee 2024-02-08 1842 void bpf_struct_ops_desc_release(struct bpf_struct_ops_desc *st_ops_desc);
27ae7997a66174c Martin KaFai Lau 2020-01-08 1843 #else
f6be98d19985411 Kui-Feng Lee 2024-01-19 @1844 #define register_bpf_struct_ops(st_ops, type) ({ (void *)(st_ops); 0; })
85d33df357b6346 Martin KaFai Lau 2020-01-08 1845 static inline bool bpf_try_module_get(const void *data, struct module *owner)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1846 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1847 return try_module_get(owner);
85d33df357b6346 Martin KaFai Lau 2020-01-08 1848 }
85d33df357b6346 Martin KaFai Lau 2020-01-08 1849 static inline void bpf_module_put(const void *data, struct module *owner)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1850 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1851 module_put(owner);
85d33df357b6346 Martin KaFai Lau 2020-01-08 1852 }
85d33df357b6346 Martin KaFai Lau 2020-01-08 1853 static inline int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map,
85d33df357b6346 Martin KaFai Lau 2020-01-08 1854 void *key,
85d33df357b6346 Martin KaFai Lau 2020-01-08 1855 void *value)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1856 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1857 return -EINVAL;
85d33df357b6346 Martin KaFai Lau 2020-01-08 1858 }
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1859 static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1860 {
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1861 return -EOPNOTSUPP;
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1862 }
1338b93346587a2 Kui-Feng Lee 2024-01-19 1863 static inline void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map)
1338b93346587a2 Kui-Feng Lee 2024-01-19 1864 {
1338b93346587a2 Kui-Feng Lee 2024-01-19 1865 }
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1866

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-05-31 18:53:17

by Alexei Starovoitov

[permalink] [raw]

Subject: Re: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops

On Wed, May 29, 2024 at 12:38 AM Benjamin Tissoires <bentiss@kernelorg> wrote:
>
> > Take a look at include/linux/bpf_mprog.h
> > I suspect it might be useful here too.
>
> From the quick look of it this is interesting, but for plain struct
> bpf_prog, right?
>
> So I would have to adapt this include for struct_ops?

yeah. It needs work to be adapted to work with struct-ops.
I mentioned it mainly from uapi perspective.
BPF_F_BEFORE, BPF_F_AFTER, BPF_F_REPLACE.
Would be good to keep things consistent.

> > > @@ -112,6 +113,63 @@ struct hid_ops {
> > >
> > > extern struct hid_ops *hid_ops;
> > >
> > > +/**
> > > + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> > > + * programs to a HID device
> > > + * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
> > > + * cannot be changed after
> > > + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
> > > + * Writeable only before ``load()``
> > > + */
> > > +struct hid_bpf_ops {
> > > + /* hid_id needs to stay first so we can easily change it
> > > + * from userspace.
> > > + */
> >
> > hmm.
> > patch 5 just does:
> > + skel->struct_ops.mouse_invert->hid_id = hid_id;
> >
> > I don't see a reason why it needs to be first.
>
> See the selftests (and my external loader, udev-hid-bpf):
>
> map = bpf_object__find_map_by_name(*self->skel->skeleton->obj,
> programs[i].name + 4);
> ASSERT_OK_PTR(map) TH_LOG("can not find struct_ops by name '%s'",
> programs[i].name + 4);
>
> /* hid_id is the first field of struct hid_bpf_ops */
> ops_hid_id = bpf_map__initial_value(map, NULL);
> ASSERT_OK_PTR(ops_hid_id) TH_LOG("unable to retrieve struct_ops data");
>
> *ops_hid_id = self->hid_id;
>
> Basically, I want to have to deal with .bpf.o and not the skeletons for
> the generic case. I want to be able to drop a file in /lib/firmware/hid
> and have udev-hid-bpf load it without having to recompile udev-hid-bpf.

and with additional restriction of only one struct-ops prog per elf file,
right?
Otherwise multiple hid_bpf_ops will be in the same elf section and
the same map.

> I couldn't seem to be able to access the struct_ops BTF information from
> libbpf, so I decided to have hid_id and flags first and consider this to
> be ABI while I figure out how to get to the real offsets :)

I see. I guess it's ok for now.