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

[Danilo Krummrich <dakr@redhat.com>]

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.

> 
> 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?

> 
> Regards,
> Boqun
> 
> > > > 
> > > > 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
>