linux-kernel - Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2024120637-handoff-monetary-c2f5@gregkh>
Date: Fri, 6 Dec 2024 14:10:27 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, arnd@...db.de, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional
 get/set value functionality

On Fri, Dec 06, 2024 at 01:06:30PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > > On Fri, Dec 06, 2024 at 12:42:14PM +0000, Lee Jones wrote:
> > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > +        let guard = self.inner.lock();
> > > > +        let value = guard.value;
> > > > +
> > > > +        // Refrain from calling write() on a locked resource
> > > > +        drop(guard);
> > > > +
> > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > > > +
> > > > +        writer.write::<i32>(&value)?;
> > > > +        Ok(0)
> > > > +    }
> > > 
> > > I don't understand why you have to drop the mutex before calling
> > > pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> > > spinlock, so you can hold it over that potentially-sleeping call, right?
> > > Or is there some other reason why here?
> > 
> > This was a request from Alice to demonstrate how to unlock a mutex.
> 
> It's common practice to apply guards only around the protected value.
> 
> Why would this be different?

It isn't, it's just that you are implying that the guard has to be
dropped because of the call to write(), which is confusing.  It's only
"needed" because you want to guard a single cpu instruction that is
guaranteed atomic by the processor :)

As this is an example driver, documentation is essential, so maybe the
comment should be:
	// Drop the mutex as we can now use our local copy
or something like that.

thanks,

greg k-h