On Mon, Feb 13, 2023 at 2:17 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Feb 13, 2023 at 12:49 PM Nick Desaulniers
> <ndesaulniers@xxxxxxxxxx> wrote:
> >
> > 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.
>
> It's a transition from kernel to bpf prog.
> If CFI trips on it it will trip on all transitions.
> All calls from kernel into bpf are more or less the same.
> Not sure what it means for other archs, but on x86 JIT emits 'endbr'
> insn to make IBT/CFI happy.
Having said the above it's good that the warning was there.
Just type casting the func is not correct.
We should call bpf progs like this:
-void bpf_rbtree_add(struct bpf_rb_root *root, struct bpf_rb_node *node,
- bool (less)(struct bpf_rb_node *a, const struct
bpf_rb_node *b))
-{
- rb_add_cached((struct rb_node *)node, (struct rb_root_cached *)root,
- (bool (*)(struct rb_node *, const struct rb_node *))less);
+void bpf_rbtree_add(struct bpf_rb_root *root, struct bpf_rb_node
*node, bpf_callback_t less)
+{
+ struct rb_node **link = &((struct rb_root_cached
*)root)->rb_root.rb_node;
+ struct rb_node *parent = NULL;
+ bool leftmost = true;
+
+ while (*link) {
+ parent = *link;
+ if (less((uintptr_t)node, (uintptr_t)parent, 0, 0, 0)) {
+ link = &parent->rb_left;
+ } else {
+ link = &parent->rb_right;
+ leftmost = false;
+ }
+ }
+
+ rb_link_node((struct rb_node *)node, parent, link);
+ rb_insert_color_cached((struct rb_node *)node, (struct
rb_root_cached *)root, leftmost);
Dave, please incorporate in the next respin.
I only compile tested the above.
