linux-ext4 - Re: [PATCH 04/50] fs: hold an i_obj_count reference for the i_wb

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: <20250822-donnerstag-sowas-477e66bd0cf1@brauner>
Date: Fri, 22 Aug 2025 13:27:47 +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 04/50] fs: hold an i_obj_count reference for the i_wb_list

On Thu, Aug 21, 2025 at 04:18:15PM -0400, Josef Bacik wrote:
> If we're holding the inode on one of the writeback lists we need to have
> a reference on that inode. Grab a reference when we add i_wb_list to
> something, drop it when it's removed.
> 
> This is potentially dangerous, because we remove the inode from the
> i_wb_list potentially under IRQ via folio_end_writeback(). This will be
> mitigated by making sure all writeback is completed on the final iput,
> before the final iobj_put, preventing a potential free under IRQ.
> 
> Signed-off-by: Josef Bacik <josef@...icpanda.com>
> ---
>  fs/fs-writeback.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 001773e6e95c..c2437e3d320a 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1332,6 +1332,7 @@ void sb_mark_inode_writeback(struct inode *inode)
>  	if (list_empty(&inode->i_wb_list)) {
>  		spin_lock_irqsave(&sb->s_inode_wblist_lock, flags);
>  		if (list_empty(&inode->i_wb_list)) {
> +			iobj_get(inode);
>  			list_add_tail(&inode->i_wb_list, &sb->s_inodes_wb);
>  			trace_sb_mark_inode_writeback(inode);
>  		}
> @@ -1346,15 +1347,26 @@ void sb_clear_inode_writeback(struct inode *inode)
>  {
>  	struct super_block *sb = inode->i_sb;
>  	unsigned long flags;
> +	bool drop = false;
>  
>  	if (!list_empty(&inode->i_wb_list)) {
>  		spin_lock_irqsave(&sb->s_inode_wblist_lock, flags);
>  		if (!list_empty(&inode->i_wb_list)) {
> +			drop = true;
>  			list_del_init(&inode->i_wb_list);
>  			trace_sb_clear_inode_writeback(inode);
>  		}
>  		spin_unlock_irqrestore(&sb->s_inode_wblist_lock, flags);
>  	}
> +
> +	/*
> +	 * This can be called in IRQ context when we're clearing writeback on
> +	 * the folio. This should not be the last iobj_put() on the inode, we
> +	 * run all of the writeback before we free the inode in order to avoid
> +	 * this possibility.
> +	 */
> +	if (drop)
> +		iobj_put(inode);

In that case it might be valuable to have a:

VFS_WARN_ON_ONCE(refcount_read(&inode->i_obj_count) < 2);

before calling iobj_put() here? It'll compile out without
CONFIG_VFS_DEBUG set.

Btw, you should also be able to write this as removing the condition.

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 2e10cc2f955f..cfdb2c2793cb 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1366,13 +1366,13 @@ void sb_mark_inode_writeback(struct inode *inode)
 void sb_clear_inode_writeback(struct inode *inode)
 {
        struct super_block *sb = inode->i_sb;
+       struct inode *drop = NULL;
        unsigned long flags;
-       bool drop = false;

        if (!list_empty(&inode->i_wb_list)) {
                spin_lock_irqsave(&sb->s_inode_wblist_lock, flags);
                if (!list_empty(&inode->i_wb_list)) {
-                       drop = true;
+                       drop = inode;
                        list_del_init(&inode->i_wb_list);
                        trace_sb_clear_inode_writeback(inode);
                }
@@ -1385,8 +1385,7 @@ void sb_clear_inode_writeback(struct inode *inode)
         * run all of the writeback before we free the inode in order to avoid
         * this possibility.
         */
-       if (drop)
-               iobj_put(inode);
+       iobj_put(drop);
 }

>  }
>  
>  /*
> @@ -2683,6 +2695,8 @@ static void wait_sb_inodes(struct super_block *sb)
>  		 * to preserve consistency between i_wb_list and the mapping
>  		 * writeback tag. Writeback completion is responsible to remove
>  		 * the inode from either list once the writeback tag is cleared.
> +		 * At that point the i_obj_count reference will be dropped for
> +		 * the i_wb_list reference.
>  		 */
>  		list_move_tail(&inode->i_wb_list, &sb->s_inodes_wb);
>  
> -- 
> 2.49.0
>