Re: [PATCH v1] rust: serdev: use Delta<Jiffy> for timeouts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rust: serdev: negative timeout clamped to 0 causes infinite wait
--

commit 68348e8f0bd03a5788e4d72cf264605070a06317
Author: FUJITA Tomonori <fujita.tomonori@gmail.com>

rust: serdev: use Delta<Jiffy> for timeouts

write_all() and wait_until_sent() take the timeout as a raw Jiffies,
which is a c_ulong alias. The type does not show the unit.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..094503f059ff5 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs

[ ... ]

> @@ -481,22 +484,19 @@ pub fn set_parity(&self, parity: Parity) -> Result {
>      /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
>      /// before any bytes were written. [`kernel::error::code::EINVAL`] if `data.len() > i32::MAX`.
>      #[inline]
> -    pub fn write_all(&self, data: &[u8], timeout: Jiffies) -> Result<usize> {
> +    pub fn write_all(&self, data: &[u8], timeout: Delta<Jiffy>) -> Result<usize> {
>          if data.len() > i32::MAX as usize {
>              return Err(EINVAL);
>          }
>  
> +        let timeout = isize::max(timeout.as_jiffies(), 0);

[Severity: High]
Does this cause an unintended infinite wait when a negative timeout is passed?

When a driver calculates a deadline that is already in the past, it will
produce a negative Delta<Jiffy>, which this clamps to 0.

However, the underlying C function serdev_device_write() treats a timeout of
0 as an infinite wait:

drivers/tty/serdev/core.c:serdev_device_write() {
    ...
	if (timeout == 0)
		timeout = MAX_SCHEDULE_TIMEOUT;
    ...
}

Should negative timeouts instead return immediately or fail with -ETIMEDOUT
rather than causing the thread to hang indefinitely?

> +
>          // SAFETY:
>          // - `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
>          // - `data.as_ptr()` is guaranteed to be a valid array pointer with the size of
>          //   `data.len()`.
>          let ret = unsafe {
> -            bindings::serdev_device_write(
> -                self.as_raw(),
> -                data.as_ptr(),
> -                data.len(),
> -                isize::try_from(timeout).unwrap_or_default(),
> -            )
> +            bindings::serdev_device_write(self.as_raw(), data.as_ptr(), data.len(), timeout)
>          };

[ ... ]

> @@ -547,14 +547,11 @@ pub fn write_flush(&self) {
>      ///
>      /// Use a timeout of 0 to wait indefinitely.
>      #[inline]
> -    pub fn wait_until_sent(&self, timeout: Jiffies) {
> +    pub fn wait_until_sent(&self, timeout: Delta<Jiffy>) {
> +        let timeout = isize::max(timeout.as_jiffies(), 0);

[Severity: High]
Can this also cause an unintended infinite wait?

Similar to write_all(), if wait_until_sent() is called with a negative timeout,
it is clamped to 0.

The underlying C implementation tty_wait_until_sent() treats a timeout of 0
as an infinite wait:

drivers/tty/tty_ioctl.c:tty_wait_until_sent() {
    ...
	if (!timeout)
		timeout = MAX_SCHEDULE_TIMEOUT;
    ...
}

Would it be safer to handle expired deadlines without waiting indefinitely?

> +
>          // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> -        unsafe {
> -            bindings::serdev_device_wait_until_sent(
> -                self.as_raw(),
> -                isize::try_from(timeout).unwrap_or_default(),
> -            )
> -        };
> +        unsafe { bindings::serdev_device_wait_until_sent(self.as_raw(), timeout) };
>      }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930041339.1551129-1-tomo@flapping.org?part=1