Rust for Linux List
 help / color / mirror / Atom feed
* [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
@ 2026-08-31 17:09 Sophon Zhang via B4 Relay
  2026-09-01 10:58 ` Alexandre Courbot
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Sophon Zhang via B4 Relay @ 2026-08-31 17:09 UTC (permalink / raw)
  To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
	Onur Özkan
  Cc: linux-pci, rust-for-linux, linux-kernel, Sophon Zhang

From: Sophon Zhang <aiqubits@hotmail.com>

IrqVectorRegistration::index() accepts a usize, but pci_irq_vector()
takes an unsigned int. On 64-bit architectures, casting an index larger
than u32::MAX wraps it before the PCI core can validate it. In
particular, u32::MAX + 1 becomes zero and can resolve to the first
allocated vector.

Use a checked conversion and return EINVAL when the index cannot be
represented by the C API.

Fixes: 2fb7755b0a7e ("rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector")
Signed-off-by: Sophon Zhang <aiqubits@hotmail.com>
---
Prevent 64-bit Rust IRQ vector indices from wrapping when they cross the
PCI C API boundary.
---
Changes in v4:
- Drop the explicit length check in favor of PCI core range validation.
- Use the existing TryFromIntError-to-Error conversion directly.
- Keep commit trailers adjacent and narrow the description to truncation.
- Link to v3: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v3-1-a2103084d20e@hotmail.com

Changes in v3:
- Check the index against the allocated vector count before entering the C API.
- Keep the checked usize-to-u32 conversion and document the C-side warning.
- Link to v2: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v2-1-4030ea7746a9@hotmail.com

Changes in v2:
- No code changes.
- Link to v1: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v1-1-d63217d99b67@hotmail.com

Testing:
- make rustfmtcheck
- Not build- or hardware-tested; bindgen is unavailable in the test environment.
---
 rust/kernel/pci/irq.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
index 6741046ec1c0..22e2cdf82a21 100644
--- a/rust/kernel/pci/irq.rs
+++ b/rust/kernel/pci/irq.rs
@@ -151,8 +151,10 @@ pub fn irq_type(&self) -> IrqType {
     /// [`Self::len()`].
     #[inline]
     pub fn index(&self, index: usize) -> Result<IrqVector<'_>> {
+        let index = u32::try_from(index)?;
+
         // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
-        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index as u32) };
+        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index) };
         if irq < 0 {
             return Err(Error::from_errno(irq));
         }

---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260831-fix-pci-irq-vector-index-truncation-6752f3751a0d

Best regards,
--  
Sophon Zhang <aiqubits@hotmail.com>



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-08-31 17:09 [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32 Sophon Zhang via B4 Relay
@ 2026-09-01 10:58 ` Alexandre Courbot
  2026-09-01 11:08   ` Danilo Krummrich
                     ` (2 more replies)
  2026-09-01 16:19 ` Gary Guo
  2026-09-01 16:31 ` Danilo Krummrich
  2 siblings, 3 replies; 15+ messages in thread
From: Alexandre Courbot @ 2026-09-01 10:58 UTC (permalink / raw)
  To: Sophon Zhang via B4 Relay
  Cc: aiqubits, Danilo Krummrich, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 2:09 AM JST, Sophon Zhang via B4 Relay wrote:
> From: Sophon Zhang <aiqubits@hotmail.com>
>
> IrqVectorRegistration::index() accepts a usize, but pci_irq_vector()
> takes an unsigned int. On 64-bit architectures, casting an index larger
> than u32::MAX wraps it before the PCI core can validate it. In
> particular, u32::MAX + 1 becomes zero and can resolve to the first
> allocated vector.
>
> Use a checked conversion and return EINVAL when the index cannot be
> represented by the C API.
>
> Fixes: 2fb7755b0a7e ("rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector")
> Signed-off-by: Sophon Zhang <aiqubits@hotmail.com>
> ---
> Prevent 64-bit Rust IRQ vector indices from wrapping when they cross the
> PCI C API boundary.
> ---
> Changes in v4:
> - Drop the explicit length check in favor of PCI core range validation.
> - Use the existing TryFromIntError-to-Error conversion directly.
> - Keep commit trailers adjacent and narrow the description to truncation.
> - Link to v3: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v3-1-a2103084d20e@hotmail.com
>
> Changes in v3:
> - Check the index against the allocated vector count before entering the C API.
> - Keep the checked usize-to-u32 conversion and document the C-side warning.
> - Link to v2: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v2-1-4030ea7746a9@hotmail.com
>
> Changes in v2:
> - No code changes.
> - Link to v1: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v1-1-d63217d99b67@hotmail.com
>
> Testing:
> - make rustfmtcheck
> - Not build- or hardware-tested; bindgen is unavailable in the test environment.
> ---
>  rust/kernel/pci/irq.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
> index 6741046ec1c0..22e2cdf82a21 100644
> --- a/rust/kernel/pci/irq.rs
> +++ b/rust/kernel/pci/irq.rs
> @@ -151,8 +151,10 @@ pub fn irq_type(&self) -> IrqType {
>      /// [`Self::len()`].
>      #[inline]
>      pub fn index(&self, index: usize) -> Result<IrqVector<'_>> {
> +        let index = u32::try_from(index)?;
> +
>          // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index as u32) };
> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index) };
>          if irq < 0 {
>              return Err(Error::from_errno(irq));
>          }

That makes me wonder, shouldn't we make `index` take a `u32` directly?
If that's what the C API expects, it does make sense to align to it
instead of forcing users to make a potential unneeded conversion if they
already have a u32.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 10:58 ` Alexandre Courbot
@ 2026-09-01 11:08   ` Danilo Krummrich
  2026-09-01 13:32     ` Alexandre Courbot
  2026-09-01 11:47   ` ai qubits
  2026-09-01 12:42   ` Miguel Ojeda
  2 siblings, 1 reply; 15+ messages in thread
From: Danilo Krummrich @ 2026-09-01 11:08 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Sophon Zhang via B4 Relay, aiqubits, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
> That makes me wonder, shouldn't we make `index` take a `u32` directly?
> If that's what the C API expects, it does make sense to align to it
> instead of forcing users to make a potential unneeded conversion if they
> already have a u32.

I intentionally did not do this, as the common type for an index is usize. Thus,
I do not expect anyone to already have a u32, but to already have a usize, e.g.
from some iterator.

The fact that the C API did pick unsigned int as index type is an implementation
detail the abstraction should bother with.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* 回复: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 10:58 ` Alexandre Courbot
  2026-09-01 11:08   ` Danilo Krummrich
@ 2026-09-01 11:47   ` ai qubits
  2026-09-01 12:06     ` Gary Guo
  2026-09-01 12:42   ` Miguel Ojeda
  2 siblings, 1 reply; 15+ messages in thread
From: ai qubits @ 2026-09-01 11:47 UTC (permalink / raw)
  To: Alexandre Courbot, Sophon Zhang via B4 Relay
  Cc: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org

Thanks for the question, Alexandre, and for the clarification, Danilo.

I agree that usize is the more appropriate type for an index-oriented
Rust API. Callers are likely to obtain indices from ranges, iterators, or
other Rust indexing operations, which naturally use usize.

The unsigned int expected by pci_irq_vector() is an implementation detail
of the C interface, so the necessary validation and conversion should
remain inside the Rust abstraction rather than being exposed to callers.

I will therefore keep IrqVectorRegistration::index() accepting usize and
handle the C API boundary internally.

Best regards,
Sophon

________________________________________
发件人: Alexandre Courbot <acourbot@nvidia.com>
发送时间: 2026年9月1日 18:58
收件人: Sophon Zhang via B4 Relay
抄送: aiqubits@hotmail.com; Danilo Krummrich; Bjorn Helgaas; Krzysztof Wilczyński; Miguel Ojeda; Boqun Feng; Gary Guo; Björn Roy Baron; Benno Lossin; Andreas Hindborg; Alice Ryhl; Trevor Gross; Daniel Almeida; Tamir Duberstein; Onur Özkan; linux-pci@vger.kernel.org; rust-for-linux@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32

On Tue Sep 1, 2026 at 2:09 AM JST, Sophon Zhang via B4 Relay wrote:
> From: Sophon Zhang <aiqubits@hotmail.com>
>
> IrqVectorRegistration::index() accepts a usize, but pci_irq_vector()
> takes an unsigned int. On 64-bit architectures, casting an index larger
> than u32::MAX wraps it before the PCI core can validate it. In
> particular, u32::MAX + 1 becomes zero and can resolve to the first
> allocated vector.
>
> Use a checked conversion and return EINVAL when the index cannot be
> represented by the C API.
>
> Fixes: 2fb7755b0a7e ("rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector")
> Signed-off-by: Sophon Zhang <aiqubits@hotmail.com>
> ---
> Prevent 64-bit Rust IRQ vector indices from wrapping when they cross the
> PCI C API boundary.
> ---
> Changes in v4:
> - Drop the explicit length check in favor of PCI core range validation.
> - Use the existing TryFromIntError-to-Error conversion directly.
> - Keep commit trailers adjacent and narrow the description to truncation.
> - Link to v3: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v3-1-a2103084d20e@hotmail.com
>
> Changes in v3:
> - Check the index against the allocated vector count before entering the C API.
> - Keep the checked usize-to-u32 conversion and document the C-side warning.
> - Link to v2: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v2-1-4030ea7746a9@hotmail.com
>
> Changes in v2:
> - No code changes.
> - Link to v1: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v1-1-d63217d99b67@hotmail.com
>
> Testing:
> - make rustfmtcheck
> - Not build- or hardware-tested; bindgen is unavailable in the test environment.
> ---
>  rust/kernel/pci/irq.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
> index 6741046ec1c0..22e2cdf82a21 100644
> --- a/rust/kernel/pci/irq.rs
> +++ b/rust/kernel/pci/irq.rs
> @@ -151,8 +151,10 @@ pub fn irq_type(&self) -> IrqType {
>      /// [`Self::len()`].
>      #[inline]
>      pub fn index(&self, index: usize) -> Result<IrqVector<'_>> {
> +        let index = u32::try_from(index)?;
> +
>          // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index as u32) };
> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index) };
>          if irq < 0 {
>              return Err(Error::from_errno(irq));
>          }

That makes me wonder, shouldn't we make `index` take a `u32` directly?
If that's what the C API expects, it does make sense to align to it
instead of forcing users to make a potential unneeded conversion if they
already have a u32.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: 回复: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 11:47   ` ai qubits
@ 2026-09-01 12:06     ` Gary Guo
  2026-09-01 12:35       ` 回复: " ai qubits
  0 siblings, 1 reply; 15+ messages in thread
From: Gary Guo @ 2026-09-01 12:06 UTC (permalink / raw)
  To: ai qubits, Alexandre Courbot, Sophon Zhang via B4 Relay
  Cc: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org

On Tue Sep 1, 2026 at 12:47 PM BST, ai qubits wrote:
> Thanks for the question, Alexandre, and for the clarification, Danilo.
> 
> I agree that usize is the more appropriate type for an index-oriented
> Rust API. Callers are likely to obtain indices from ranges, iterators, or
> other Rust indexing operations, which naturally use usize.
> 
> The unsigned int expected by pci_irq_vector() is an implementation detail
> of the C interface, so the necessary validation and conversion should
> remain inside the Rust abstraction rather than being exposed to callers.
> 
> I will therefore keep IrqVectorRegistration::index() accepting usize and
> handle the C API boundary internally.
> 
> Best regards,
> Sophon

Hi Sophon,

Please avoid top-posting. See
https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying (and
rest of that page, too).

Also, your email contains byte order marks (FEFF). Not sure what clients are you
using, but please pick a proper one for plain text mails.

Thanks,
Gary


^ permalink raw reply	[flat|nested] 15+ messages in thread

* 回复: 回复: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 12:06     ` Gary Guo
@ 2026-09-01 12:35       ` ai qubits
  0 siblings, 0 replies; 15+ messages in thread
From: ai qubits @ 2026-09-01 12:35 UTC (permalink / raw)
  To: Gary Guo, Sophon Zhang via B4 Relay
  Cc: Alexandre Courbot, Danilo Krummrich, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org

Thanks for pointing this out, Gary. I will optimize these email responses in future emails.

Best regards,
Sophon

________________________________________
发件人: Gary Guo <gary@garyguo.net>
发送时间: 2026年9月1日 20:06
收件人: ai qubits; Alexandre Courbot; Sophon Zhang via B4 Relay
抄送: Danilo Krummrich; Bjorn Helgaas; Krzysztof Wilczyński; Miguel Ojeda; Boqun Feng; Gary Guo; Björn Roy Baron; Benno Lossin; Andreas Hindborg; Alice Ryhl; Trevor Gross; Daniel Almeida; Tamir Duberstein; Onur Özkan; linux-pci@vger.kernel.org; rust-for-linux@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: 回复: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32

On Tue Sep 1, 2026 at 12:47 PM BST, ai qubits wrote:
> Thanks for the question, Alexandre, and for the clarification, Danilo.
> 
> I agree that usize is the more appropriate type for an index-oriented
> Rust API. Callers are likely to obtain indices from ranges, iterators, or
> other Rust indexing operations, which naturally use usize.
> 
> The unsigned int expected by pci_irq_vector() is an implementation detail
> of the C interface, so the necessary validation and conversion should
> remain inside the Rust abstraction rather than being exposed to callers.
> 
> I will therefore keep IrqVectorRegistration::index() accepting usize and
> handle the C API boundary internally.
> 
> Best regards,
> Sophon

Hi Sophon,

Please avoid top-posting. See
https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying (and
rest of that page, too).

Also, your email contains byte order marks (FEFF). Not sure what clients are you
using, but please pick a proper one for plain text mails.

Thanks,
Gary


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 10:58 ` Alexandre Courbot
  2026-09-01 11:08   ` Danilo Krummrich
  2026-09-01 11:47   ` ai qubits
@ 2026-09-01 12:42   ` Miguel Ojeda
  2 siblings, 0 replies; 15+ messages in thread
From: Miguel Ojeda @ 2026-09-01 12:42 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Sophon Zhang via B4 Relay, aiqubits, Danilo Krummrich,
	Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida,
	Tamir Duberstein, Onur Özkan, linux-pci, rust-for-linux,
	linux-kernel

On Tue, Sep 1, 2026 at 12:58 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> That makes me wonder, shouldn't we make `index` take a `u32` directly?
> If that's what the C API expects, it does make sense to align to it
> instead of forcing users to make a potential unneeded conversion if they
> already have a u32.

In general, Rust users shouldn't be dealing with C APIs, so if they
have a `u32` then it usually is because we exposed it from somewhere
else (e.g. we returned it to them) or because that is generally the
right underlying type, in which case it may make sense to align
everything.

But even in those cases, it may have made sense to define a Rust
newtype or similar instead.

So other than exceptional cases, the types that C APIs use shouldn't
drive the decisions on the Rust signatures, since they shouldn't be
seen by the Rust users to begin with.

That definitely introduces some friction on our side, but it does give
us a lot of freedom defining APIs the best way we can, which is a
major advantage, i.e. since we have to provide these abstractions, it
is a good time to be able to clean old decisions and improve on them
using whatever tools Rust give us.

Otherwise, one could also argue we should be passing the underlying
types in general, e.g. even pointers.

(Of course, you know this, I am just elaborating; and obviously there
may be cases it may make sense to just use the underlying type since
it is something that has never changed or is fixed due to some
"standard" etc.).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 11:08   ` Danilo Krummrich
@ 2026-09-01 13:32     ` Alexandre Courbot
  2026-09-01 13:36       ` Danilo Krummrich
  2026-09-01 13:48       ` Gary Guo
  0 siblings, 2 replies; 15+ messages in thread
From: Alexandre Courbot @ 2026-09-01 13:32 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Sophon Zhang via B4 Relay, aiqubits, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>> If that's what the C API expects, it does make sense to align to it
>> instead of forcing users to make a potential unneeded conversion if they
>> already have a u32.
>
> I intentionally did not do this, as the common type for an index is usize. Thus,
> I do not expect anyone to already have a u32, but to already have a usize, e.g.
> from some iterator.
>
> The fact that the C API did pick unsigned int as index type is an implementation
> detail the abstraction should bother with.

Thanks for the clarification (and Miguel for elaborating - I wasn't
completely aware of it!). In that case this patch looks correct to me.

Note that there is another `as` right after, in the same method:

  if irq < 0 {
      return Err(Error::from_errno(irq));
  }

  // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
  Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })


We could get rid of it by replacing the `if irq < 0` test with:

  let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;

Sophon, if you feel like doing it I think this could improve the patch
further.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 13:32     ` Alexandre Courbot
@ 2026-09-01 13:36       ` Danilo Krummrich
  2026-09-01 13:52         ` Alexandre Courbot
  2026-09-01 13:48       ` Gary Guo
  1 sibling, 1 reply; 15+ messages in thread
From: Danilo Krummrich @ 2026-09-01 13:36 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Sophon Zhang via B4 Relay, aiqubits, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On 9/1/26 3:32 PM, Alexandre Courbot wrote:
> On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
>> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>>> If that's what the C API expects, it does make sense to align to it
>>> instead of forcing users to make a potential unneeded conversion if they
>>> already have a u32.
>>
>> I intentionally did not do this, as the common type for an index is usize. Thus,
>> I do not expect anyone to already have a u32, but to already have a usize, e.g.
>> from some iterator.
>>
>> The fact that the C API did pick unsigned int as index type is an implementation
>> detail the abstraction should bother with.
> 
> Thanks for the clarification (and Miguel for elaborating - I wasn't
> completely aware of it!). In that case this patch looks correct to me.
> 
> Note that there is another `as` right after, in the same method:
> 
>   if irq < 0 {
>       return Err(Error::from_errno(irq));
>   }
> 
>   // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
>   Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })
> 
> 
> We could get rid of it by replacing the `if irq < 0` test with:
> 
>   let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;

That's a good suggestion!

> Sophon, if you feel like doing it I think this could improve the patch
> further.

Let's please do that as a separate patch though.

Thanks,
Danilo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 13:32     ` Alexandre Courbot
  2026-09-01 13:36       ` Danilo Krummrich
@ 2026-09-01 13:48       ` Gary Guo
  2026-09-01 14:08         ` Alexandre Courbot
  1 sibling, 1 reply; 15+ messages in thread
From: Gary Guo @ 2026-09-01 13:48 UTC (permalink / raw)
  To: Alexandre Courbot, Danilo Krummrich
  Cc: Sophon Zhang via B4 Relay, aiqubits, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 2:32 PM BST, Alexandre Courbot wrote:
> On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
>> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>>> If that's what the C API expects, it does make sense to align to it
>>> instead of forcing users to make a potential unneeded conversion if they
>>> already have a u32.
>>
>> I intentionally did not do this, as the common type for an index is usize. Thus,
>> I do not expect anyone to already have a u32, but to already have a usize, e.g.
>> from some iterator.
>>
>> The fact that the C API did pick unsigned int as index type is an implementation
>> detail the abstraction should bother with.
>
> Thanks for the clarification (and Miguel for elaborating - I wasn't
> completely aware of it!). In that case this patch looks correct to me.
>
> Note that there is another `as` right after, in the same method:
>
>   if irq < 0 {
>       return Err(Error::from_errno(irq));
>   }
>
>   // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
>   Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })
>
>
> We could get rid of it by replacing the `if irq < 0` test with:
>
>   let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;

I wonder if we can just change kernel::error::to_result to return `Result<u32>`
instead and have the cast there?

Best,
Gary

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 13:36       ` Danilo Krummrich
@ 2026-09-01 13:52         ` Alexandre Courbot
  0 siblings, 0 replies; 15+ messages in thread
From: Alexandre Courbot @ 2026-09-01 13:52 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Sophon Zhang via B4 Relay, aiqubits, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 10:36 PM JST, Danilo Krummrich wrote:
> On 9/1/26 3:32 PM, Alexandre Courbot wrote:
>> On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
>>> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>>>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>>>> If that's what the C API expects, it does make sense to align to it
>>>> instead of forcing users to make a potential unneeded conversion if they
>>>> already have a u32.
>>>
>>> I intentionally did not do this, as the common type for an index is usize. Thus,
>>> I do not expect anyone to already have a u32, but to already have a usize, e.g.
>>> from some iterator.
>>>
>>> The fact that the C API did pick unsigned int as index type is an implementation
>>> detail the abstraction should bother with.
>> 
>> Thanks for the clarification (and Miguel for elaborating - I wasn't
>> completely aware of it!). In that case this patch looks correct to me.
>> 
>> Note that there is another `as` right after, in the same method:
>> 
>>   if irq < 0 {
>>       return Err(Error::from_errno(irq));
>>   }
>> 
>>   // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
>>   Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })
>> 
>> 
>> We could get rid of it by replacing the `if irq < 0` test with:
>> 
>>   let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;
>
> That's a good suggestion!

I expect this pattern to be common, so how about we add a variant of
`error::to_result` that returns the value as a `u32`? I.e.

  pub fn to_result_value(err: crate::ffi::c_int) -> Result<u32>

>
>> Sophon, if you feel like doing it I think this could improve the patch
>> further.
>
> Let's please do that as a separate patch though.

In this case, the current patch is

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 13:48       ` Gary Guo
@ 2026-09-01 14:08         ` Alexandre Courbot
  2026-09-01 15:58           ` 回复: " ai qubits
  0 siblings, 1 reply; 15+ messages in thread
From: Alexandre Courbot @ 2026-09-01 14:08 UTC (permalink / raw)
  To: Gary Guo
  Cc: Danilo Krummrich, Sophon Zhang via B4 Relay, aiqubits,
	Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda,
	Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein,
	Onur Özkan, linux-pci, rust-for-linux, linux-kernel

On Tue Sep 1, 2026 at 10:48 PM JST, Gary Guo wrote:
> On Tue Sep 1, 2026 at 2:32 PM BST, Alexandre Courbot wrote:
>> On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
>>> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>>>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>>>> If that's what the C API expects, it does make sense to align to it
>>>> instead of forcing users to make a potential unneeded conversion if they
>>>> already have a u32.
>>>
>>> I intentionally did not do this, as the common type for an index is usize. Thus,
>>> I do not expect anyone to already have a u32, but to already have a usize, e.g.
>>> from some iterator.
>>>
>>> The fact that the C API did pick unsigned int as index type is an implementation
>>> detail the abstraction should bother with.
>>
>> Thanks for the clarification (and Miguel for elaborating - I wasn't
>> completely aware of it!). In that case this patch looks correct to me.
>>
>> Note that there is another `as` right after, in the same method:
>>
>>   if irq < 0 {
>>       return Err(Error::from_errno(irq));
>>   }
>>
>>   // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
>>   Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })
>>
>>
>> We could get rid of it by replacing the `if irq < 0` test with:
>>
>>   let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;
>
> I wonder if we can just change kernel::error::to_result to return `Result<u32>`
> instead and have the cast there?

Looks like our messages crossed [1]. :)

Converting `to_result` would require quite a bit of work to update all
the callers, but maybe we can introduce a new variant indeed.

But this makes me think of another step we can take to harden
`to_result`: it should probably warn if the non-error value if not `0`,
as that would indicate the caller needs to consider it.

[1] https://lore.kernel.org/all/DL412XWBP4Y2.K1TH9NELBKPR@nvidia.com/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* 回复: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-09-01 14:08         ` Alexandre Courbot
@ 2026-09-01 15:58           ` ai qubits
  0 siblings, 0 replies; 15+ messages in thread
From: ai qubits @ 2026-09-01 15:58 UTC (permalink / raw)
  To: Alexandre Courbot, Gary Guo
  Cc: Danilo Krummrich, Sophon Zhang via B4 Relay, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Onur Özkan,
	linux-pci@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org

I agree with Alexandre on this point. 
I will keep v4 unchanged and handle the 
return-value conversion helper separately.

Best regards,
Sophon

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-08-31 17:09 [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32 Sophon Zhang via B4 Relay
  2026-09-01 10:58 ` Alexandre Courbot
@ 2026-09-01 16:19 ` Gary Guo
  2026-09-01 16:31 ` Danilo Krummrich
  2 siblings, 0 replies; 15+ messages in thread
From: Gary Guo @ 2026-09-01 16:19 UTC (permalink / raw)
  To: aiqubits, Danilo Krummrich, Bjorn Helgaas,
	Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
	Onur Özkan
  Cc: linux-pci, rust-for-linux, linux-kernel

On Mon Aug 31, 2026 at 6:09 PM BST, Sophon Zhang via B4 Relay wrote:
> From: Sophon Zhang <aiqubits@hotmail.com>
> 
> IrqVectorRegistration::index() accepts a usize, but pci_irq_vector()
> takes an unsigned int. On 64-bit architectures, casting an index larger
> than u32::MAX wraps it before the PCI core can validate it. In
> particular, u32::MAX + 1 becomes zero and can resolve to the first
> allocated vector.
> 
> Use a checked conversion and return EINVAL when the index cannot be
> represented by the C API.
> 
> Fixes: 2fb7755b0a7e ("rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector")
> Signed-off-by: Sophon Zhang <aiqubits@hotmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> Prevent 64-bit Rust IRQ vector indices from wrapping when they cross the
> PCI C API boundary.
> ---
> Changes in v4:
> - Drop the explicit length check in favor of PCI core range validation.
> - Use the existing TryFromIntError-to-Error conversion directly.
> - Keep commit trailers adjacent and narrow the description to truncation.
> - Link to v3: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v3-1-a2103084d20e@hotmail.com
> 
> Changes in v3:
> - Check the index against the allocated vector count before entering the C API.
> - Keep the checked usize-to-u32 conversion and document the C-side warning.
> - Link to v2: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v2-1-4030ea7746a9@hotmail.com
> 
> Changes in v2:
> - No code changes.
> - Link to v1: https://patch.msgid.link/20260831-fix-pci-irq-vector-index-truncation-v1-1-d63217d99b67@hotmail.com
> 
> Testing:
> - make rustfmtcheck
> - Not build- or hardware-tested; bindgen is unavailable in the test environment.
> ---
>  rust/kernel/pci/irq.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32
  2026-08-31 17:09 [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32 Sophon Zhang via B4 Relay
  2026-09-01 10:58 ` Alexandre Courbot
  2026-09-01 16:19 ` Gary Guo
@ 2026-09-01 16:31 ` Danilo Krummrich
  2 siblings, 0 replies; 15+ messages in thread
From: Danilo Krummrich @ 2026-09-01 16:31 UTC (permalink / raw)
  To: Sophon Zhang
  Cc: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
	Onur Özkan, linux-pci, rust-for-linux, linux-kernel

On Tue, 01 Sep 2026 01:09:53 +0800, Sophon Zhang wrote:
> [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32

Applied, thanks!

  Branch: driver-core-linus
  Tree:   git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git

[1/1] rust: pci: reject IRQ vector indices that do not fit in u32
      commit: 8d7b3e41ffec

The patch will appear in the next linux-next integration (typically within 24
hours on weekdays).

The patch is queued up for Linus's tree and should land in the next -rc release.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-09-01 16:31 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-31 17:09 [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32 Sophon Zhang via B4 Relay
2026-09-01 10:58 ` Alexandre Courbot
2026-09-01 11:08   ` Danilo Krummrich
2026-09-01 13:32     ` Alexandre Courbot
2026-09-01 13:36       ` Danilo Krummrich
2026-09-01 13:52         ` Alexandre Courbot
2026-09-01 13:48       ` Gary Guo
2026-09-01 14:08         ` Alexandre Courbot
2026-09-01 15:58           ` 回复: " ai qubits
2026-09-01 11:47   ` ai qubits
2026-09-01 12:06     ` Gary Guo
2026-09-01 12:35       ` 回复: " ai qubits
2026-09-01 12:42   ` Miguel Ojeda
2026-09-01 16:19 ` Gary Guo
2026-09-01 16:31 ` Danilo Krummrich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox