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

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

Message-ID: <20250512-unrat-kapital-2122d3777c5d@brauner>
Date: Mon, 12 May 2025 11:52:14 +0200
From: Christian Brauner <brauner@...nel.org>
To: Max Kellermann <max.kellermann@...os.com>, jack@...e.cz
Cc: 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

Sorry, coming back to this now. I lost sight of this patch.

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.

> I wrote this while debugging lots of netfs/nfs/ceph bugs; even without
> these bugs, I/O operations on netfs can take a looong time (if the
> server is slow) and the inode is locked during the whole operation.
> That can cause lots of other processes to go stuck, and my patch
> allows these operations to be canceled. Without this, the processes
> not only remain stuck until the inode is unlocked, but all stuck
> processes have to finish all their I/O before anything can continue.
> I'd like to be able to "kill -9" stuck processes.
> 
> A similar NFS-specific patch I wrote was merged last year:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=38a125b31504f91bf6fdd3cfc3a3e9a721e6c97a
> The same patch for Ceph was never merged (but not explicitly
> rejected): https://lore.kernel.org/lkml/20241206165014.165614-1-max.kellermann@ionos.com/
> Prior to my work, several NFS operations were already killable.