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

[PATCH v3] 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>'s backing storage is allocated through VecExt<T> extension
functions.

This only works as long as align_of::<T>(), used by Unique::dangling() to
derive the dangling pointer, resolves to a value 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, since there may be types whose
alignment may exceed the range still covered by krealloc(), plus other
kernel allocators are not as tolerant either.

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")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
Changes in v3:
  - correct description of which cases actually can cause the issue (Wedson)
  - use the old pointer to rebuild the old vector on allocation failure (Wedson)
  - add RB tag (Benno)

Note: I did not remove the "Fixes" tag, if you think it will be invalidated,
      feel free to remove it when applying the patch.

Changes in v2:
  - correct the description of the pointer value produced by
    Unique::dangling() (Alice)
  - add RB tags (Alice, Boqun)
---
 rust/kernel/alloc/vec_ext.rs | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
index 6a916fcf8bf1..2fefc5623455 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`].
@@ -135,14 +136,23 @@ fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>
         let new_cap = core::cmp::max(cap * 2, len.checked_add(additional).ok_or(AllocError)?);
         let layout = core::alloc::Layout::array::<T>(new_cap).map_err(|_| AllocError)?;
 
-        let (ptr, len, cap) = destructure(self);
+        let (old_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(),
+            _ => old_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) };
         if new_ptr.is_null() {
             // SAFETY: We are just rebuilding the existing `Vec` with no changes.
-            unsafe { rebuild(self, ptr, len, cap) };
+            unsafe { rebuild(self, old_ptr, len, cap) };
             Err(AllocError)
         } else {
             // SAFETY: `ptr` has been reallocated with the layout for `new_cap` elements. New cap

base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
-- 
2.44.0

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

[Wedson Almeida Filho]

On Wed, 1 May 2024 at 10:48, 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>'s backing storage is allocated through VecExt<T> extension
> functions.
>
> This only works as long as align_of::<T>(), used by Unique::dangling() to
> derive the dangling pointer, resolves to a value 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, since there may be types whose
> alignment may exceed the range still covered by krealloc(), plus other
> kernel allocators are not as tolerant either.
>
> 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")
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>

> +
> +        // 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(),
> +            _ => old_ptr,
> +        };

I still think this should be an `if`.

What happened to adding the sample/test? It should be straightforward
to do it...

Cheers,
-Wedson

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

[Danilo Krummrich]

On 5/6/24 16:11, Wedson Almeida Filho wrote:
> On Wed, 1 May 2024 at 10:48, 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>'s backing storage is allocated through VecExt<T> extension
>> functions.
>>
>> This only works as long as align_of::<T>(), used by Unique::dangling() to
>> derive the dangling pointer, resolves to a value 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, since there may be types whose
>> alignment may exceed the range still covered by krealloc(), plus other
>> kernel allocators are not as tolerant either.
>>
>> 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")
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> 
> Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>
> 
>> +
>> +        // 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(),
>> +            _ => old_ptr,
>> +        };
> 
> I still think this should be an `if`.

Is there any benefit using an if here, or is that your personal preference?

> 
> What happened to adding the sample/test? It should be straightforward
> to do it...

As mentioned, I can send a patch for that soon.

> 
> Cheers,
> -Wedson
> 

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

[Wedson Almeida Filho]

On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
>
> On 5/6/24 16:11, Wedson Almeida Filho wrote:
> > On Wed, 1 May 2024 at 10:48, 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>'s backing storage is allocated through VecExt<T> extension
> >> functions.
> >>
> >> This only works as long as align_of::<T>(), used by Unique::dangling() to
> >> derive the dangling pointer, resolves to a value 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, since there may be types whose
> >> alignment may exceed the range still covered by krealloc(), plus other
> >> kernel allocators are not as tolerant either.
> >>
> >> 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")
> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> >> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> >
> > Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> >> +
> >> +        // 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(),
> >> +            _ => old_ptr,
> >> +        };
> >
> > I still think this should be an `if`.
>
> Is there any benefit using an if here, or is that your personal preference?

Readability.

I don't remember ever seeing anyone implement a != 0 comparison with a
match or switch.

But if you insist on innovating on what seems poor style to me, go
ahead. I'll fix it later.

> >
> > What happened to adding the sample/test? It should be straightforward
> > to do it...
>
> As mentioned, I can send a patch for that soon.

Oh, I see. You intend to send this separately. I thought the patch you
had mentioned before was the v3 for this.

We don't normally send documentation examples separately. But I'm fine
either way.

Cheers,
-Wedson

some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[David Airlie]

> On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> > > On Wed, 1 May 2024 at 10:48, 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>'s backing storage is allocated through VecExt<T> extension
> > >> functions.
> > >>
> > >> This only works as long as align_of::<T>(), used by Unique::dangling() to
> > >> derive the dangling pointer, resolves to a value 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, since there may be types whose
> > >> alignment may exceed the range still covered by krealloc(), plus other
> > >> kernel allocators are not as tolerant either.
> > >>
> > >> 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")
> > >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > >> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > >> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > >> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > >
> > > Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > >
> > >> +
> > >> +        // 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(),
> > >> +            _ => old_ptr,
> > >> +        };
> > >
> > > I still think this should be an `if`.
> >
> > Is there any benefit using an if here, or is that your personal preference?
>
> Readability.
>
> I don't remember ever seeing anyone implement a != 0 comparison with a
> match or switch.
>
> But if you insist on innovating on what seems poor style to me, go
> ahead. I'll fix it later.

I'd like to just respond with a bit of maintainer experience here,
please try and use less suggestive language and more requirements
language in reviews if you want to see something change.

"I still think this should be an 'if'" allows for disagreement and
discussion, but clearly in some cases you want to directly inform the
patch submitter of some maintainer requirement, I consider stylistic
preference one of those cases.

If you are in the position as a code maintainer that you say "do
whatever, I'll just fix it up myself later", it comes across as
passive aggressive, whereas you could just have said that in the
initial review,  "change this to an if for consistent style" or
because of maintainer preference it leaves out any ambiguity that you
want to be see the change in order to merge it. In my experience most
contributors will just make changes and move on, and are happy with
less ambiguity.

I'd suggest if we wanted to establish conventions and rules around if
vs match we should hash it out on zulip and update some docs
somewhere, or we can just leave it as is and have maintainers state
their requirements to avoid ambiguity.

Dave.

Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[Wedson Almeida Filho]

On Mon, 6 May 2024 at 21:36, David Airlie <airlied@redhat.com> wrote:
>
> > On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> > >
> > > On 5/6/24 16:11, Wedson Almeida Filho wrote:
[snip]
> > > > I still think this should be an `if`.
> > >
> > > Is there any benefit using an if here, or is that your personal preference?
> >
> > Readability.
> >
> > I don't remember ever seeing anyone implement a != 0 comparison with a
> > match or switch.
> >
> > But if you insist on innovating on what seems poor style to me, go
> > ahead. I'll fix it later.
>
> I'd like to just respond with a bit of maintainer experience here,
> please try and use less suggestive language and more requirements
> language in reviews if you want to see something change.

Thanks for the advice.

I don't want to be intransigent though: if there's a real need for
this to be a 'match', I of course wouldn't mind it (though I don't
believe one exists).

> "I still think this should be an 'if'" allows for disagreement and
> discussion, but clearly in some cases you want to directly inform the
> patch submitter of some maintainer requirement, I consider stylistic
> preference one of those cases.

By any chance, have you seen the thread that precedes this comment (in
v2)? In case you haven't, check it out here:
https://lore.kernel.org/rust-for-linux/ZjFq1uVXi4k1jjQc@pollux/

Between sending this response and sending a v3 that completely ignores
the discussion, he waited a grand total of 16 hours.

> If you are in the position as a code maintainer that you say "do
> whatever, I'll just fix it up myself later", it comes across as
> passive aggressive, whereas you could just have said that in the
> initial review,  "change this to an if for consistent style" or
> because of maintainer preference it leaves out any ambiguity that you
> want to be see the change in order to merge it. In my experience most
> contributors will just make changes and move on, and are happy with
> less ambiguity.

The latest exchange didn't lead me to believe this contributor "would
just make changes and move on" (as you suggest above), so I had two
options: reject the patch or accept it and change the style later.
Since I obviously want the fix and for him to be properly credited (as
he deserves and insisted that it be by means of his own commit -- see
the discussion in v1, of which I wasn't part), I chose the second
option because Miguel and I want to finalize the changes for 6.10 now.

> I'd suggest if we wanted to establish conventions and rules around if
> vs match we should hash it out on zulip and update some docs
> somewhere, or we can just leave it as is and have maintainers state
> their requirements to avoid ambiguity.

Zulip? :)

Cheers,
-Wedson

Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[David Airlie]

On Tue, May 7, 2024 at 12:33 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> On Mon, 6 May 2024 at 21:36, David Airlie <airlied@redhat.com> wrote:
> >
> > > On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> > > >
> > > > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> [snip]
> > > > > I still think this should be an `if`.
> > > >
> > > > Is there any benefit using an if here, or is that your personal preference?
> > >
> > > Readability.
> > >
> > > I don't remember ever seeing anyone implement a != 0 comparison with a
> > > match or switch.
> > >
> > > But if you insist on innovating on what seems poor style to me, go
> > > ahead. I'll fix it later.
> >
> > I'd like to just respond with a bit of maintainer experience here,
> > please try and use less suggestive language and more requirements
> > language in reviews if you want to see something change.
>
> Thanks for the advice.
>
> I don't want to be intransigent though: if there's a real need for
> this to be a 'match', I of course wouldn't mind it (though I don't
> believe one exists).
>
> > "I still think this should be an 'if'" allows for disagreement and
> > discussion, but clearly in some cases you want to directly inform the
> > patch submitter of some maintainer requirement, I consider stylistic
> > preference one of those cases.
>
> By any chance, have you seen the thread that precedes this comment (in
> v2)? In case you haven't, check it out here:
> https://lore.kernel.org/rust-for-linux/ZjFq1uVXi4k1jjQc@pollux/
>
> Between sending this response and sending a v3 that completely ignores
> the discussion, he waited a grand total of 16 hours.

I think sending a v4 would not be a problem in the same timeframe
though, even for a one line change like that.

You used nit: prefix and asked a question, answering that question was
the objective of the question? if changing the patch was the objective
"nit: please change this match to an if" would prove a better idea,
asking a submitter a question and them replying while sending a v3
doesn't mean they ignored your questions, it just means they are
wanting more information before sending v4. Review questions on an
older version of the patch can still remain open while a new version
exists.

There is nothing against sending updated patches as quickly as
possible, and adding feedback from open questions in another round,
otherwise patch series could be stalled on "nit" for a long time.

> The latest exchange didn't lead me to believe this contributor "would
> just make changes and move on" (as you suggest above), so I had two
> options: reject the patch or accept it and change the style later.

You can just ask for a v4, turnaround time for patch generation
shouldn't be taking a long time here, if v3 was 16 hours why do you
think a v4 would have stalled out the process for long?

> Since I obviously want the fix and for him to be properly credited (as
> he deserves and insisted that it be by means of his own commit -- see
> the discussion in v1, of which I wasn't part), I chose the second
> option because Miguel and I want to finalize the changes for 6.10 now.
>
> > I'd suggest if we wanted to establish conventions and rules around if
> > vs match we should hash it out on zulip and update some docs
> > somewhere, or we can just leave it as is and have maintainers state
> > their requirements to avoid ambiguity.
>
> Zulip? :)

or in the RFL call, but somewhere outside the flow of a patch merge
that we've admitted has some time critical pieces to it.

Dave.

Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[Wedson Almeida Filho]

On Mon, 6 May 2024 at 23:47, David Airlie <airlied@redhat.com> wrote:
>
> I think sending a v4 would not be a problem in the same timeframe
> though, even for a one line change like that.
>
> You used nit: prefix and asked a question, answering that question was
> the objective of the question? if changing the patch was the objective
> "nit: please change this match to an if" would prove a better idea,
> asking a submitter a question and them replying while sending a v3
> doesn't mean they ignored your questions, it just means they are
> wanting more information before sending v4. Review questions on an
> older version of the patch can still remain open while a new version
> exists.
>
> There is nothing against sending updated patches as quickly as
> possible, and adding feedback from open questions in another round,
> otherwise patch series could be stalled on "nit" for a long time.

I'm not sure I agree with this. Once a new version is out, we migrate
to that new version.

> > The latest exchange didn't lead me to believe this contributor "would
> > just make changes and move on" (as you suggest above), so I had two
> > options: reject the patch or accept it and change the style later.
>
> You can just ask for a v4, turnaround time for patch generation
> shouldn't be taking a long time here, if v3 was 16 hours why do you
> think a v4 would have stalled out the process for long?

I can't demand, expect, nor be certain that contributors will turn
around quickly.

But let's try it on both counts. I'll respond to v2 and request a new patch.

> > Since I obviously want the fix and for him to be properly credited (as
> > he deserves and insisted that it be by means of his own commit -- see
> > the discussion in v1, of which I wasn't part), I chose the second
> > option because Miguel and I want to finalize the changes for 6.10 now.
> >
> > > I'd suggest if we wanted to establish conventions and rules around if
> > > vs match we should hash it out on zulip and update some docs
> > > somewhere, or we can just leave it as is and have maintainers state
> > > their requirements to avoid ambiguity.
> >
> > Zulip? :)
>
> or in the RFL call, but somewhere outside the flow of a patch merge
> that we've admitted has some time critical pieces to it.
>
> Dave.
>

Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[David Airlie]

On Tue, May 7, 2024 at 12:33 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> On Mon, 6 May 2024 at 21:36, David Airlie <airlied@redhat.com> wrote:
> >
> > > On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> > > >
> > > > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> [snip]
> > > > > I still think this should be an `if`.
> > > >
> > > > Is there any benefit using an if here, or is that your personal preference?
> > >
> > > Readability.
> > >
> > > I don't remember ever seeing anyone implement a != 0 comparison with a
> > > match or switch.
> > >
> > > But if you insist on innovating on what seems poor style to me, go
> > > ahead. I'll fix it later.
> >
> > I'd like to just respond with a bit of maintainer experience here,
> > please try and use less suggestive language and more requirements
> > language in reviews if you want to see something change.
>
> Thanks for the advice.
>
> I don't want to be intransigent though: if there's a real need for
> this to be a 'match', I of course wouldn't mind it (though I don't
> believe one exists).
>
> > "I still think this should be an 'if'" allows for disagreement and
> > discussion, but clearly in some cases you want to directly inform the
> > patch submitter of some maintainer requirement, I consider stylistic
> > preference one of those cases.
>
> By any chance, have you seen the thread that precedes this comment (in
> v2)? In case you haven't, check it out here:
> https://lore.kernel.org/rust-for-linux/ZjFq1uVXi4k1jjQc@pollux/

So the same problem exists also in this thread, there is nothing
actionable for the submitter. There are suggestions of a discussion
being useful, but no actual do this to get this patch accepted
clarity. If you want a change to be made, be direct and state it
otherwise people will not take action if further discussion is needed.

Dave.

Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[Danilo Krummrich]

On Mon, May 06, 2024 at 11:33:31PM -0300, Wedson Almeida Filho wrote:
> On Mon, 6 May 2024 at 21:36, David Airlie <airlied@redhat.com> wrote:
> >
> > > On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> > > >
> > > > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> [snip]
> > > > > I still think this should be an `if`.
> > > >
> > > > Is there any benefit using an if here, or is that your personal preference?
> > >
> > > Readability.
> > >
> > > I don't remember ever seeing anyone implement a != 0 comparison with a
> > > match or switch.
> > >
> > > But if you insist on innovating on what seems poor style to me, go
> > > ahead. I'll fix it later.
> >
> > I'd like to just respond with a bit of maintainer experience here,
> > please try and use less suggestive language and more requirements
> > language in reviews if you want to see something change.
> 
> Thanks for the advice.
> 
> I don't want to be intransigent though: if there's a real need for
> this to be a 'match', I of course wouldn't mind it (though I don't
> believe one exists).
> 
> > "I still think this should be an 'if'" allows for disagreement and
> > discussion, but clearly in some cases you want to directly inform the
> > patch submitter of some maintainer requirement, I consider stylistic
> > preference one of those cases.
> 
> By any chance, have you seen the thread that precedes this comment (in
> v2)? In case you haven't, check it out here:
> https://lore.kernel.org/rust-for-linux/ZjFq1uVXi4k1jjQc@pollux/
> 
> Between sending this response and sending a v3 that completely ignores
> the discussion, he waited a grand total of 16 hours.
> 
> > If you are in the position as a code maintainer that you say "do
> > whatever, I'll just fix it up myself later", it comes across as
> > passive aggressive, whereas you could just have said that in the
> > initial review,  "change this to an if for consistent style" or
> > because of maintainer preference it leaves out any ambiguity that you
> > want to be see the change in order to merge it. In my experience most
> > contributors will just make changes and move on, and are happy with
> > less ambiguity.
> 
> The latest exchange didn't lead me to believe this contributor "would

While I also don't agree with some other points in this context, I only want to
focus on the following two aspects.

I don't appreciate being referred to as "this contributor". If you want to refer
to me, please do so by using my name. Doing otherwise feels like a gesture of
disrespect to me and hence doesn't provide a foundation for working together.

> just make changes and move on" (as you suggest above), so I had two
> options: reject the patch or accept it and change the style later.
> Since I obviously want the fix and for him to be properly credited (as
> he deserves and insisted that it be by means of his own commit -- see

I also don't appreciate spreading false information here.

Please note that I was only sharing that my experience has been that people
typically do care whether their own patch or the code of their patch squashed
into an existing one is landing.

I did not say that I personally insist on the former. I even explicitly said the
opposite:

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

[1] https://lore.kernel.org/rust-for-linux/ZjEfW6QpErDDnntk@pollux/

> the discussion in v1, of which I wasn't part), I chose the second
> option because Miguel and I want to finalize the changes for 6.10 now.
> 
> > I'd suggest if we wanted to establish conventions and rules around if
> > vs match we should hash it out on zulip and update some docs
> > somewhere, or we can just leave it as is and have maintainers state
> > their requirements to avoid ambiguity.
> 
> Zulip? :)
> 
> Cheers,
> -Wedson
> 

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

[Miguel Ojeda]

On Mon, May 6, 2024 at 5:22 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Is there any benefit using an if here, or is that your personal preference?

`if` is meant to test a boolean condition, which is what you are doing
here, so the question is why would we want to use a `match` here -- we
should first of all try to be consistent.

Now, I can understand the appeal of how the `match` looks like (e.g.
less braces, less zig-zag indentation), and if this were a `if`-`else`
chain with several cases and the wildcard `_` was not used, e.g.
something like:

    ... = match n {
        1 => ...
        2 => ...
        3 => ...
    }

then I agree it would be clearly better.

Perhaps you are arguing for something like "let's always use `match`
in cases where a value is returned, i.e. when used as an expression,
and save `if` for the C-like cases". If so, would that mean then that
we need to do:

    match c {
        false => ...
        true => ...
    }

to be consistent?

Then there are other things like `if let`, the potential `is` operator
and postfix `match` that should probably be considered.

Anyway, I am picking the fix as it is.

Cheers,
Miguel

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

[Danilo Krummrich]

On 5/6/24 19:50, Miguel Ojeda wrote:
> On Mon, May 6, 2024 at 5:22 PM Danilo Krummrich <dakr@redhat.com> wrote:
>>
>> Is there any benefit using an if here, or is that your personal preference?
> 
> `if` is meant to test a boolean condition, which is what you are doing
> here, so the question is why would we want to use a `match` here -- we
> should first of all try to be consistent.
> 
> Now, I can understand the appeal of how the `match` looks like (e.g.
> less braces, less zig-zag indentation), and if this were a `if`-`else`
> chain with several cases and the wildcard `_` was not used, e.g.
> something like:
> 
>      ... = match n {
>          1 => ...
>          2 => ...
>          3 => ...
>      }
> 
> then I agree it would be clearly better.
> 
> Perhaps you are arguing for something like "let's always use `match`
> in cases where a value is returned, i.e. when used as an expression,
> and save `if` for the C-like cases". If so, would that mean then that
> we need to do:
> 
>      match c {
>          false => ...
>          true => ...
>      }
> 
> to be consistent?
> 
> Then there are other things like `if let`, the potential `is` operator
> and postfix `match` that should probably be considered.
> 

Thanks for your thoughts, but style wise I don't have a strong preference
for either of those. My question was honestly about whether there is any
other reason other than style.

However, I don't think using match is that unreasonable. There are cases
in the std library as well, e.g. [1]. But again, I'm fine with either of
those.

> Anyway, I am picking the fix as it is.

Please feel free to change the match statement to an if statement if that's
what you prefer.

- Danilo

[1] https://doc.rust-lang.org/src/alloc/string.rs.html#1358

> 
> Cheers,
> Miguel
> 

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

[Miguel Ojeda]

On Wed, May 1, 2024 at 3:48 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>'s backing storage is allocated through VecExt<T> extension
> functions.
>
> This only works as long as align_of::<T>(), used by Unique::dangling() to
> derive the dangling pointer, resolves to a value 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, since there may be types whose
> alignment may exceed the range still covered by krealloc(), plus other
> kernel allocators are not as tolerant either.
>
> 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")
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

Applied to `rust-next` -- thanks everyone!

Cheers,
Miguel