Re: [RFC PATCH bpf-next 6/7] sched_ext: Make SCX use BPF capabilities

[Juntong Deng]

On 2025/1/17 16:58, Tejun Heo wrote:
> Hello,
>
> On Thu, Jan 16, 2025 at 07:41:11PM +0000, Juntong Deng wrote:
> ...
> > +static int bpf_scx_bpf_capabilities_adjust(unsigned long *bpf_capabilities,
> > +					   u32 context_info, bool enter)
> > +{
> > +	if (enter) {
> > +		switch (context_info) {
> > +		case offsetof(struct sched_ext_ops, select_cpu):
> > +			ENABLE_BPF_CAPABILITY(bpf_capabilities, BPF_CAP_SCX_KF_SELECT_CPU);
> > +			ENABLE_BPF_CAPABILITY(bpf_capabilities, BPF_CAP_SCX_KF_ENQUEUE);
> > +			break;
> ...
> > +		}
> > +	} else {
> > +		switch (context_info) {
> > +		case offsetof(struct sched_ext_ops, select_cpu):
> > +			DISABLE_BPF_CAPABILITY(bpf_capabilities, BPF_CAP_SCX_KF_SELECT_CPU);
> > +			DISABLE_BPF_CAPABILITY(bpf_capabilities, BPF_CAP_SCX_KF_ENQUEUE);
> > +			break;
> ...
> > +	}
> > +	return 0;
> > +}
>
>  From sched_ext's POV, this, or whatever works is fine but I have some basic
> comments:
>
> - The caps are u32. If all kfuncs use this facility for access control, it's
>    plausible or even likely that 32 is not going to be enough. I suppose it
>    can be turned into u64 and then a proper bitmap later? Maybe good idea to
>    start out with a proper bitmap in the first place?

Thanks for your reply.

I considered this, and 32 capabilities is definitely not enough.

So although in BTF_ID_FLAGS the capability is 32 bits, this is used as
an index and a bitmap is used in struct bpf_verifier_env.

We can have UINT_MAX quantity capability.

> - There are benefits to centralizing all the caps in a single place but it
>    can also be kinda cumbersome.

Yes, I agree, maybe centralized declarations are cumbersome.

But the purpose of centralized declaration is to give each capability a
unique number for distinction.

I am not sure there is a non-centralized declarative way to do this.

Do you have any suggested approach?

> - Even with global defs, the cap adjustments are procedural, not
>    declarative. If it needs to be procedural anyway, I wonder whether the
>    global defs are necessary in the first place. What prevents implementing
>    it the other way around - pass in the calling context and provide helpers
>    and macros to respond yay or nay procedurally.

You are right!

Thanks for pointing this out!

If it is procedural, then global definitions are really not necessary.

I will rethink this.

> Thanks.