linux-kernel - Re: [PATCH 2/3] rust: file: add f_pos and set_f

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: <20240927-ergaben-abdrehen-a02a861abaf5@brauner>
Date: Fri, 27 Sep 2024 09:32:02 +0200
From: Christian Brauner <brauner@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Arnd Bergmann <arnd@...db.de>, Miguel Ojeda <ojeda@...nel.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

On Thu, Sep 26, 2024 at 02:58:56PM GMT, Alice Ryhl wrote:
> Add accessors for the file position. Most of the time, you should not
> use these methods directly, and you should instead use a guard for the
> file position to prove that you hold the fpos lock. However, under
> limited circumstances, files are allowed to choose a different locking
> strategy for their file position. These accessors can be used to handle
> that case.
> 
> For now, these accessors are the only way to access the file position
> within the llseek and read_iter callbacks.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/fs/file.rs | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
> index e03dbe14d62a..c896a3b1d182 100644
> --- a/rust/kernel/fs/file.rs
> +++ b/rust/kernel/fs/file.rs
> @@ -333,6 +333,26 @@ pub fn flags(&self) -> u32 {
>          // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>          unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
>      }
> +
> +    /// Read the file position.
> +    ///
> +    /// # Safety
> +    ///
> +    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> +    #[inline]
> +    pub unsafe fn f_pos(&self) -> i64 {
> +        unsafe { (*self.as_ptr()).f_pos }
> +    }
> +
> +    /// Set the file position.
> +    ///
> +    /// # Safety
> +    ///
> +    /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> +    #[inline]
> +    pub unsafe fn set_f_pos(&self, value: i64) {
> +        unsafe { (*self.as_ptr()).f_pos = value };
> +    }

I don't think we want to expose f_pos with its weird locking rule
through rust wrappers. Ideally, it's completely opaque to the callers
and not accessed directly at all.