Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool

[Yury Norov]

On Mon, Aug 17, 2026 at 04:08:09PM +0200, Danilo Krummrich wrote:
> On Mon Aug 17, 2026 at 3:02 PM CEST, Gary Guo wrote:
> > Now, with `NonZero` or `Bounded`, we are doing none of that. The only thing here
> > is that there is a range restriction. Other than the value restriction
> > themselves, they carry no other semantic meanings. How you interpret these types
> > still fully depend on the API that accepts them. Therefore, it is very common
> > that you'd be using these with literals, and it becomes an ergnomic pain.
> 
> I agree that NonZero and Bounded are on the weaker end of the argument. But
> there's still the flexibility argument.
> 
> The API itself, i.e. alloc_area(), does not need to bother with how the value is
> checked. I.e. is it a runtime check, compile or build time check, or is it even
> unchecked (or panicking) because we can derive the invariant from another type.
> 
> > Personally I value ergnomics higher than possibility of misuse if latter can
> > be easily mitigated otherwise (in this case, by WARN_ON or just support
> > zero-sized alloc).
> 
> Both is not a mitigation IMO.
> 
> WARN_ON() is does not prevent misuse of the API in the first place and in case
> the value comes from userspace even introduces a vulnerability.
> 
> Making zero a valid argument simply ignores the problem or just moves it
> elsewhere, e.g. where the caller has to validate the returned type instead, i.e.
> the ChannelIdArea. IOW, we'd remove the invariant ChannelIdArea carries.

Let's get back to roots, maybe?

The kernel functions don't check parameters, with the very few
exceptions, because the kernel trusts itself.

What are those exceptions?

1. strnlen(char *s, unsigned count). Here the 'count' hard-stops
traversing the array 's' in case it's not null-terminated. Why?
Because c-string is the bad data structure, and it's a very common,
very well known vulnerability of C strings to have them not
null-terminated. It brings tons of troubles, that's why.

GENMASK(hi, lo) falls into the same category, for example. People
always think of it as GENMASK(lo, hi). Bad design...

2. FIELD_PREP(mask, val) checks mask for being dense and wide enough to
fit the val. It's a very basic operation, has 0 cost and implemented at
compile time. In fact, FIELD_PREP() is a compile-time macro. The
corresponding run-time field_prep() doesn't check for any consistency.

3. int pin_user_pages_fast(...) 
   {
        if (!is_valid_gup_args(pages, NULL, &gup_flags, FOLL_PIN))
                return -EINVAL;
        return gup_fast_fallback(start, nr_pages, gup_flags, pages);
   }

It's not a validator per se, like nr_pages == 0. It's rather a
consistency checker.

I don't think that alloc(size, align) matches one of the above. The
underlying bitmap is not as bad as C string, the alloc_area() is not
a compile-time macro, and NonZero<0>() doesn't look like a complex
cross-validation.

>From maintenance perspective, I don't think it's even possible to pass
0 into the alloc_area(), so that it slips through the reviewers
attention. NonZero<0>() is a pure complication for absolutely no
reason.

Again, any kernel API trusts it's caller. It holds for assembler,
for C, and I don't see any reason why it shouldn't hold for Rust.

Rust is more restrictive to undefined behavior. Having that in mind,
we may want to prevent the known undef. But that should never become
a part of the API. And undef isn't the case for alloc(0) - instead
of making non-zero 'size' a part of API contract, we must make the
function behavior well defined for this case. kmalloc(0), for example,
returns ZERO_SIZE_PTR. The pool.alloc_area(0) may return None, and
probably trigger some warning.

The underlying bitmap_find_next_zero_area_of(0) doesn't behave well, so
Rust wrapper should take care of it. pool.alloc_area() doesn't call
the C function directly, it calls the wrapper. The problem must be
already resolved at this level.

Thanks,
Yury