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
