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: <20240927193809.GV3550746@ZenIV>
Date: Fri, 27 Sep 2024 20:38:09 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arnd Bergmann <arnd@...db.de>, Miguel Ojeda <ojeda@...nel.org>,
	Christian Brauner <brauner@...nel.org>, 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 Fri, Sep 27, 2024 at 08:56:50AM +0200, Alice Ryhl wrote:

> Okay, interesting. I did not know about all of these llseek helpers.
> I'm definitely happy to make the Rust API force users to do the right
> thing if we can.
> 
> It sounds like we basically have a few different seeking behaviors
> that the driver can choose between, and we want to force the user to
> use one of them?

Depends...  Basically, SEEK_HOLE/SEEK_DATA is seriously fs-specific
(unsurprisingly), and pretty much everything wants the usual relation
between SEEK_SET and SEEK_CUR (<SEEK_CUR,n> is the same as <SEEK_SET,
current position + n>).  SEEK_END availability varies - the simplest
variant is <SEEK_END, n> == <SEEK_SET, size + n>, but there are
cases that genuinely have nothing resembling end-relative seek
(e.g. anything seq_file-related).

It's not so much available instances as available helpers; details of
semantics may seriously vary by the driver.

Note that once upon a time ->f_pos had been exposed to ->read() et.al.;
caused recurring bugs, until we switched to "sample ->f_pos before calling
->read(), pass the reference to local copy into the method, then put
what's the method left behind in there back into ->f_pos".

Something similar might be a good idea for ->llseek().  Locking is
an unpleasant problem, unfortunately.  lseek() is not a terribly hot
codepath, but read() and write() are.  For a while we used to do exclusion
on per-struct file basis for _all_ read/write/lseek; see 797964253d35
"file: reinstate f_pos locking optimization for regular files" for the
point where it eroded.

FWIW, I suspect that unconditionally taking ->f_pos_mutex for llseek(2)
would solve most of the problems - for one thing, with guaranteed
per-struct-file serialization of vfs_llseek() we could handle SEEK_CUR
right there, so that ->llseek() instances would never see it; for another,
we just might be able to pull the same 'pass a reference to local variable
and let it be handled there' trick for ->llseek().  That would require
an audit of locking in the instances, though...