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

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

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