linux-ext4 - Re: [PATCH 18/50] fs: disallow 0 reference count inodes

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: <20250825-person-knapp-e802daccfe5b@brauner>
Date: Mon, 25 Aug 2025 12:54:01 +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 18/50] fs: disallow 0 reference count inodes

On Thu, Aug 21, 2025 at 04:18:29PM -0400, Josef Bacik wrote:
> Now that we take a full reference for inodes on the LRU, move the logic
> to add the inode to the LRU to before we drop our last reference. This
> allows us to ensure that if the inode has a reference count it can be
> used, and we no longer hold onto inodes that have a 0 reference count.
> 
> Signed-off-by: Josef Bacik <josef@...icpanda.com>
> ---
>  fs/inode.c | 53 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index de0ec791f9a3..b4145ddbaf8e 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -614,7 +614,7 @@ static void __inode_add_lru(struct inode *inode, bool rotate)
>  
>  	if (inode->i_state & (I_FREEING | I_WILL_FREE))
>  		return;
> -	if (atomic_read(&inode->i_count))
> +	if (atomic_read(&inode->i_count) != 1)
>  		return;
>  	if (inode->__i_nlink == 0)
>  		return;
> @@ -1966,28 +1966,11 @@ EXPORT_SYMBOL(generic_delete_inode);
>   * in cache if fs is alive, sync and evict if fs is
>   * shutting down.
>   */
> -static void iput_final(struct inode *inode, bool skip_lru)
> +static void iput_final(struct inode *inode, bool drop)
>  {
> -	struct super_block *sb = inode->i_sb;
> -	const struct super_operations *op = inode->i_sb->s_op;
>  	unsigned long state;
> -	int drop;
>  
>  	WARN_ON(inode->i_state & I_NEW);
> -
> -	if (op->drop_inode)
> -		drop = op->drop_inode(inode);
> -	else
> -		drop = generic_drop_inode(inode);
> -
> -	if (!drop && !skip_lru &&
> -	    !(inode->i_state & I_DONTCACHE) &&
> -	    (sb->s_flags & SB_ACTIVE)) {
> -		__inode_add_lru(inode, true);
> -		spin_unlock(&inode->i_lock);
> -		return;
> -	}
> -
>  	WARN_ON(!list_empty(&inode->i_lru));
>  
>  	state = inode->i_state;
> @@ -2009,8 +1992,29 @@ static void iput_final(struct inode *inode, bool skip_lru)
>  	evict(inode);
>  }
>  
> +static bool maybe_add_lru(struct inode *inode, bool skip_lru)
> +{
> +	const struct super_operations *op = inode->i_sb->s_op;
> +	struct super_block *sb = inode->i_sb;
> +	bool drop = false;
> +
> +	if (op->drop_inode)
> +		drop = op->drop_inode(inode);
> +	else
> +		drop = generic_drop_inode(inode);
> +
> +	if (!drop && !skip_lru &&
> +	    !(inode->i_state & I_DONTCACHE) &&
> +	    (sb->s_flags & SB_ACTIVE))
> +		__inode_add_lru(inode, true);
> +
> +	return drop;
> +}

Can we rewrite this as:

static bool maybe_add_lru(struct inode *inode, bool skip_lru)
{
	const struct super_operations *op = inode->i_sb->s_op;
	const struct super_block *sb = inode->i_sb;
	bool drop = false;

	if (op->drop_inode)
		drop = op->drop_inode(inode);
	else
		drop = generic_drop_inode(inode);

	if (drop)
		return drop;

	if (skip_lru)
		return drop;

	if (inode->i_state & I_DONTCACHE)
		return drop;

	if (!(sb->s_flags & SB_ACTIVE))
		return drop;

	__inode_add_lru(inode, true);
	return drop;
}

so it's a lot easier to follow. I really dislike munging conditions
together with a bunch of ands and negations mixed in.

And btw for both I_DONTCACHE and !SB_ACTIVE it seems that returning
anything other than false from op->drop_inode() would be a bug probably
a technicality but I find it pretty odd.

Maybe we add a VFS_WARN_ON_ONCE() at least in your local testing to see
whether you see anything that ever hits this case.

> +
>  static void __iput(struct inode *inode, bool skip_lru)
>  {
> +	bool drop;
> +
>  	if (!inode)
>  		return;
>  	BUG_ON(inode->i_state & I_CLEAR);
> @@ -2026,8 +2030,17 @@ static void __iput(struct inode *inode, bool skip_lru)
>  	}
>  
>  	spin_lock(&inode->i_lock);
> +
> +	/*
> +	 * If we want to keep the inode around on an LRU we will grab a ref to
> +	 * the inode when we add it to the LRU list, so we can safely drop the
> +	 * callers reference after this. If we didn't add the inode to the LRU
> +	 * then the refcount will still be 1 and we can do the final iput.
> +	 */
> +	drop = maybe_add_lru(inode, skip_lru);
> +
>  	if (atomic_dec_and_test(&inode->i_count))
> -		iput_final(inode, skip_lru);
> +		iput_final(inode, drop);
>  	else
>  		spin_unlock(&inode->i_lock);
>  
> -- 
> 2.49.0
>