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

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

Message-ID: <20250826-benimm-muster-781f3fa24fe8@brauner>
Date: Tue, 26 Aug 2025 11:28:05 +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 Mon, Aug 25, 2025 at 03:26:10PM -0400, Josef Bacik wrote:
> On Mon, Aug 25, 2025 at 12:54:01PM +0200, Christian Brauner wrote:
> > 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.
> 
> Not necsessarily, maybe we had some delayed iput (*cough* btrfs *cough*) that
> didn't run until umount time and now we have true coming from ->drop_inode()
> with SB_ACTIVE turned off.  That would be completely valid.  Thanks,

Ah, right, thanks! Yeah, that's seems legit.