linux-kernel - Re: [PATCH 2/2] fs: make several inode lock operations killable

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: <20250513-gebilde-beglichen-e60708a46caf@brauner>
Date: Tue, 13 May 2025 09:30:39 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: Max Kellermann <max.kellermann@...os.com>, viro@...iv.linux.org.uk, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] fs: make several inode lock operations killable

On Mon, May 12, 2025 at 07:33:13PM +0200, Jan Kara wrote:
> On Mon 12-05-25 11:52:14, Christian Brauner wrote:
> > On Tue, Apr 29, 2025 at 01:28:49PM +0200, Max Kellermann wrote:
> > > On Tue, Apr 29, 2025 at 1:12 PM Christian Brauner <brauner@...nel.org> wrote:
> > > > > --- a/fs/read_write.c
> > > > > +++ b/fs/read_write.c
> > > > > @@ -332,7 +332,9 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
> > > > >       struct inode *inode = file_inode(file);
> > > > >       loff_t retval;
> > > > >
> > > > > -     inode_lock(inode);
> > > > > +     retval = inode_lock_killable(inode);
> > > >
> > > > That change doesn't seem so obviously fine to me.
> > > 
> > > Why do you think so? And how is this different than the other two.
> > 
> > chown_common() and chmod_common() are very close to the syscall boundary
> > so it's very unlikely that we run into weird issues apart from userspace
> > regression when they suddenly fail a change for new unexpected reasons.
> > 
> > But just look at default_llseek():
> > 
> >     > git grep default_llseek | wc -l
> >     461
> > 
> > That is a lot of stuff and it's not immediately clear how deeply or
> > nested they are called. For example from overlayfs in stacked
> > callchains. Who knows what strange assumptions some of the callers have
> > including the possible return values from that helper.
> > 
> > > 
> > > > Either way I'd like to see this split in three patches and some
> > > > reasoning why it's safe and some justification why it's wanted...
> > > 
> > > Sure I can split this patch, but before I spend the time, I'd like us
> > > first to agree that the patch is useful.
> > 
> > This is difficult to answer. Yes, on the face of it it seems useful to
> > be able to kill various operations that sleep on inode lock but who
> > knows what implicit guarantees/expectations we're going to break if we
> > do it. Maybe @Jan has some thoughts here as well.
> 
> So I think having lock killable is useful and generally this should be safe
> wrt userspace (the process will get killed before it can notice the
> difference anyway). The concern about guarantees / expectations is still
> valid for the *kernel* code (which is I think what you meant above). So I
> guess some audit of kernel paths that can end up calling ->llseek handler
> and whether they are OK with the handler returning EINTR is needed. I
> expect this will be fine but I would not bet too much on it :)

Great. @Max you want to send an updated version where split into
separate patches?