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

[Danilo Krummrich <dakr@redhat.com>]

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?

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

I can't speak for other maintainers, but AFAICT it's rather uncommon to rewrite
the history once it has been exposed to the public. It'd be especially uncommon
for a subsystems's -next branch. See also [1].

Patch series shouldn't introduce bugs in between patches, indeed. They should
also not break the build, neither in general nor in between, for the reasons you
mentioned. Ideally, this should be fixed before we hit public trees, but if it
happens, as mentioned above, I think it's rather uncommon to rewrite history
because of that.

[1] https://docs.kernel.org/maintainer/rebasing-and-merging.html#rebasing

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

Even though I wouldn't mind personally, my experience has been that people do
care about the difference.

> 
> Regards,
> Boqun
> 
> > - Danilo
> > 
> [...]
>