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

From: Yonghong Song <yhs@meta.com>
To: Martin KaFai Lau <martin.lau@linux.dev>, Yonghong Song <yhs@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Martin KaFai Lau <martin.lau@kernel.org>,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v8 4/4] selftests/bpf: Add tests for bpf_rcu_read_lock()
Date: Tue, 22 Nov 2022 17:13:46 -0800	[thread overview]
Message-ID: <b1b8d321-1c3a-ca41-707f-95b3cef7f124@meta.com> (raw)
In-Reply-To: <201c1603-cb3e-7893-c411-e7949ef8e9d3@linux.dev>

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.
> 