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

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

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.

> fast-forward only, the commit IDs should be preserved on merge, hence it should
> be fine to keep the "Fixes" tag.
> 
> As for squashing fixes into existing commits, this is something I would generally
> not recommend doing. This would be a non-fast-forward operation and hence break
> potential references to other commits in general (not only "Fixes" tags). Plus,

Yes, but here what you fix is a bug, and generally, if we find a bug in
some commit and that commit is not upstreamed, we should rework that
commit other than introducing another patch that fixes the bug. It'll
provide better bisect and less confusion. It's the same reason that why
we don't allow a patch series to include a bug in the middle.

> it's usually not providing a great motivation for potential contributors.
> 

With proper SoB tags and other tags, I don't see a big difference here,
or I'm missing something subtle?

Regards,
Boqun

> - Danilo
> 
[...]