linux-kernel - Re: [PATCH v5 06/26] rust: alloc: implement `Vmalloc` allocator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zr30pNwc5aanRaqj@cassiopeiae>
Date: Thu, 15 Aug 2024 14:29:24 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v5 06/26] rust: alloc: implement `Vmalloc` allocator

On Thu, Aug 15, 2024 at 06:48:19AM +0000, Benno Lossin wrote:
> On 15.08.24 01:20, Danilo Krummrich wrote:
> > On Thu, Aug 15, 2024 at 12:13:06AM +0200, Danilo Krummrich wrote:
> >>
> >>>
> >>>> +        ptr: Option<NonNull<u8>>,
> >>>> +        layout: Layout,
> >>>> +        flags: Flags,
> >>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>>> +        // TODO: Support alignments larger than PAGE_SIZE.
> >>>> +        if layout.align() > bindings::PAGE_SIZE {
> >>>> +            pr_warn!("Vmalloc does not support alignments larger than PAGE_SIZE yet.\n");
> >>>> +            return Err(AllocError);
> >>>
> >>> I think here we should first try to use `build_error!`, most often the
> >>> alignment will be specified statically, so it should get optimized away.
> >>
> >> Sure, we can try that first.
> > 
> > I think I spoke too soon here. I don't think `build_error!` or `build_assert!`
> > can work here, it would also fail the build when the compiler doesn't know the
> > value of the alignment, wouldn't it? I remember that I wasn't overly happy about
> > failing this on runtime either when I first thought about this case, but I also
> > couldn't think of something better.
> 
> Yes, it might fail even though the alignment at runtime will be fine.
> But that's why I suggested trying `build_error!`(or `build_assert!`)
> first, if nobody hits the case where the compiler cannot figure it out,
> then we can keep it. If there are instances, where it fails, but the
> alignment would be fine at runtime, then we can change it to the above.
> (I would add such a comment above the assert).

Unfortunately, it already does fail with just the test cases.

Anyway, even if it would have been fine, I don't think it would have been nice
for a future user to run into a build error even though the alignment is
perfectlly within bounds.

> 
> > In the end it's rather unlikely to ever hit this case, and probably even more
> > unlikely to hit it for a sane reason.
> 
> Yeah, but I still prefer the build to fail, rather than emitting a warn
> message that can be overlooked at runtime.
> 
> >>> How difficult will it be to support this? (it is a weird requirement,
> >>> but I dislike just returning an error...)
> >>
> >> It's not difficult to support at all. But it requires a C API taking an
> >> alignment argument (same for `KVmalloc`).
> 
> I see, that's good to know.
> 
> >> Coming up with a vrealloc_aligned() is rather trivial. kvrealloc_aligned() would
> >> be a bit weird though, because the alignment argument could only be really
> >> honored if we run into the vrealloc() case. For the krealloc() case it'd still
> >> depend on the bucket size that is selected for the requested size.
> 
> Yeah... Maybe some more logic on the Rust side can help with that.

Only if we reimplement `KVmalloc` in Rust, However, there are quite some special
cases in __kvmalloc_node_noprof(), i.e. fixup page flags, sanity check the size
on kmalloc failure, fail on certain page flags, etc.

I don't really want to duplicate this code, unless we absolutely have to.

> 
> >> Adding the C API, I'm also pretty sure someone's gonna ask what we need an
> >> alignment larger than PAGE_SIZE for and if we have a real use case for that.
> >> I'm not entirely sure we have a reasonable answer for that.
> 
> We could argue that we can remove an "ugly hack" (when we don't have the
> build assert, if we do have that, I don't mind not supporting it), but I
> agree that finding a user will be difficult.

I'd argue it's not really a hack to fail on something that's not supported
(yet). Allocations can (almost) always fail, this is just another case.

> 
> >> I got some hacked up patches for that, but I'd rather polish and send them once
> >> we actually need it.
> 
> Sure, just wanted to check why you don't want to do it this series.
> 
> ---
> Cheers,
> Benno
>