linux-ext4 - Re: [PATCH 21/50] fs: use refcount_inc_not

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: <20250825-bahnnetz-fragen-c6571104ea56@brauner>
Date: Mon, 25 Aug 2025 13:21:28 +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 21/50] fs: use refcount_inc_not_zero in igrab

On Thu, Aug 21, 2025 at 04:18:32PM -0400, Josef Bacik wrote:
> We are going to use igrab everywhere we want to acquire a live inode.
> Update it to do a refcount_inc_not_zero on the i_count, and if
> successful grab an reference to i_obj_count. Add a comment explaining
> why we do this and the safety.
> 
> Signed-off-by: Josef Bacik <josef@...icpanda.com>
> ---
>  fs/inode.c         | 26 +++++++++++++-------------
>  include/linux/fs.h | 27 +++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 28d197731914..b9122c1eee1d 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1648,20 +1648,20 @@ EXPORT_SYMBOL(iunique);
>  
>  struct inode *igrab(struct inode *inode)
>  {
> +	lockdep_assert_not_held(&inode->i_lock);
> +
> +	inode = inode_tryget(inode);
> +	if (!inode)
> +		return NULL;
> +
> +	/*
> +	 * If this inode is on the LRU, take it off so that we can re-run the
> +	 * LRU logic on the next iput().
> +	 */
>  	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);
> -		/*
> -		 * Handle the case where s_op->clear_inode is not been
> -		 * called yet, and somebody is calling igrab
> -		 * while the inode is getting freed.
> -		 */
> -		inode = NULL;
> -	}
> +	inode_lru_list_del(inode);
> +	spin_unlock(&inode->i_lock);
> +
>  	return inode;
>  }
>  EXPORT_SYMBOL(igrab);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 34fb40ba8a94..b731224708be 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3393,6 +3393,33 @@ static inline void iobj_get(struct inode *inode)
>  	refcount_inc(&inode->i_obj_count);
>  }
>  
> +static inline struct inode *inode_tryget(struct inode *inode)
> +{
> +	/*
> +	 * We are using inode_tryget() because we're interested in getting a
> +	 * live reference to the inode, which is ->i_count. Normally we would
> +	 * grab i_obj_count first, as it is the highe priority reference.
> +	 * However we're only interested in making sure we have a live inode,
> +	 * and we know that if we get a reference for i_count then we can safely
> +	 * acquire i_obj_count because we always drop i_obj_count after dropping
> +	 * an i_count reference.
> +	 *
> +	 * This is meant to be used either in a place where we have an existing
> +	 * i_obj_count reference on the inode, or under rcu_read_lock() so we
> +	 * know we're safe in accessing this inode still.

Maybe add a debug assert to that effect?

VFS_WAR_ON_ONCE(!icount_read(inode) && !rcu_read_lock_held());

> +	 */
> +	if (!refcount_inc_not_zero(&inode->i_count)) {
> +		/*
> +		 * If we failed to increment the reference count, then the
> +		 * inode is being freed or has been freed.  We return NULL
> +		 * in this case.
> +		 */
> +		return NULL;
> +	}

I would invert the logic here?

	if (refcount_inc_not_zero()) {
		iobj_get(inode);
		return inode;
	}

        /*
         * If we failed to increment the reference count, then the
         * inode is being freed or has been freed.  We return NULL
         * in this case.
         */
	return NULL;