| Message ID | 20220718063410.338626-9-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] md: fix mddev->kobj lifetime | expand |
On 7/18/22 08:34, Christoph Hellwig wrote: > Just do a simple list_for_each_entry_safe on all_mddevs, and only grab a > reference when we drop the lock and delete the now unused for_each_mddev > macro. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/md/md.c | 39 +++++++++++---------------------------- > 1 file changed, 11 insertions(+), 28 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 44e4071b43148..805f2b4ed9c0d 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -368,28 +368,6 @@ EXPORT_SYMBOL_GPL(md_new_event); > static LIST_HEAD(all_mddevs); > static DEFINE_SPINLOCK(all_mddevs_lock); > > -/* > - * iterates through all used mddevs in the system. > - * We take care to grab the all_mddevs_lock whenever navigating > - * the list, and to always hold a refcount when unlocked. > - * Any code which breaks out of this loop while own > - * a reference to the current mddev and must mddev_put it. > - */ > -#define for_each_mddev(_mddev,_tmp) \ > - \ > - for (({ spin_lock(&all_mddevs_lock); \ > - _tmp = all_mddevs.next; \ > - _mddev = NULL;}); \ > - ({ if (_tmp != &all_mddevs) \ > - mddev_get(list_entry(_tmp, struct mddev, all_mddevs));\ > - spin_unlock(&all_mddevs_lock); \ > - if (_mddev) mddev_put(_mddev); \ > - _mddev = list_entry(_tmp, struct mddev, all_mddevs); \ > - _tmp != &all_mddevs;}); \ > - ({ spin_lock(&all_mddevs_lock); \ > - _tmp = _tmp->next;}) \ > - ) > - > /* Rather than calling directly into the personality make_request function, > * IO requests come here first so that we can check if the device is > * being suspended pending a reconfiguration. > @@ -9925,8 +9903,7 @@ void md_autostart_arrays(int part) > > static __exit void md_exit(void) > { > - struct mddev *mddev; > - struct list_head *tmp; > + struct mddev *mddev, *n; > int delay = 1; > > unregister_blkdev(MD_MAJOR,"md"); > @@ -9946,17 +9923,23 @@ static __exit void md_exit(void) > } > remove_proc_entry("mdstat", NULL); > > - for_each_mddev(mddev, tmp) { > + spin_lock(&all_mddevs_lock); > + list_for_each_entry_safe(mddev, n, &all_mddevs, all_mddevs) { > + mddev_get(mddev); > + spin_unlock(&all_mddevs_lock); > export_array(mddev); > mddev->ctime = 0; > mddev->hold_active = 0; > /* > - * for_each_mddev() will call mddev_put() at the end of each > - * iteration. As the mddev is now fully clear, this will > - * schedule the mddev for destruction by a workqueue, and the > + * As the mddev is now fully clear, mddev_put will schedule > + * the mddev for destruction by a workqueue, and the > * destroy_workqueue() below will wait for that to complete. > */ > + mddev_put(mddev); > + spin_lock(&all_mddevs_lock); > } > + spin_unlock(&all_mddevs_lock); > + > destroy_workqueue(md_rdev_misc_wq); > destroy_workqueue(md_misc_wq); > destroy_workqueue(md_wq); Having thought about it some more ... wouldn't it make sense to modify mddev_get() to if (atomic_inc_not_zero(&mddev->active)) return NULL; to ensure we're not missing any use-after-free issues, which previously would have been caught by the 'for_each_mddev()' macro? Cheers, Hannes
On 2022-07-18 00:34, Christoph Hellwig wrote: > Just do a simple list_for_each_entry_safe on all_mddevs, and only grab a > reference when we drop the lock and delete the now unused for_each_mddev > macro. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Logan
On Mon, Jul 18, 2022 at 09:10:59AM +0200, Hannes Reinecke wrote: > > Having thought about it some more ... wouldn't it make sense to modify > mddev_get() to > > if (atomic_inc_not_zero(&mddev->active)) > return NULL; > > to ensure we're not missing any use-after-free issues, which previously > would have been caught by the 'for_each_mddev()' macro? No. mddev->active = 0 can still be a perfectly valid mddev and they are not automatically deleted. Check the conditions in mddev_put.
On 7/19/22 09:03, Christoph Hellwig wrote: > On Mon, Jul 18, 2022 at 09:10:59AM +0200, Hannes Reinecke wrote: >> >> Having thought about it some more ... wouldn't it make sense to modify >> mddev_get() to >> >> if (atomic_inc_not_zero(&mddev->active)) >> return NULL; >> >> to ensure we're not missing any use-after-free issues, which previously >> would have been caught by the 'for_each_mddev()' macro? > > No. mddev->active = 0 can still be a perfectly valid mddev and they > are not automatically deleted. Check the conditions in mddev_put. Sigh. Okay. MD is a mess anyway. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
diff --git a/drivers/md/md.c b/drivers/md/md.c index 44e4071b43148..805f2b4ed9c0d 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -368,28 +368,6 @@ EXPORT_SYMBOL_GPL(md_new_event); static LIST_HEAD(all_mddevs); static DEFINE_SPINLOCK(all_mddevs_lock); -/* - * iterates through all used mddevs in the system. - * We take care to grab the all_mddevs_lock whenever navigating - * the list, and to always hold a refcount when unlocked. - * Any code which breaks out of this loop while own - * a reference to the current mddev and must mddev_put it. - */ -#define for_each_mddev(_mddev,_tmp) \ - \ - for (({ spin_lock(&all_mddevs_lock); \ - _tmp = all_mddevs.next; \ - _mddev = NULL;}); \ - ({ if (_tmp != &all_mddevs) \ - mddev_get(list_entry(_tmp, struct mddev, all_mddevs));\ - spin_unlock(&all_mddevs_lock); \ - if (_mddev) mddev_put(_mddev); \ - _mddev = list_entry(_tmp, struct mddev, all_mddevs); \ - _tmp != &all_mddevs;}); \ - ({ spin_lock(&all_mddevs_lock); \ - _tmp = _tmp->next;}) \ - ) - /* Rather than calling directly into the personality make_request function, * IO requests come here first so that we can check if the device is * being suspended pending a reconfiguration. @@ -9925,8 +9903,7 @@ void md_autostart_arrays(int part) static __exit void md_exit(void) { - struct mddev *mddev; - struct list_head *tmp; + struct mddev *mddev, *n; int delay = 1; unregister_blkdev(MD_MAJOR,"md"); @@ -9946,17 +9923,23 @@ static __exit void md_exit(void) } remove_proc_entry("mdstat", NULL); - for_each_mddev(mddev, tmp) { + spin_lock(&all_mddevs_lock); + list_for_each_entry_safe(mddev, n, &all_mddevs, all_mddevs) { + mddev_get(mddev); + spin_unlock(&all_mddevs_lock); export_array(mddev); mddev->ctime = 0; mddev->hold_active = 0; /* - * for_each_mddev() will call mddev_put() at the end of each - * iteration. As the mddev is now fully clear, this will - * schedule the mddev for destruction by a workqueue, and the + * As the mddev is now fully clear, mddev_put will schedule + * the mddev for destruction by a workqueue, and the * destroy_workqueue() below will wait for that to complete. */ + mddev_put(mddev); + spin_lock(&all_mddevs_lock); } + spin_unlock(&all_mddevs_lock); + destroy_workqueue(md_rdev_misc_wq); destroy_workqueue(md_misc_wq); destroy_workqueue(md_wq);
Just do a simple list_for_each_entry_safe on all_mddevs, and only grab a reference when we drop the lock and delete the now unused for_each_mddev macro. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/md.c | 39 +++++++++++---------------------------- 1 file changed, 11 insertions(+), 28 deletions(-)