| Message ID | 20230821193311.3290257-8-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 312aa5bde8985dd2aef99d3e20abc0889c6f2a3e |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF Refcount followups 3: bpf_mem_free_rcu refcounted nodes | expand |
On 8/21/23 12:33 PM, Dave Marchevsky wrote: > Confirm that the following sleepable prog states fail verification: > * bpf_rcu_read_unlock before bpf_spin_unlock > * RCU CS will last at least as long as spin_lock CS I think the reason is bpf_spin_lock() does not allow any functions in spin lock region except some graph api kfunc's. > > Also confirm that correct usage passes verification, specifically: > * Explicit use of bpf_rcu_read_{lock, unlock} in sleepable test prog > * Implied RCU CS due to spin_lock CS > > None of the selftest progs actually attach to bpf_testmod's > bpf_testmod_test_read. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > .../selftests/bpf/progs/refcounted_kptr.c | 71 +++++++++++++++++++ > .../bpf/progs/refcounted_kptr_fail.c | 28 ++++++++ > 2 files changed, 99 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c > index c55652fdc63a..893a4fdb4b6e 100644 [...] > diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c > index 0b09e5c915b1..1ef07f6ee580 100644 > --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c > +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c > @@ -13,6 +13,9 @@ struct node_acquire { > struct bpf_refcount refcount; > }; > > +extern void bpf_rcu_read_lock(void) __ksym; > +extern void bpf_rcu_read_unlock(void) __ksym; > + > #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8))) > private(A) struct bpf_spin_lock glock; > private(A) struct bpf_rb_root groot __contains(node_acquire, node); > @@ -71,4 +74,29 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx) > return 0; > } > > +SEC("?fentry.s/bpf_testmod_test_read") > +__failure __msg("function calls are not allowed while holding a lock") > +int BPF_PROG(rbtree_fail_sleepable_lock_across_rcu, > + struct file *file, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) > +{ > + struct node_acquire *n; > + > + n = bpf_obj_new(typeof(*n)); > + if (!n) > + return 0; > + > + /* spin_{lock,unlock} are in different RCU CS */ > + bpf_rcu_read_lock(); > + bpf_spin_lock(&glock); > + bpf_rbtree_add(&groot, &n->node, less); > + bpf_rcu_read_unlock(); > + > + bpf_rcu_read_lock(); > + bpf_spin_unlock(&glock); > + bpf_rcu_read_unlock(); > + > + return 0; > +} > + > char _license[] SEC("license") = "GPL";
On 8/21/23 11:18 PM, Yonghong Song wrote: > > > On 8/21/23 12:33 PM, Dave Marchevsky wrote: >> Confirm that the following sleepable prog states fail verification: >> * bpf_rcu_read_unlock before bpf_spin_unlock >> * RCU CS will last at least as long as spin_lock CS > > I think the reason is bpf_spin_lock() does not allow any functions > in spin lock region except some graph api kfunc's. > Yeah, agreed, this test isn't really validating anything with current verifier logic. But, given that spin_lock CS w/ disabled preemption is an RCU CS, do you forsee wanting to allow rcu_read_unlock within spin_lock CS? I'll delete the test if you think it should go, but maybe it's worth keeping with a comment summarizing why it's an interesting example. Also, the existing comment in that test is incorrect, will fix. >> >> Also confirm that correct usage passes verification, specifically: >> * Explicit use of bpf_rcu_read_{lock, unlock} in sleepable test prog >> * Implied RCU CS due to spin_lock CS >> >> None of the selftest progs actually attach to bpf_testmod's >> bpf_testmod_test_read. >> >> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> >> --- >> .../selftests/bpf/progs/refcounted_kptr.c | 71 +++++++++++++++++++ >> .../bpf/progs/refcounted_kptr_fail.c | 28 ++++++++ >> 2 files changed, 99 insertions(+) >> >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c >> index c55652fdc63a..893a4fdb4b6e 100644 > [...] >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >> index 0b09e5c915b1..1ef07f6ee580 100644 >> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >> @@ -13,6 +13,9 @@ struct node_acquire { >> struct bpf_refcount refcount; >> }; >> +extern void bpf_rcu_read_lock(void) __ksym; >> +extern void bpf_rcu_read_unlock(void) __ksym; >> + >> #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8))) >> private(A) struct bpf_spin_lock glock; >> private(A) struct bpf_rb_root groot __contains(node_acquire, node); >> @@ -71,4 +74,29 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx) >> return 0; >> } >> +SEC("?fentry.s/bpf_testmod_test_read") >> +__failure __msg("function calls are not allowed while holding a lock") >> +int BPF_PROG(rbtree_fail_sleepable_lock_across_rcu, >> + struct file *file, struct kobject *kobj, >> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) >> +{ >> + struct node_acquire *n; >> + >> + n = bpf_obj_new(typeof(*n)); >> + if (!n) >> + return 0; >> + >> + /* spin_{lock,unlock} are in different RCU CS */ >> + bpf_rcu_read_lock(); >> + bpf_spin_lock(&glock); >> + bpf_rbtree_add(&groot, &n->node, less); >> + bpf_rcu_read_unlock(); >> + >> + bpf_rcu_read_lock(); >> + bpf_spin_unlock(&glock); >> + bpf_rcu_read_unlock(); >> + >> + return 0; >> +} >> + >> char _license[] SEC("license") = "GPL";
On 8/21/23 10:21 PM, David Marchevsky wrote: > On 8/21/23 11:18 PM, Yonghong Song wrote: >> >> >> On 8/21/23 12:33 PM, Dave Marchevsky wrote: >>> Confirm that the following sleepable prog states fail verification: >>> * bpf_rcu_read_unlock before bpf_spin_unlock >>> * RCU CS will last at least as long as spin_lock CS >> >> I think the reason is bpf_spin_lock() does not allow any functions >> in spin lock region except some graph api kfunc's. >> > > Yeah, agreed, this test isn't really validating anything with current verifier > logic. But, given that spin_lock CS w/ disabled preemption is an RCU CS, do > you forsee wanting to allow rcu_read_unlock within spin_lock CS? > > I'll delete the test if you think it should go, but maybe it's worth > keeping with a comment summarizing why it's an interesting example. Ya, it is an interesting case for interaction of rcu lock vs. spin lock. I guess you can keep it with comments, unless there are some other objections. > > Also, the existing comment in that test is incorrect, will fix. > >>> >>> Also confirm that correct usage passes verification, specifically: >>> * Explicit use of bpf_rcu_read_{lock, unlock} in sleepable test prog >>> * Implied RCU CS due to spin_lock CS >>> >>> None of the selftest progs actually attach to bpf_testmod's >>> bpf_testmod_test_read. >>> >>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> >>> --- >>> .../selftests/bpf/progs/refcounted_kptr.c | 71 +++++++++++++++++++ >>> .../bpf/progs/refcounted_kptr_fail.c | 28 ++++++++ >>> 2 files changed, 99 insertions(+) >>> >>> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c >>> index c55652fdc63a..893a4fdb4b6e 100644 >> [...] >>> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >>> index 0b09e5c915b1..1ef07f6ee580 100644 >>> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >>> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c >>> @@ -13,6 +13,9 @@ struct node_acquire { >>> struct bpf_refcount refcount; >>> }; >>> +extern void bpf_rcu_read_lock(void) __ksym; >>> +extern void bpf_rcu_read_unlock(void) __ksym; >>> + >>> #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8))) >>> private(A) struct bpf_spin_lock glock; >>> private(A) struct bpf_rb_root groot __contains(node_acquire, node); >>> @@ -71,4 +74,29 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx) >>> return 0; >>> } >>> +SEC("?fentry.s/bpf_testmod_test_read") >>> +__failure __msg("function calls are not allowed while holding a lock") >>> +int BPF_PROG(rbtree_fail_sleepable_lock_across_rcu, >>> + struct file *file, struct kobject *kobj, >>> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) >>> +{ >>> + struct node_acquire *n; >>> + >>> + n = bpf_obj_new(typeof(*n)); >>> + if (!n) >>> + return 0; >>> + >>> + /* spin_{lock,unlock} are in different RCU CS */ >>> + bpf_rcu_read_lock(); >>> + bpf_spin_lock(&glock); >>> + bpf_rbtree_add(&groot, &n->node, less); >>> + bpf_rcu_read_unlock(); >>> + >>> + bpf_rcu_read_lock(); >>> + bpf_spin_unlock(&glock); >>> + bpf_rcu_read_unlock(); >>> + >>> + return 0; >>> +} >>> + >>> char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c index c55652fdc63a..893a4fdb4b6e 100644 --- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c @@ -8,6 +8,9 @@ #include "bpf_misc.h" #include "bpf_experimental.h" +extern void bpf_rcu_read_lock(void) __ksym; +extern void bpf_rcu_read_unlock(void) __ksym; + struct node_data { long key; long list_data; @@ -497,4 +500,72 @@ long rbtree_wrong_owner_remove_fail_a2(void *ctx) return 0; } +SEC("?fentry.s/bpf_testmod_test_read") +__success +int BPF_PROG(rbtree_sleepable_rcu, + struct file *file, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) +{ + struct bpf_rb_node *rb; + struct node_data *n, *m = NULL; + + n = bpf_obj_new(typeof(*n)); + if (!n) + return 0; + + bpf_rcu_read_lock(); + bpf_spin_lock(&lock); + bpf_rbtree_add(&root, &n->r, less); + rb = bpf_rbtree_first(&root); + if (!rb) + goto err_out; + + rb = bpf_rbtree_remove(&root, rb); + if (!rb) + goto err_out; + + m = container_of(rb, struct node_data, r); + +err_out: + bpf_spin_unlock(&lock); + bpf_rcu_read_unlock(); + if (m) + bpf_obj_drop(m); + return 0; +} + +SEC("?fentry.s/bpf_testmod_test_read") +__success +int BPF_PROG(rbtree_sleepable_rcu_no_explicit_rcu_lock, + struct file *file, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) +{ + struct bpf_rb_node *rb; + struct node_data *n, *m = NULL; + + n = bpf_obj_new(typeof(*n)); + if (!n) + return 0; + + /* No explicit bpf_rcu_read_lock */ + bpf_spin_lock(&lock); + bpf_rbtree_add(&root, &n->r, less); + rb = bpf_rbtree_first(&root); + if (!rb) + goto err_out; + + rb = bpf_rbtree_remove(&root, rb); + if (!rb) + goto err_out; + + m = container_of(rb, struct node_data, r); + +err_out: + bpf_spin_unlock(&lock); + /* No explicit bpf_rcu_read_unlock */ + if (m) + bpf_obj_drop(m); + return 0; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c index 0b09e5c915b1..1ef07f6ee580 100644 --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c @@ -13,6 +13,9 @@ struct node_acquire { struct bpf_refcount refcount; }; +extern void bpf_rcu_read_lock(void) __ksym; +extern void bpf_rcu_read_unlock(void) __ksym; + #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8))) private(A) struct bpf_spin_lock glock; private(A) struct bpf_rb_root groot __contains(node_acquire, node); @@ -71,4 +74,29 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx) return 0; } +SEC("?fentry.s/bpf_testmod_test_read") +__failure __msg("function calls are not allowed while holding a lock") +int BPF_PROG(rbtree_fail_sleepable_lock_across_rcu, + struct file *file, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len) +{ + struct node_acquire *n; + + n = bpf_obj_new(typeof(*n)); + if (!n) + return 0; + + /* spin_{lock,unlock} are in different RCU CS */ + bpf_rcu_read_lock(); + bpf_spin_lock(&glock); + bpf_rbtree_add(&groot, &n->node, less); + bpf_rcu_read_unlock(); + + bpf_rcu_read_lock(); + bpf_spin_unlock(&glock); + bpf_rcu_read_unlock(); + + return 0; +} + char _license[] SEC("license") = "GPL";
Confirm that the following sleepable prog states fail verification: * bpf_rcu_read_unlock before bpf_spin_unlock * RCU CS will last at least as long as spin_lock CS Also confirm that correct usage passes verification, specifically: * Explicit use of bpf_rcu_read_{lock, unlock} in sleepable test prog * Implied RCU CS due to spin_lock CS None of the selftest progs actually attach to bpf_testmod's bpf_testmod_test_read. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- .../selftests/bpf/progs/refcounted_kptr.c | 71 +++++++++++++++++++ .../bpf/progs/refcounted_kptr_fail.c | 28 ++++++++ 2 files changed, 99 insertions(+)