linux-kernel - Re: [PATCH v2 1/6] writeback: protect race between bdi release and bdi_debug_stats

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: <7232e82a-5073-003c-539f-f4627a9ee688@huaweicloud.com>
Date: Wed, 3 Apr 2024 10:16:22 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Brian Foster <bfoster@...hat.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, jack@...e.cz,
 tj@...nel.org, dsterba@...e.com, mjguzik@...il.com, dhowells@...hat.com,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] writeback: protect race between bdi release and
 bdi_debug_stats_show



on 3/29/2024 1:53 AM, Brian Foster wrote:
> 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?
Sorry for missing this. The race seems not possible if debugfs could prevent
this. Thanks for the information. I will drop this in next version.

Kemeng

> 
> 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@...weicloud.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
>>
> 
>