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

[Danilo Krummrich <dakr@redhat.com>]

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

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

It also says that: "History that has been exposed to the world beyond your
private system should usually not be changed."

> 
> and in rust-for-linux.com, it says[2]:
> 
> 	It is part of linux-next.

Which should just mean it's regularly merged into 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.

I don't see how the exception of the linux-next tree propagates to rust-next.

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

The -next branches (including rust-next) typically carry the patches which are
queued up to be merged in Linus' tree for the next merge window.

Those branches are not for trail and test purposes. Testing of the patches must
happen before.

Their purpose is more to let things cook a while to spot potential unexpected
regressions.

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

For a patch series, no question. For a whole subsystem pull request, it's a
whole different scale. Maybe it scales for rather small trees though.

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