| Message ID | 20240327155751.3536-2-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve visibility of writeback | expand |
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 >
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@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 >> > >
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);
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 <shikemeng@huaweicloud.com> --- mm/backing-dev.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)