Re: [PATCH] rust: alloc: fix dangling pointer in VecExt<T>::reserve()

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, May 01, 2024 at 12:19:17AM +0200, Danilo Krummrich wrote:
> On Tue, Apr 30, 2024 at 02:08:31PM -0700, Boqun Feng wrote:
> > On Tue, Apr 30, 2024 at 01:59:19PM -0700, Boqun Feng wrote:
> > > 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.
> > > 
> 
> I meant -next branches in general. As I understood you previously I thought
> you're not sure whether rust-next is prone to altering its history. Now, you're
> clearly saying it is - noted.
> 

Yeah, I wasn't so sure myself, so I digged into the history. My
understanding is that maintainers are usually allowed to alter the
history, and most subsystems I work on do alter the histories from time
to time.

// It's a curse of the knowledge, once I see the power of rewriting
// history, I cannot unsee it ;-)

> > 
> > nor it will be the base of the final pull request.
> > 
> > In short words, -next branches are subject to rebase for various
> > reasons. Commit id from them is not stable, period.
> 
> As you just educated me, for rust-next that seems to be case - again, good to
> know. Generally, I don't think that's commonly the case though, hence my
> surprise.
> 
> This throws up a new questions though. Does this mean you'll actually just
> squash fixes into commits that are in rust-next only?
> 

Again it's up to Miguel. For this particular case, in the common case of
history-rewriting-allowed subsystem, folding your patch in (with proper
update to commit log and SoB) is an option. Taking it as it is is also 
an option (maybe drop the "Fixes" tag), since although it's a bug, no
in-kernel users really get affected(?). If it's not -rc6 already, I
would definitely lean towards folding. That's why I said I'm OK either
way.

Regards,
Boqun