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@xxxxxx>
>> ---
>> .../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";
