linux-ext4 - Re: [PATCH v2 17/54] fs: remove the inode from the LRU list on unlink/rmdir

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: <20250828-rockermilieu-absender-ae4d11591ec9@brauner>
Date: Thu, 28 Aug 2025 12:43:54 +0200
From: Christian Brauner <brauner@...nel.org>
To: Josef Bacik <josef@...icpanda.com>
Cc: linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org, 
	kernel-team@...com, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, 
	viro@...iv.linux.org.uk, amir73il@...il.com
Subject: Re: [PATCH v2 17/54] fs: remove the inode from the LRU list on
 unlink/rmdir

On Thu, Aug 28, 2025 at 11:06:37AM +0200, Christian Brauner wrote:
> On Tue, Aug 26, 2025 at 11:39:17AM -0400, Josef Bacik wrote:
> > We can end up with an inode on the LRU list or the cached list, then at
> > some point in the future go to unlink that inode and then still have an
> > elevated i_count reference for that inode because it is on one of these
> > lists.
> > 
> > The more common case is the cached list. We open a file, write to it,
> > truncate some of it which triggers the inode_add_lru code in the
> > pagecache, adding it to the cached LRU.  Then we unlink this inode, and
> > it exists until writeback or reclaim kicks in and removes the inode.
> > 
> > To handle this case, delete the inode from the LRU list when it is
> > unlinked, so we have the best case scenario for immediately freeing the
> > inode.
> > 
> > Signed-off-by: Josef Bacik <josef@...icpanda.com>
> > ---
> >  fs/namei.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 138a693c2346..e56dcb5747e4 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4438,6 +4438,7 @@ SYSCALL_DEFINE2(mkdir, const char __user *, pathname, umode_t, mode)
> >  int vfs_rmdir(struct mnt_idmap *idmap, struct inode *dir,
> >  		     struct dentry *dentry)
> >  {
> > +	struct inode *inode = dentry->d_inode;
> >  	int error = may_delete(idmap, dir, dentry, 1);
> >  
> >  	if (error)
> > @@ -4447,11 +4448,11 @@ int vfs_rmdir(struct mnt_idmap *idmap, struct inode *dir,
> >  		return -EPERM;
> >  
> >  	dget(dentry);
> > -	inode_lock(dentry->d_inode);
> > +	inode_lock(inode);
> >  
> >  	error = -EBUSY;
> >  	if (is_local_mountpoint(dentry) ||
> > -	    (dentry->d_inode->i_flags & S_KERNEL_FILE))
> > +	    (inode->i_flags & S_KERNEL_FILE))
> >  		goto out;
> >  
> >  	error = security_inode_rmdir(dir, dentry);
> > @@ -4463,12 +4464,21 @@ int vfs_rmdir(struct mnt_idmap *idmap, struct inode *dir,
> >  		goto out;
> >  
> >  	shrink_dcache_parent(dentry);
> > -	dentry->d_inode->i_flags |= S_DEAD;
> > +	inode->i_flags |= S_DEAD;
> >  	dont_mount(dentry);
> >  	detach_mounts(dentry);
> >  
> >  out:
> > -	inode_unlock(dentry->d_inode);
> > +	/*
> > +	 * The inode may be on the LRU list, so delete it from the LRU at this
> > +	 * point in order to make sure that the inode is freed as soon as
> > +	 * possible.
> > +	 */
> > +	spin_lock(&inode->i_lock);
> > +	inode_lru_list_del(inode);
> > +	spin_unlock(&inode->i_lock);
> > +
> > +	inode_unlock(inode);
> >  	dput(dentry);
> >  	if (!error)
> >  		d_delete_notify(dir, dentry);
> > @@ -4653,8 +4663,18 @@ int do_unlinkat(int dfd, struct filename *name)
> >  		dput(dentry);
> 
> Why are you doing that in do_unlinkat() instead of vfs_unlink() (as
> you're doing it in vfs_rmdir() and not do_rmdir())?
> 
> Doing it in do_unlinkat() means any stacking filesystem such as
> overlayfs will end up skipping the LRU list removal as they use
> vfs_unlink() directly.
> 
> And does btrfs subvolume/snapshot deletion special treatment as well for
> this as it's semantically equivalent to an rmdir?

Another thing. If this is unlinking an inode with multiple hardlinks
then vfs_unlink() might not remove the inode if it's not the last
hardlink. Do you really want to remove the inode from the LRU list in
that case as well?

> >  	}
> >  	inode_unlock(path.dentry->d_inode);
> > -	if (inode)
> > +	if (inode) {
> > +		/*
> > +		 * The LRU may be holding a reference, remove the inode from the
> > +		 * LRU here before dropping our hopefully final reference on the
> > +		 * inode.
> > +		 */
> > +		spin_lock(&inode->i_lock);
> > +		inode_lru_list_del(inode);
> > +		spin_unlock(&inode->i_lock);
> > +
> >  		iput(inode);	/* truncate the inode here */
> > +	}
> >  	inode = NULL;
> >  	if (delegated_inode) {
> >  		error = break_deleg_wait(&delegated_inode);
> > -- 
> > 2.49.0
> >