linux-kernel - Re: [PATCH v5 04/26] rust: alloc: implement `Allocator` for `Kmalloc`

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: <Zry4iOGtR0nd6lNP@cassiopeiae>
Date: Wed, 14 Aug 2024 16:00:40 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.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 04/26] rust: alloc: implement `Allocator` for `Kmalloc`

On Wed, Aug 14, 2024 at 03:50:27PM +0200, Alice Ryhl wrote:
> On Wed, Aug 14, 2024 at 3:48 PM Danilo Krummrich <dakr@...nel.org> wrote:
> >
> > On Wed, Aug 14, 2024 at 03:44:56PM +0200, Alice Ryhl wrote:
> > > On Wed, Aug 14, 2024 at 3:36 PM Danilo Krummrich <dakr@...nel.org> wrote:
> > > >
> > > > On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote:
> > > > > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@...nel.org> wrote:
> > > > > >
> > > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
> > > > > > typically used for objects smaller than page size.
> > > > > >
> > > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`.
> > > > > >
> > > > > > It serves as allocator for the subsequently introduced types `KBox` and
> > > > > > `KVec`.
> > > > > >
> > > > > > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > > > > > ---
> > > > > >  rust/helpers.c                 |  3 +-
> > > > > >  rust/kernel/alloc.rs           |  2 +-
> > > > > >  rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++-
> > > > > >  3 files changed, 64 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > > > > index 92d3c03ae1bd..9f7275493365 100644
> > > > > > --- a/rust/helpers.c
> > > > > > +++ b/rust/helpers.c
> > > > > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> > > > > >
> > > > > > -void * __must_check __realloc_size(2)
> > > > > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> > > > > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
> > > > > >  {
> > > > > >         return krealloc(objp, new_size, flags);
> > > > > >  }
> > > > >
> > > > > Why are the various annotations on this helper being removed?
> > > >
> > > > rust_helper_krealloc() is only called from Rust, hence neither __must_check nor
> > > > __realloc_size() should have any effect.
> > > >
> > > > I also do not apply them in subsequent commits for the vrealloc() and
> > > > kvrealloc() helpers for this reason and removed them here for consistency.
> > > >
> > > > > This deserves an explanation in the commit message.
> > > >
> > > > I can also add a separate commit for that.
> > >
> > > I think your change would be more obviously correct if you keep them.
> >
> > As in generally, or just for this patch?
> >
> > Generally, I don't think we should indicate compiler checks that actually are
> > never done.
> >
> > For this patch, yes, it's probably better to separate it.
> 
> In general. If you keep it, then I don't have to think about whether
> it affects bindgen's output. That's the main reason.

Well, it doesn't.

If we keep them, we'd consequently also need to add them for vrealloc() and
kvrealloc(). But again, they don't do anything for us, and hence are more
misleading than helpful IMO.

> 
> Alice
>