Received-SPF: pass (google.com: domain of linux-kernel+bounces-123378-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 28 Mar 2024 13:53:31 -0400
From: Brian Foster <bfoster@redhat.com>
To: Kemeng Shi <shikemeng@huaweicloud.com>
Cc: akpm@linux-foundation.org, willy@infradead.org, jack@suse.cz,
	tj@kernel.org, dsterba@suse.com, mjguzik@gmail.com,
	dhowells@redhat.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 1/6] writeback: protect race between bdi release and
 bdi_debug_stats_show
Message-ID: <ZgWum7SWr44w0rie@bfoster>
References: <20240327155751.3536-1-shikemeng@huaweicloud.com>
 <20240327155751.3536-2-shikemeng@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240327155751.3536-2-shikemeng@huaweicloud.com>

On Wed, Mar 27, 2024 at 11:57:46PM +0800, Kemeng Shi wrote:
> There is a race between bdi release and bdi_debug_stats_show:
> /* get debug info */		/* bdi release */
> bdi_debug_stats_show
>   bdi = m->private;
>   wb = &bdi->wb;
> 				bdi_unregister
> 				bdi_put
> 				  release_bdi
> 				    kfree(bdi)
>   /* use after free */
>   spin_lock(&wb->list_lock);
> 

Maybe I'm missing something, but it looks to me that
bdi_unregister_debug() can't complete until active readers of associated
debugfs files have completed. For example, see __debugfs_file_removed()
and use of ->active_users[_drained]. Once the dentry is unlinked,
further reads fail (I think) via debugfs_file_get(). Hm?

Brian

> Search bdi on bdi_list under rcu lock in bdi_debug_stats_show to ensure
> the bdi is not freed to fix the issue.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  mm/backing-dev.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 5f2be8c8df11..70f02959f3bd 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -46,16 +46,44 @@ static void bdi_debug_init(void)
>  	bdi_debug_root = debugfs_create_dir("bdi", NULL);
>  }
>  
> -static int bdi_debug_stats_show(struct seq_file *m, void *v)
> +static struct backing_dev_info *lookup_bdi(struct seq_file *m)
>  {
> +	const struct file *file = m->file;
>  	struct backing_dev_info *bdi = m->private;
> -	struct bdi_writeback *wb = &bdi->wb;
> +	struct backing_dev_info *exist;
> +
> +	list_for_each_entry_rcu(exist, &bdi_list, bdi_list) {
> +		if (exist != bdi)
> +			continue;
> +
> +		if (exist->debug_dir == file->f_path.dentry->d_parent)
> +			return bdi;
> +		else
> +			return NULL;
> +	}
> +
> +	return NULL;
> +}
> +
> +
> +static int bdi_debug_stats_show(struct seq_file *m, void *v)
> +{
> +	struct backing_dev_info *bdi;
> +	struct bdi_writeback *wb;
>  	unsigned long background_thresh;
>  	unsigned long dirty_thresh;
>  	unsigned long wb_thresh;
>  	unsigned long nr_dirty, nr_io, nr_more_io, nr_dirty_time;
>  	struct inode *inode;
>  
> +	rcu_read_lock();
> +	bdi = lookup_bdi(m);
> +	if (!bdi) {
> +		rcu_read_unlock();
> +		return -EEXIST;
> +	}
> +
> +	wb = &bdi->wb;
>  	nr_dirty = nr_io = nr_more_io = nr_dirty_time = 0;
>  	spin_lock(&wb->list_lock);
>  	list_for_each_entry(inode, &wb->b_dirty, i_io_list)
> @@ -101,6 +129,7 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
>  		   nr_dirty_time,
>  		   !list_empty(&bdi->bdi_list), bdi->wb.state);
>  
> +	rcu_read_unlock();
>  	return 0;
>  }
>  DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
> -- 
> 2.30.0
>