Re: [PATCH] rust: cpumask: Validate CPU number in set() and clear()

[Boqun Feng <boqun.feng@gmail.com>]

On Fri, Jun 06, 2025 at 09:34:04PM +0200, Benno Lossin wrote:
[...]
> >> >
> >> > All `Cpumask` functions then take `CpuId` instead of `i32` as the
> >> > parameter. Needless to say if we were to have a cpumask_next() wrapper,
> >> > the return value will be `CpuId` (or `Option<CpuId>`), i.e. if a bit was
> >> > set in a cpumask, then it must represent a correct cpu id.
> >> >
> >> > Make sense?
> >> 
> >> Just to make sure, the `nr_cpu_ids` stays constant, right?
> >> 
> >
> > Sort of. I believe the value won't be changed once the kernel boots, in
> > most cases (modulo NR_CPUS=1 or CONFIG_FORCE_NR_CPUS=y), it's a
> > read-mostly variable not a constant, and the value gets set by either a
> > kernel command line or how many CPUs the kernel actually detect at boot
> > time. See:
> >
> > https://github.com/Rust-for-Linux/linux/blob/rust-next/kernel/smp.c#L995:w
> 
> It's allowed to increase, but if it ever decreases, the invariant of
> `CpuId` will be wrong (ie it's not *invariant* :).
> 

I don't think it's allowed to be changed once set after boot, certainly
not allowed to decrease.

> >> >> @@ -101,10 +108,16 @@ pub fn set(&mut self, cpu: u32) {
> >> >>      /// This mismatches kernel naming convention and corresponds to the C
> >> >>      /// function `__cpumask_clear_cpu()`.
> >> >>      #[inline]
> >> >> -    pub fn clear(&mut self, cpu: i32) {
> >> >> +    pub fn clear(&mut self, cpu: i32) -> Result {
> >> >> +        // SAFETY: It is safe to read `nr_cpu_ids`.
> >> >> +        if unsafe { cpu as u32 >= bindings::nr_cpu_ids } {
> >> >
> >> > You probably want to check whether `bindings::nr_cpu_ids` can be
> >> > accessible if NR_CPUS == 1 or CONFIG_FORCE_NR_CPUS=y, because then
> >> > nr_cpu_ids is a macro definition.
> >> 
> >> Just define a helper function?
> >> 
> >
> > Maybe, but it is then "a variable read" vs "a FFI function call" if we
> > want to check every time in clear()/set(), of course if we only check it
> > in CpuId::from_i32() mentioned above, the performance impact shouldn't
> > be observable, because we won't call that method often.
> 
> Sure, you could also have a rust function that is inlined that does the
> two different checks depending on the config.
> 

Yeah, that's what I'm thinking about as well.

Regards,
Boqun

> > Either, I was just pointing out the current fix may cause build errors.
> 
> Yeah that should be fixed.
> 
> ---
> Cheers,
> Benno