Re: [PATCH bpf-next v9 3/4] bpf: Add kfunc bpf_rcu_read_lock/unlock()

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Yonghong Song <yhs@meta.com>
Cc: Yonghong Song <yhs@fb.com>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>,
	Martin KaFai Lau <martin.lau@kernel.org>
Subject: Re: [PATCH bpf-next v9 3/4] bpf: Add kfunc bpf_rcu_read_lock/unlock()
Date: Wed, 23 Nov 2022 20:09:09 -0800	[thread overview]
Message-ID: <CAADnVQ+uoW+G9Uts3B1q=9Qxws3+dmLqUUWSVaeRgRYLvNkkQw@mail.gmail.com> (raw)
In-Reply-To: <37b640ad-7258-adb8-7cec-23ae776f5764@meta.com>

On Wed, Nov 23, 2022 at 6:57 PM Yonghong Song <yhs@meta.com> wrote:
> >>
> >> +       rcu_lock = is_kfunc_bpf_rcu_read_lock(&meta);
> >> +       rcu_unlock = is_kfunc_bpf_rcu_read_unlock(&meta);
> >> +       if (env->cur_state->active_rcu_lock) {
> >> +               struct bpf_func_state *state;
> >> +               struct bpf_reg_state *reg;
> >> +
> >> +               if (rcu_lock) {
> >> +                       verbose(env, "nested rcu read lock (kernel function %s)\n", func_name);
> >> +                       return -EINVAL;
> >> +               } else if (rcu_unlock) {
> >> +                       bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
> >> +                               if (reg->type & MEM_RCU)
> >> +                                       __mark_reg_unknown(env, reg);
> >> +                       }));
> >
> > That feels too drastic.
> > rcu_unlock will mark all pointers as scalar,
> > but the prog can still do bpf_rdonly_cast and read them.
> > Why force the prog to jump through such hoops?
> > Are we trying to prevent some kind of programming mistake?
> >
> > Maybe clear MEM_RCU flag here and add PTR_UNTRUSTED instead?
>
> The original idea is to prevent rcu pointer from leaking out of rcu read
> lock region. The goal is to ensure rcu common practice. Maybe this is
> indeed too strict. As you suggested, the rcu pointer can be marked as
> PTR_UNTRUSTED so it can still be used outside rcu read lock region
> but not able to pass to helper/kfunc.

This is the part where gcc vs clang difference can be observed:

bpf_rcu_read_lock();
ptr = rcu_ptr->rcu_marked_field;
bpf_rcu_read_unlock();
ptr2 = ptr->var;
here it will fail on clang because ptr is a scalar
while it will work on gcc because ptr is still ptr_to_btf_id
and rcu_read_lock/unlock are nop-s.

Making it PTR_UNTRUSTED will still have difference gcc vs clang,
but more subtle: ptr_to_btf_id|untrusted vs ptr_to_btf_id.

So it's best to limit new kfuncs to clang.
ptr_untrusted here is a minor detail. We can change it later.
It feels that ptr_untrusted will be easier on users
especially if we improve error messages.
Say that ptr2 above is later passed into helper/kfunc
and the verifier errors on it.
If the message says 'expected trusted ptr_to_btf_id but scalar is seen'
the prog author will be perplexed, because it's clearly a pointer.
'Why the verifier is so dumb?...'
But if we do ptr_untrusted the message will be:
'expected trusted ptr_to_btf_id but untrusted ptr_btf_id is seen'
which may be interpreted by the user: "hmm. I'm probably doing
something wrong with the rcu section here'.