On Mon, Feb 13, 2023 at 12:45 PM Dave Marchevsky <davemarchevsky@xxxxxxxx> wrote: > > On 2/12/23 6:21 AM, kernel test robot wrote: > > Hi Dave, > > > >>> kernel/bpf/helpers.c:1901:9: warning: cast from 'bool (*)(struct bpf_rb_node *, const struct bpf_rb_node *)' (aka '_Bool (*)(struct bpf_rb_node *, const struct bpf_rb_node *)') to 'bool (*)(struct rb_node *, const struct rb_node *)' (aka '_Bool (*)(struct rb_node *, const struct rb_node *)') converts to incompatible function type [-Wcast-function-type-strict] > > (bool (*)(struct rb_node *, const struct rb_node *))less); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This is the only new warning introduced by this series. A previous version had > the same complaint by kernel test robot. > > struct bpf_rb_node is an opaque struct with the same size as struct rb_node. > It's not intended to be manipulated directly by any BPF program or bpf-rbtree > kernel code, but rather to be used as a struct rb_node by rbtree library > helpers. > > Here, the compiler complains that the less() callback taken by bpf_rbtree_add > is typed > > bool (*)(struct bpf_rb_node *, const struct bpf_rb_node *) > > while the actual rbtree lib helper rb_add's less() is typed > > bool (*)(struct rb_node *, const struct rb_node *) > > I'm not a C standard expert, but based on my googling, for C99 it's not valid > to cast a function pointer to anything aside from void* and its original type. > Furthermore, since struct bpf_rb_node an opaque bitfield and struct rb_node > has actual members, C99 standard 6.2.7 paragraph 1 states that they're not > compatible: > > Moreover, two structure, > union, or enumerated types declared in separate translation units are compatible if their > tags and members satisfy the following requirements: If one is declared with a tag, the > other shall be declared with the same tag. If both are complete types, then the following > additional requirements apply: there shall be a one-to-one correspondence between their > members such that each pair of corresponding members are declared with compatible > types, and such that if one member of a corresponding pair is declared with a name, the > other member is declared with the same name. For two structures, corresponding > members shall be declared in the same order > > I'm not sure how to proceed here. We could change bpf_rbtree_add's less() cb to I haven't looked at the series in question, but note that this compile time warning is meant to help us catch Control Flow Integrity runtime violations, which may result in a panic. > take two rb_node *'s, but then each such cb implementation would have to cast > its parameters before doing anything useful with them. Furthermore this would > require introducing non-opaque rb_node type into BPF programs. Other ideas seem > similarly hacky. > > Note that this flag was only recently introduced [0] and discussion in that > linked thread notes that ~1500 other places in the kernel raise same warning. > > > [0]: https://reviews.llvm.org/D134831 > > > > > 7 warnings generated. > > > > > > vim +1901 kernel/bpf/helpers.c > > > > 1896 > > 1897 void bpf_rbtree_add(struct bpf_rb_root *root, struct bpf_rb_node *node, > > 1898 bool (less)(struct bpf_rb_node *a, const struct bpf_rb_node *b)) > > 1899 { > > 1900 rb_add_cached((struct rb_node *)node, (struct rb_root_cached *)root, > >> 1901 (bool (*)(struct rb_node *, const struct rb_node *))less); > > 1902 } > > 1903 > > > -- Thanks, ~Nick Desaulniers
