Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard <bshephar@bne-home.net>]

On Thu, Nov 27, 2025 at 10:42:31PM +0900, Alexandre Courbot wrote:
> On Thu Nov 27, 2025 at 10:07 PM JST, Brendan Shephard wrote:
> > Changed page_align() to return Option<usize> which allows for validation
> 
> Please use imperative style as per the patch submission guidelines:
> "Change page_align() ...".
> 
> https://docs.kernel.org/process/submitting-patches.html
> 

Eh, my bad. Yeah, I'll fix that in v2.

> > of the provided addr value. This ensures that any value that is provided
> > within one PAGE_SIZE of usize::MAX will not panic and instead page_align
> > will return None.
> >
> > Callers of page_align() should raise a EINVAL when they receive None
> > from page_align().
> 
> Callers will return the error that is relevant to them. :) This
> sentence is not needed.
> 
Ack, I'll remove that part.
> >
> > Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> > ---
> >  rust/kernel/page.rs | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index 432fc0297d4a..b78473b67003 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -26,13 +26,12 @@
> >  pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
> >
> >  /// Round up the given number to the next multiple of [`PAGE_SIZE`].
> > -///
> > -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
> > -/// [`usize`].
> > -pub const fn page_align(addr: usize) -> usize {
> > -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
> > -    // cases.
> > -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> > +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
> 
> You can doclink to [`None`].
> 
> Nit: dot at end of sentence missing.
>
Will fix this in v2

> > +pub const fn page_align(addr: usize) -> Option<usize> {
> > +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> > +        return Some(sum & PAGE_MASK);
> > +    }
> > +    None
> 
> A more idiomatic way to do this:
> 
>     addr.checked_add(PAGE_SIZE - 1).map(|sum| sum & PAGE_MASK)
> 
>

That would be nice, but alas since this is a const fn, we can't use map here.
I assume that was done for optimisation to ensure this could be referenced
at compile time. So I'm not sure we want to remove that optimisation here.