Re: [PATCH bpf-next v3 1/2] bpf: Enforce RCU protection for KF_RCU_PROTECTED

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-09-18 at 23:37 +0200, Kumar Kartikeya Dwivedi wrote:
> On Thu, 18 Sept 2025 at 23:00, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2025-09-17 at 03:27 +0000, Kumar Kartikeya Dwivedi wrote:
> > > Currently, KF_RCU_PROTECTED only applies to iterator APIs and that too
> > > in a convoluted fashion: the presence of this flag on the kfunc is used
> > > to set MEM_RCU in iterator type, and the lack of RCU protection results
> > > in an error only later, once next() or destroy() methods are invoked on
> > > the iterator. While there is no bug, this is certainly a bit
> > > unintuitive, and makes the enforcement of the flag iterator specific.
> > > 
> > > In the interest of making this flag useful for other upcoming kfuncs,
> > > e.g. scx_bpf_cpu_curr() [0][1], add enforcement for invoking the kfunc
> > > in an RCU critical section in general.
> > > 
> > > This would also mean that iterator APIs using KF_RCU_PROTECTED will
> > > error out earlier, instead of throwing an error for lack of RCU CS
> > > protection when next() or destroy() methods are invoked.
> > > 
> > > In addition to this, if the kfuncs tagged KF_RCU_PROTECTED return a
> > > pointer value, ensure that this pointer value is only usable in an RCU
> > > critical section. There might be edge cases where the return value is
> > > special and doesn't need to imply MEM_RCU semantics, but in general, the
> > > assumption should hold for the majority of kfuncs, and we can revisit
> > > things if necessary later.
> > > 
> > >   [0]: https://lore.kernel.org/all/20250903212311.369697-3-christian.loehle@arm.com
> > >   [1]: https://lore.kernel.org/all/20250909195709.92669-1-arighi@nvidia.com
> > > 
> > > Tested-by: Andrea Righi <arighi@nvidia.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > 
> > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > 
> > [...]
> > 
> > > @@ -14037,6 +14045,8 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > > 
> > >                       if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache])
> > >                               regs[BPF_REG_0].type |= PTR_UNTRUSTED;
> > > +                     else if (is_kfunc_rcu_protected(&meta))
> > > +                             regs[BPF_REG_0].type |= MEM_RCU;
> > > 
> > >                       if (is_iter_next_kfunc(&meta)) {
> > >                               struct bpf_reg_state *cur_iter;
> > 
> > The code below this hunk looks as follows:
> > 
> >                         if (is_iter_next_kfunc(&meta)) {
> >                                 struct bpf_reg_state *cur_iter;
> > 
> >                                 cur_iter = get_iter_from_state(env->cur_state, &meta);
> > 
> >                                 if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
> >                                         regs[BPF_REG_0].type |= MEM_RCU;
> >                                 else
> >                                         regs[BPF_REG_0].type |= PTR_TRUSTED;
> >                         }
> > 
> > Do we want to reduce it to:
> > 
> >                         if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache])
> >                                 regs[BPF_REG_0].type |= PTR_UNTRUSTED;
> >                         else if (is_kfunc_rcu_protected(&meta))
> >                                 regs[BPF_REG_0].type |= MEM_RCU;
> >                         else if (is_iter_next_kfunc(&meta))
> >                                 regs[BPF_REG_0].type |= PTR_TRUSTED;
> 
> I thought so too but we cannot do this. Suppose that the RCU read lock
> is dropped and reacquired between new() and next(). Right now, we rely
> on MEM_RCU in iter->type stack object that gets invalidated properly.
> With such a change we'd lose the ability to track continued protection
> using RCU while the iterator is alive on the stack, and continue to
> mark returned pointers as MEM_RCU.

The change I suggest does not invalidate this mechanics.
The iterator is still marked with MEM_RCU and this mark is converted
to PTR_UNTRUSTED when RCU section exits.
The check for PTR_UNTRUSTED happens in process_iter_arg() called
from check_kfunc_args().

> 
> > 
> > And mark relevant iterator next (and destroy?) functions as KF_RCU_PROTECTED?
> > (bpf_iter_css_next, bpf_iter_task_next, bpf_iter_scx_dsq_next).
> > 
> > I ask, because setting |= MEM_RCU in two places of this if branch
> > looks a bit iffy.
> > 
> > [...]