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

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

[Danilo Krummrich]

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.

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")
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc/vec_ext.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
index 6a916fcf8bf1..ffcf8a19f715 100644
--- a/rust/kernel/alloc/vec_ext.rs
+++ b/rust/kernel/alloc/vec_ext.rs
@@ -4,6 +4,7 @@
 
 use super::{AllocError, Flags};
 use alloc::vec::Vec;
+use core::ptr;
 use core::result::Result;
 
 /// Extensions to [`Vec`].
@@ -137,6 +138,15 @@ fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>
 
         let (ptr, len, cap) = destructure(self);
 
+        // We need to make sure that ptr is either NULL or comes from a previous call to
+        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
+        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>'s capacity
+        // to be zero if no memory has been allocated yet.
+        let ptr = match cap {
+            0 => ptr::null_mut(),
+            _ => ptr,
+        };
+
         // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
         // `krealloc_aligned`. We also verified that the type is not a ZST.
         let new_ptr = unsafe { super::allocator::krealloc_aligned(ptr.cast(), layout, flags) };

base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
-- 
2.44.0

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

[Boqun Feng]

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.

Either way:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/alloc/vec_ext.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
> index 6a916fcf8bf1..ffcf8a19f715 100644
> --- a/rust/kernel/alloc/vec_ext.rs
> +++ b/rust/kernel/alloc/vec_ext.rs
> @@ -4,6 +4,7 @@
>  
>  use super::{AllocError, Flags};
>  use alloc::vec::Vec;
> +use core::ptr;
>  use core::result::Result;
>  
>  /// Extensions to [`Vec`].
> @@ -137,6 +138,15 @@ fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>
>  
>          let (ptr, len, cap) = destructure(self);
>  
> +        // We need to make sure that ptr is either NULL or comes from a previous call to
> +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
> +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>'s capacity
> +        // to be zero if no memory has been allocated yet.
> +        let ptr = match cap {
> +            0 => ptr::null_mut(),
> +            _ => ptr,
> +        };
> +
>          // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
>          // `krealloc_aligned`. We also verified that the type is not a ZST.
>          let new_ptr = unsafe { super::allocator::krealloc_aligned(ptr.cast(), layout, flags) };
> 
> base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
> -- 
> 2.44.0
> 

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

[Danilo Krummrich]

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
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,
it's usually not providing a great motivation for potential contributors.

- Danilo

> 
> Either way:
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Regards,
> Boqun
> 
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>> ---
>>   rust/kernel/alloc/vec_ext.rs | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
>> index 6a916fcf8bf1..ffcf8a19f715 100644
>> --- a/rust/kernel/alloc/vec_ext.rs
>> +++ b/rust/kernel/alloc/vec_ext.rs
>> @@ -4,6 +4,7 @@
>>   
>>   use super::{AllocError, Flags};
>>   use alloc::vec::Vec;
>> +use core::ptr;
>>   use core::result::Result;
>>   
>>   /// Extensions to [`Vec`].
>> @@ -137,6 +138,15 @@ fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>
>>   
>>           let (ptr, len, cap) = destructure(self);
>>   
>> +        // We need to make sure that ptr is either NULL or comes from a previous call to
>> +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
>> +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>'s capacity
>> +        // to be zero if no memory has been allocated yet.
>> +        let ptr = match cap {
>> +            0 => ptr::null_mut(),
>> +            _ => ptr,
>> +        };
>> +
>>           // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
>>           // `krealloc_aligned`. We also verified that the type is not a ZST.
>>           let new_ptr = unsafe { super::allocator::krealloc_aligned(ptr.cast(), layout, flags) };
>>
>> base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
>> -- 
>> 2.44.0
>>
> 

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

[Boqun Feng]

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

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

[Danilo Krummrich]

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

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

[Boqun Feng]

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

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

[Danilo Krummrich]

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
> 

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

[Boqun Feng]

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

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

[Boqun Feng]

On Tue, Apr 30, 2024 at 01:59:19PM -0700, Boqun Feng wrote:
> 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.
> 

nor it will be the base of the final pull request.

In short words, -next branches are subject to rebase for various
reasons. Commit id from them is not stable, period.

Regards,
Boqun

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

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

[Danilo Krummrich]

On Tue, Apr 30, 2024 at 02:08:31PM -0700, Boqun Feng wrote:
> On Tue, Apr 30, 2024 at 01:59:19PM -0700, Boqun Feng wrote:
> > 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.
> > 

I meant -next branches in general. As I understood you previously I thought
you're not sure whether rust-next is prone to altering its history. Now, you're
clearly saying it is - noted.

> 
> nor it will be the base of the final pull request.
> 
> In short words, -next branches are subject to rebase for various
> reasons. Commit id from them is not stable, period.

As you just educated me, for rust-next that seems to be case - again, good to
know. Generally, I don't think that's commonly the case though, hence my
surprise.

This throws up a new questions though. Does this mean you'll actually just
squash fixes into commits that are in rust-next only?

> 
> Regards,
> Boqun
> 
> > > > 
> > > > 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
> 

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

[Boqun Feng]

On Wed, May 01, 2024 at 12:19:17AM +0200, Danilo Krummrich wrote:
> On Tue, Apr 30, 2024 at 02:08:31PM -0700, Boqun Feng wrote:
> > On Tue, Apr 30, 2024 at 01:59:19PM -0700, Boqun Feng wrote:
> > > 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.
> > > 
> 
> I meant -next branches in general. As I understood you previously I thought
> you're not sure whether rust-next is prone to altering its history. Now, you're
> clearly saying it is - noted.
> 

Yeah, I wasn't so sure myself, so I digged into the history. My
understanding is that maintainers are usually allowed to alter the
history, and most subsystems I work on do alter the histories from time
to time.

// It's a curse of the knowledge, once I see the power of rewriting
// history, I cannot unsee it ;-)

> > 
> > nor it will be the base of the final pull request.
> > 
> > In short words, -next branches are subject to rebase for various
> > reasons. Commit id from them is not stable, period.
> 
> As you just educated me, for rust-next that seems to be case - again, good to
> know. Generally, I don't think that's commonly the case though, hence my
> surprise.
> 
> This throws up a new questions though. Does this mean you'll actually just
> squash fixes into commits that are in rust-next only?
> 

Again it's up to Miguel. For this particular case, in the common case of
history-rewriting-allowed subsystem, folding your patch in (with proper
update to commit log and SoB) is an option. Taking it as it is is also 
an option (maybe drop the "Fixes" tag), since although it's a bug, no
in-kernel users really get affected(?). If it's not -rc6 already, I
would definitely lean towards folding. That's why I said I'm OK either
way.

Regards,
Boqun

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

[Boqun Feng]

On Tue, Apr 30, 2024 at 10:46:52PM +0200, Danilo Krummrich wrote:
[...]
> > 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.
> 

You're not wrong ;-) It heavily depends on the scale of the subsystem. I
do see the point (and examples) that subsystems with larger scales
"Fixes" their own unmerged commits (e.g. in drm or netdev). But Rust
tree may stay relatively small, so rewriting histories makes sense to
me.

Regards,
Boqun

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

[Miguel Ojeda]

On Mon, Apr 29, 2024 at 11:01 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> I'd be surprised if rust-next wouldn't be fast-forward only, is it? If

Like most branches, it is generally not rebased, but it can be if
there is a good reason for it, i.e. there is no hard guarantee, and
rebases indeed happen sometimes, and they are allowed for branches in
linux-next (again, when deemed necessary).

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

It is a judgment call. It depends on how serious the mistake is and
how costly rebasing is, which in turn depends on the branch in
question, its state, its expected users, the testing time one would
lose, etc.

In this case, to be practical: does anybody think that the risk of
getting bisected here and hitting the issue (especially with the code
we have so far in-tree, i.e. the `T`s that would apply here) is high?

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

Credit can (and should) still be given even in that case.

But, yes, for e.g. a new contributor to the kernel looking for their
first actual commit, it could be a big deal.

Cheers,
Miguel

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

[Alice Ryhl]

On Mon, Apr 29, 2024 at 9:24 PM Danilo Krummrich <dakr@redhat.com> 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.

Good catch!

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

This isn't quite true. The value of Unique::dangling() is actually
align_of::<T>() rather than 0x1. So for types that have an alignment
of 0x20 or greater, it would not work today.

> 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")
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

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

[Danilo Krummrich]

On Tue, Apr 30, 2024 at 10:25:07AM +0200, Alice Ryhl wrote:
> On Mon, Apr 29, 2024 at 9:24 PM Danilo Krummrich <dakr@redhat.com> 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.
> 
> Good catch!
> 
> > 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.
> 
> This isn't quite true. The value of Unique::dangling() is actually
> align_of::<T>() rather than 0x1. So for types that have an alignment
> of 0x20 or greater, it would not work today.

Oh, indeed - gonna send a v2 fixing the commit message.

Thanks,
Danilo

> 
> > 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")
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>