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);
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>
>
>