From: Boqun Feng <boqun.feng@gmail.com>
To: Danilo Krummrich <dakr@redhat.com>
Cc: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com,
gary@garyguo.net, bjorn3_gh@protonmail.com,
benno.lossin@proton.me, a.hindborg@samsung.com,
aliceryhl@google.com, rust-for-linux@vger.kernel.org
Subject: Re: [PATCH] rust: alloc: fix dangling pointer in VecExt<T>::reserve()
Date: Tue, 30 Apr 2024 13:59:19 -0700 [thread overview]
Message-ID: <ZjFbp_XABvDRXIaL@boqun-archlinux> (raw)
In-Reply-To: <ZjFYvPyTl6UaYSrn@pollux>
On Tue, Apr 30, 2024 at 10:46:52PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 30, 2024 at 11:33:39AM -0700, Boqun Feng wrote:
> > On Tue, Apr 30, 2024 at 06:42:03PM +0200, Danilo Krummrich wrote:
> > > On Mon, Apr 29, 2024 at 03:01:10PM -0700, Boqun Feng wrote:
> > > > On Mon, Apr 29, 2024 at 11:01:45PM +0200, Danilo Krummrich wrote:
> > > > > On 4/29/24 21:52, Boqun Feng wrote:
> > > > > > On Mon, Apr 29, 2024 at 09:24:04PM +0200, Danilo Krummrich wrote:
> > > > > > > Currently, a Vec<T>'s ptr value, after calling Vec<T>::new(), is
> > > > > > > initialized to Unique::dangling(). Hence, in VecExt<T>::reserve(), we're
> > > > > > > passing a dangling pointer (instead of NULL) to krealloc() whenever a
> > > > > > > new Vec<T> is created through VecExt<T> extension functions.
> > > > > > >
> > > > > > > This only works since it happens that Unique::dangling()'s value (0x1)
> > > > > > > falls within the range between 0x0 and ZERO_SIZE_PTR (0x10) and
> > > > > > > krealloc() hence treats it the same as a NULL pointer however.
> > > > > > >
> > > > > >
> > > > > > Good catch!
> > > > > >
> > > > > > > This isn't a case we should rely on, especially since other kernel
> > > > > > > allocators are not as tolerant. Instead, pass a real NULL pointer to
> > > > > > > krealloc_aligned() if Vec<T>'s capacity is zero.
> > > > > > >
> > > > > > > Fixes: 5ab560ce12ed ("rust: alloc: update `VecExt` to take allocation flags")
> > > > > >
> > > > > > However, since this commit is not upstreamed yet, so it's suject to
> > > > > > change, I'd avoid the "Fixes" tag here. Alternatively, Miguel can fold
> > > > > > this patch into that commit in his tree.
> > > > >
> > > > > I'd be surprised if rust-next wouldn't be fast-forward only, is it? If
> > > >
> > > > Well, I cannot speak for Miguel, but there's no guarantee of that IMO.
> > >
> > > @Miguel, which one is it?
> > >
> >
> > Just FYI, linux-next has all the history of rust-next snapshots, in
> > 20230411:
> >
> > commit ("rust: sync: add functions for initializing
> > `UniqueArc<MaybeUninit<T>>`") has commit id
> > 2d0dec625d872a41632a68fce2e69453ed87df91:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20230411&id=2d0dec625d872a41632a68fce2e69453ed87df91
> >
> > in 20230421 (also in the PULL request), the commmit changes its id to
> > 1944caa8e8dcb2d93d99d8364719ad8d07aa163f :
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20230421&id=1944caa8e8dcb2d93d99d8364719ad8d07aa163f
>
> Yes, linux-next is an exception. But linux-next is also never directly pulled
> into Linus' tree.
>
The point is that linux-next merges a snapshot of the -next branches it
tracks, and what I post is an example that a particular commit changes
its id in rust-next. In other words, you CANNOT assume that today's
rust-next will be the final version merged in Linus' tree.
> >
> > The -next branches are subject to rebase for multiples reasons (e.g.
> > applying a Reviewed-by tag after queued), so the commit id in these
> > branches is not guaranteed to stay the same.
>
> I've never seen that this has been common practice after patches have been
> applied already.
>
Here you go:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20230411&id=105d7c03679002c977e98b13e7a4008cc3933fde
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20230421&id=692e8935e23efab6c5d5fc4b003816b33c8082f7
in this case, Alice's Reviewed-by was added between different versions
(snapshots) of rust-next.
Regards,
Boqun
next prev parent reply other threads:[~2024-04-30 20:59 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 19:24 [PATCH] rust: alloc: fix dangling pointer in VecExt<T>::reserve() Danilo Krummrich
2024-04-29 19:52 ` Boqun Feng
2024-04-29 21:01 ` Danilo Krummrich
2024-04-29 22:01 ` Boqun Feng
2024-04-30 16:42 ` Danilo Krummrich
2024-04-30 18:33 ` Boqun Feng
2024-04-30 20:46 ` Danilo Krummrich
2024-04-30 20:59 ` Boqun Feng [this message]
2024-04-30 21:08 ` Boqun Feng
2024-04-30 22:19 ` Danilo Krummrich
2024-04-30 22:41 ` Boqun Feng
2024-04-30 22:06 ` Boqun Feng
2024-04-30 22:44 ` Miguel Ojeda
2024-04-30 8:25 ` Alice Ryhl
2024-04-30 12:07 ` Danilo Krummrich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjFbp_XABvDRXIaL@boqun-archlinux \
--to=boqun.feng@gmail.com \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=dakr@redhat.com \
--cc=gary@garyguo.net \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).