On 11/23/22 8:09 PM, Alexei Starovoitov wrote:
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.
Agree. This will make code reasoning much simpler.
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'.
Indeed, my previous approach changed rcu ptr to a scalar
and error message might mislead people...
