linux-kernel - Re: [PATCH 4/4] leds: add driver for synology atmega1608 controlled LEDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53fd34386cefd4a789c56a6e6d7c5618c06c531b.camel@posteo.de>
Date: Thu, 09 Oct 2025 12:30:17 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, Miguel
 Ojeda	 <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Igor
 Korotin	 <igor.korotin.linux@...il.com>, Lorenzo Stoakes
 <lorenzo.stoakes@...cle.com>,  Vlastimil Babka	 <vbabka@...e.cz>, "Liam R.
 Howlett" <Liam.Howlett@...cle.com>, Uladzislau Rezki	 <urezki@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>, Andreas
 Hindborg	 <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross	 <tmgross@...ch.edu>, Daniel Almeida
 <daniel.almeida@...labora.com>, 	linux-leds@...r.kernel.org,
 rust-for-linux@...r.kernel.org, 	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] leds: add driver for synology atmega1608 controlled
 LEDs

On Thu, 2025-10-09 at 14:20 +0200, Danilo Krummrich wrote:
> (Not a full review (let's work out the dependencies first), but
> there's one
> thing that stood out to me.)
> 
> On Wed Oct 8, 2025 at 8:10 PM CEST, Markus Probst wrote:
> > +struct Atmega1608Led {
> > +    addr: Atmega1608LedAddress,
> > +    id: Atmega1608LedId,
> > +
> > +    client: ARef<I2cClient>,
> > +
> > +    mode_lock: Arc<Mutex<()>>,
> 
> Mutex<()> raises an eyebrow, since a mutex that doesn't protect
> anything is
> pointless. So, I assume it is protecting some data, but in an unsound
> way.
> 
> > +impl Atmega1608Led {
> > +    fn update_mode(&self, mode: Atmega1608LedMode) ->
> > Result<Atmega1608LedMode> {
> > +        let _guard = self.mode_lock.lock();
> 
> What exactly does the mutex protect in the code below?
Otherwise there would be a race condition. Each register has 8 bits,
each led has 2 bits. If the led mode is updated at the same time with
another one in the same register, it could lead to the first action
being overwritten by the second.
Meaning if two actions run at the same time:
- led0 reads from the register
- led1 reads from the register
- led0 writes to the register
- led1 writes to the register (the changes for led0 have been
overwritten here, as it did read the register before led0 has written
to it)

> 
> > +
> > +        let mut current = self
> > +            .client
> > +            .read_byte_data(self.addr as u8)
> > +            .inspect_err(|err| {
> > +                dev_err!(
> > +                    self.client.as_ref(),
> > +                    "failed to read {:#2x}: {err:?}\n",
> > +                    self.addr as u8
> > +                );
> > +            })?;
> > +
> > +        current =
> > +            (current & !self.id.mask()) | (((mode as u8) <<
> > self.id.shift()) & self.id.mask());
> > +
> > +        self.client
> > +            .write_byte_data(self.addr as u8, current)
> > +            .inspect_err(|err| {
> > +                dev_err!(
> > +                    self.client.as_ref(),
> > +                    "failed to write {:#2x}: {err:?}",
> > +                    self.addr as u8
> > +                );
> > +            })?;
> > +
> > +        Ok(mode)
> > +    }
> > +}

Thanks
- Markus Probst