On Wed, Nov 23, 2022 at 6:57 PM Yonghong Song <yhs@xxxxxxxx> 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'.
