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

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

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

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.

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

That link says:

"""
Some trees (linux-next being a significant example) are frequently
rebased by their nature, and developers know not to base work on them.
"""

and in rust-for-linux.com, it says[2]:

	It is part of linux-next.

So I expect rebasing of rust-next is expected. Normally it won't be a
problem, since most maintainers will maintain the branch in a way that
patches can still be applied on -next branches after rebasing, but
"Fixes" tag may not work due to the change of commit id.

[2]: https://rust-for-linux.com/branches#rust-next

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

Honestly it's not that uncommon to me, since -next branches are more for
trial and test purposes. There are a lot of testing happening at
linux-next level that I know of, and that's the purpose of linux-next
and -next branches, so fixing a bug in a -next branch is not uncommon.
Plus I generally think a pull request is the same as a patchset, I'd
avoid adding a commit at last saying "this commit fixes a bug introduced
by some commit in the middle".

But once again, it's up to Miguel ;-)

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

Thank you, now I see. I think we should work hard on that to recognize
the contribution in mutliple ways. Will keep that in mind.

Regards,
Boqun