Re: [PATCH sched_ext/for-7.3-fixes] sched_ext: Add a size argument to scx_bpf_cid_topo() so struct scx_cid_topo can grow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: scx_bpf_cid_topo() requires unnecessary buffer initialization
--

commit 20cb7828d17e22cbc3ce2d88fade05994d50f729
Author: Tejun Heo <tj@kernel.org>

sched_ext: Add a size argument to scx_bpf_cid_topo() so struct scx_cid_topo can grow

This commit adds a size argument to the scx_bpf_cid_topo() kfunc to allow
the struct scx_cid_topo layout to grow over time. The kernel copies the
smaller of the provided buffer size and the kernel's struct size, setting
the remainder to -1.

> diff --git a/kernel/sched/ext/cid.c b/kernel/sched/ext/cid.c
> index bc4eee5bb4cbf..4b08866d75f6a 100644
> --- a/kernel/sched/ext/cid.c
> +++ b/kernel/sched/ext/cid.c
[ ... ]
> @@ -912,30 +912,36 @@ bool scx_cmask_empty(const struct scx_cmask *m)
>  /**
>   * scx_bpf_cid_topo - Copy out per-cid topology info
>   * @cid: cid to look up
> - * @out__uninit: where to copy the topology info; fully written by this call
> + * @out: where to copy the topology info
> + * @out__sz: size of @out, the program's sizeof(struct scx_cid_topo)
>   * @aux: implicit BPF argument to access bpf_prog_aux hidden from BPF progs
>   *
> - * Fill @out__uninit with the topology info for @cid. Trigger scx_error() if
> - * @cid is out of range. If @cid is valid but in the no-topo section, all fields
> - * are set to -1. All fields are also set to -1 when no cid tables have been
> - * published yet, which a program may observe while racing the root enable.
> + * Fill @out with the topology info for @cid. Trigger scx_error() if @cid is out
> + * of range. If @cid is valid but in the no-topo section, all fields are set to
> + * -1. All fields are also set to -1 when no cid tables have been published yet,
> + * which a program may observe while racing the root enable.
> + *
> + * The program's struct may be older or newer than the kernel's. The smaller of
> + * @out__sz and the kernel's size is copied and the rest of @out is set to -1.
>   */
> -__bpf_kfunc void scx_bpf_cid_topo(s32 cid, struct scx_cid_topo *out__uninit,
> +__bpf_kfunc void scx_bpf_cid_topo(s32 cid, struct scx_cid_topo *out, size_t out__sz,
>  				  const struct bpf_prog_aux *aux)

[Severity: Medium]
Will dropping the __uninit suffix from the out parameter force BPF
programs to initialize the struct scx_cid_topo buffer before calling
this kfunc?

Since the function fully overwrites the buffer via memset() and memcpy(),
forcing BPF programs to pre-initialize the memory creates unnecessary
overhead, as the verifier will reject uninitialized stack memory without
the MEM_UNINIT flag (which is applied when using the __uninit suffix).

The verifier should be able to map the size parameter based on the __sz
suffix regardless of the memory buffer's name (similar to existing kfuncs
like bpf_kfunc_test_uninit_mem(void *out__uninit, u32 out__sz)), so
out__uninit and out__sz can be used together safely.

>  {
> +	size_t len = min(out__sz, sizeof(*out));
>  	struct scx_cid_topo *topo;
>  	struct scx_sched *sch;
>  
> +	/* the error cases and fields the kernel lacks read as -1 */
> +	memset(out, 0xff, out__sz);
> +
>  	guard(rcu)();
>  
>  	sch = scx_prog_sched(aux);
>  	topo = rcu_dereference(scx_cid_topo);
> -	if (unlikely(!sch) || !cid_valid(sch, cid) || unlikely(!topo)) {
> -		*out__uninit = SCX_CID_TOPO_NEG;
> +	if (unlikely(!sch) || !cid_valid(sch, cid) || unlikely(!topo))
>  		return;
> -	}
>  
> -	*out__uninit = topo[cid];
> +	memcpy(out, &topo[cid], len);
>  }
>  
>  __bpf_kfunc_end_defs();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/fa6479d36b528573565ca22914451317@kernel.org?part=1