Re: [PATCH bpf-next v8 4/4] selftests/bpf: Add tests for bpf_rcu_read_lock()

[Yonghong Song <yhs@meta.com>]

On 11/22/22 4:56 PM, Martin KaFai Lau wrote:
> On 11/22/22 11:53 AM, Yonghong Song wrote:
>> +SEC("?fentry.s/" SYS_PREFIX "sys_nanosleep")
>> +int task_acquire(void *ctx)
>> +{
>> +    struct task_struct *task, *real_parent;
>> +
>> +    task = bpf_get_current_task_btf();
>> +    bpf_rcu_read_lock();
>> +    real_parent = task->real_parent;
>> +    /* acquire a reference which can be used outside rcu read lock 
>> region */
>> +    real_parent = bpf_task_acquire(real_parent);
> Does the bpf_task_acquire() kfunc need a change to do 
> refcount_inc_not_zero() and KF_RET_NULL?

We have this definition in kernel:
BTF_ID_FLAGS(func, bpf_task_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)

So the argument is trusted args so, either marked as 
PTR_TRUSTED/MEM_ALLOC or have a reference acquired already, so
I guess we should be fine here.

> 
> Also, some more 'skip' checks in prog_tests/rcu_read_lock.c is needed 
> for gcc. This is failing in gcc CI:
> 
> https://github.com/kernel-patches/bpf/actions/runs/3527747280/jobs/5917628248#step:6:5624
> 
>    ; bpf_rcu_read_lock();
>    2: (85) call bpf_rcu_read_lock#26650
>    ; real_parent = task->real_parent;
>    3: (79) r1 = *(u64 *)(r6 +1416)       ; 
> R1_w=ptr_task_struct(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
>    ; real_parent = bpf_task_acquire(real_parent);
>    4: (85) call bpf_task_acquire#26666
>    R1 must be referenced or trusted
>    processed 5 insns (limit 1000000) max_states_per_insn 0 total_states 
> 0 peak_states 0 mark_read 0
>    -- END PROG LOAD LOG --
>    libbpf: prog 'task_acquire': failed to load: -22

Yes, we should skip this for gcc compiled kernel since rcu tag is not
available.

> 
>> +    bpf_rcu_read_unlock();
>> +    (void)bpf_task_storage_get(&map_a, real_parent, 0, 0);
>> +    bpf_task_release(real_parent);
>> +    return 0;
>> +}
>> +
>> +SEC("?fentry.s/" SYS_PREFIX "sys_nanosleep")
>> +int no_lock(void *ctx)
>> +{
>> +    struct task_struct *task, *real_parent;
>> +
>> +    /* no bpf_rcu_read_lock(), old code still works */
>> +    task = bpf_get_current_task_btf();
>> +    real_parent = task->real_parent;
>> +    bpf_printk("pid %u\n", real_parent->pid);
> 
> nit. Can bpf_printk be avoided here?

I could add a target_pid comparison to prevent the issue. But
will follow your suggestion to use a different function instead
of bpf_printk.

> 
> Others lgtm.
>