linux-ext4 - Re: [PATCH 15/50] fs: delete the inode from the LRU list on lookup

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] [day] [month] [year] [list]

Message-ID: <20250822-werden-hinein-419c34f78154@brauner>
Date: Fri, 22 Aug 2025 17:27:56 +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
Subject: Re: [PATCH 15/50] fs: delete the inode from the LRU list on lookup

On Thu, Aug 21, 2025 at 04:18:26PM -0400, Josef Bacik wrote:
> When we move to holding a full reference on the inode when it is on an
> LRU list we need to have a mechanism to re-run the LRU add logic. The
> use case for this is btrfs's snapshot delete, we will lookup all the
> inodes and try to drop them, but if they're on the LRU we will not call
> ->drop_inode() because their refcount will be elevated, so we won't know
> that we need to drop the inode.
> 
> Fix this by simply removing the inode from it's respective LRU list when
> we grab a reference to it in a way that we have active users.  This will
> ensure that the logic to add the inode to the LRU or drop the inode will
> be run on the final iput from the user.
> 
> Signed-off-by: Josef Bacik <josef@...icpanda.com>
> ---
>  fs/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index adcba0a4d776..72981b890ec6 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1146,6 +1146,7 @@ static struct inode *find_inode(struct super_block *sb,
>  			return ERR_PTR(-ESTALE);
>  		}
>  		__iget(inode);
> +		inode_lru_list_del(inode);
>  		spin_unlock(&inode->i_lock);
>  		rcu_read_unlock();
>  		return inode;
> @@ -1187,6 +1188,7 @@ static struct inode *find_inode_fast(struct super_block *sb,
>  			return ERR_PTR(-ESTALE);
>  		}
>  		__iget(inode);
> +		inode_lru_list_del(inode);
>  		spin_unlock(&inode->i_lock);
>  		rcu_read_unlock();
>  		return inode;
> @@ -1653,6 +1655,7 @@ struct inode *igrab(struct inode *inode)
>  	spin_lock(&inode->i_lock);
>  	if (!(inode->i_state & (I_FREEING|I_WILL_FREE))) {
>  		__iget(inode);
> +		inode_lru_list_del(inode);
>  		spin_unlock(&inode->i_lock);
>  	} else {
>  		spin_unlock(&inode->i_lock);

Interesting, so the previous behavior implies that igrab(),
find_inode(), find_inode_fast() are called on inodes that are hashed and
on an LRU. None of them even raise I_REFERENCED.

I would think that this means that there are callers that grab very
temporary references to inodes that they immediately drop without
wanting to prevent reclaim.

Oh, because btrfs subvolume delete is effectively a recursive directory
removal and that's why that happens? I wonder if there are other users.
So if this regresses someone it would regress btrfs I guess. :)