Re: [PATCH sched_ext/for-6.15 v3 3/5] sched_ext: Add scx_kfunc_ids_ops_context_sensitive for unified filtering of context-sensitive SCX kfuncs

[Tejun Heo <tj@kernel.org>]

Hello,

On Fri, Feb 28, 2025 at 06:42:11PM +0000, Juntong Deng wrote:
> > > Return 0 means allowed. So kfuncs in scx_kfunc_ids_unlocked can be
> > > called by other struct_ops programs.
> > 
> > Hmm... would that mean a non-sched_ext bpf prog would be able to call e.g.
> > scx_bpf_dsq_insert()?
> 
> For other struct_ops programs, yes, in the current logic,
> when prog->aux->st_ops != &bpf_sched_ext_ops, all calls are allowed.
> 
> This may seem a bit weird, but the reason I did it is that in other
> struct_ops programs, the meaning of member_off changes, so the logic
> that follows makes no sense at all.
> 
> Of course, we can change this, and ideally there would be some groupings
> (kfunc id set) that declare which kfunc can be called by other
> struct_ops programs and which cannot.

Other than any and unlocked, I don't think other bpf struct ops should be
able to call SCX kfuncs. They all assume rq lock to be held which wouldn't
be true for other struct_ops after all.

...
> > I see, scx_dsq_move_*() are in both groups, so it should be fine. I'm not
> > fully sure the groupings are the actually implemented filtering are in sync.
> > They are intended to be but the grouping didn't really matter in the
> > previous implementation. So, they need to be carefully audited.
> 
> After you audit the current groupings of scx kfuncs, please tell me how
> you would like to change the current groupings.

Yeah, I'll go over them but after all, we need to ensure that the behavior
currently implemented by scx_kf_allowed*() matches what the new code does,
so I'd appreciate if you can go over with that in mind too. This is kinda
confusing so we can definitely use more eyes.

> > Right, the coverage there isn't perfect. Testing all conditions would be too
> > much but it'd be nice to have a test case which at least confirms that all
> > allowed cases verify successfully.
> 
> Yes, we can add a simple test case for each operation that is not
> SCX_OPS_KF_ANY.

That'd be great. Thanks.

-- 
tejun