On Wed, Nov 26, 2025 at 11:00:00PM +0900, Alexandre Courbot wrote: > On Wed Nov 26, 2025 at 10:36 PM JST, Alice Ryhl wrote: > > On Wed, Nov 26, 2025 at 10:22:14PM +0900, Alexandre Courbot wrote: > >> On Wed Nov 26, 2025 at 6:54 PM JST, Alice Ryhl wrote: > >> > On Wed, Nov 26, 2025 at 09:31:46AM +0900, Alexandre Courbot wrote: > >> >> On Tue Nov 25, 2025 at 11:59 PM JST, Alice Ryhl wrote: > >> >> > On Tue, Nov 25, 2025 at 3:55 PM Alexandre Courbot > >> >> > <[email protected]> wrote: > >> >> >> > >> >> >> On Tue Nov 25, 2025 at 11:41 PM JST, Alice Ryhl wrote: > >> >> >> > On Tue, Nov 25, 2025 at 3:29 PM Alexandre Courbot > >> >> >> > <[email protected]> wrote: > >> >> >> >> > >> >> >> >> On Fri Nov 21, 2025 at 1:04 PM JST, bshephar wrote: > >> >> >> >> > return Err(EINVAL); > >> >> >> >> > } > >> >> >> >> > > >> >> >> >> > + let aligned_size = page_align(size); > >> >> >> >> > >> >> >> >> `page_align` won't panic on overflow, but it will still return an > >> >> >> >> invalid size. This is a job for `kernel::ptr::Alignment`, which > >> >> >> >> let's > >> >> >> >> you return an error when an overflow occurs. > >> >> >> > > >> >> >> > The Rust implementation of page_align() is implemented as (addr + > >> >> >> > (PAGE_SIZE - 1)) & PAGE_MASK, which definitely will panic on > >> >> >> > overflow > >> >> >> > if the appropriate config options are enabled. > >> >> >> > >> >> >> That's right, I skimmed its code too fast. ^_^; All the more reason > >> >> >> to > >> >> >> use `Alignment`. > >> >> > > >> >> > Alignment stores values that are powers of two, not multiples of > >> >> > PAGE_SIZE. > >> >> > >> >> Isn't PAGE_SIZE always a power of two though? > >> > > >> > Yes it is. Maybe you can elaborate on how you wanted to use Alignment? > >> > It sounds like you have something different in mind than what I thought. > >> > >> I thought we could just do something like this: > >> > >> use kernel::ptr::{Alignable, Alignment}; > >> > >> let aligned_size = size > >> .align_up(Alignment::new::<PAGE_SIZE>()) > >> .ok_or(EOVERFLOW)?; > >> > >> (maybe we could also have that `Alignment<PAGE_SIZE>` stored as a const > >> in `page.rs` for convenience, as it might be used often) > > > > If you're trying to round up a number to the next multiple of PAGE_SIZE, > > then you should use page_align() because that's exactly what the > > function does. > > > > If you think there is something wrong with the design of page_align(), > > change it instead of reimplemtning it. > > In that case I would suggest that `page_align` returns an `Option` > instead of potentially panicking. Does that sound reasonable? I cannot > find any user of it in the Rust code for now. >
This sounds reasonable, and I did wonder when I read the comment on page_align() whether it should just implement that check within the function rather than relying on callers. But, I think changing the signature of that function is probably something that should be done separately to this change. I can't see anyone using it atm either, but there could be unmerged branches or pending reviews that are using it. If that's the path we want to go down, and it sounds completely reasonable, I'll send that separately before posting a v2 of this patch to use it. Happy to take your guidance on that to satisfy the logistics of the kernels ML driven patch approach.
