Re: [PATCH v2 bpf-next 7/7] selftests/bpf: Add tests for rbtree API interaction in sleepable progs

[David Marchevsky <david.marchevsky@linux.dev>]

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";