| Message ID | 1513069072-32514-2-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2017-12-12 at 09:57 +0100, Hannes Reinecke wrote: > From: Hannes Reinecke <hare@suse.com> > > When a device is probed asynchronously del_gendisk() might be called > before the async probing was run, causing del_gendisk() to crash > due to uninitialized sysfs objects. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > block/genhd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/genhd.c b/block/genhd.c > index c2223f1..cc40d95 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -697,6 +697,9 @@ void del_gendisk(struct gendisk *disk) > struct disk_part_iter piter; > struct hd_struct *part; > > + if (!(disk->flags & GENHD_FL_UP)) > + return; > + > blk_integrity_del(disk); > disk_del_events(disk); Hello Hannes, Thank you for having published your approach for increasing disk probing concurrency. Your approach looks interesting to me. However, I don't think that patches 1/4..3/4 are sufficient to avoid races between e.g. device_add_disk() and del_gendisk(). As far as I know no locks are held around the device_add_disk() and del_gendisk() calls. Does that mean that del_gendisk() can call e.g. blk_integrity_del() before device_add_disk() has called blk_integrity_add()? Bart.
On 12/12/2017 05:57 PM, Bart Van Assche wrote: > On Tue, 2017-12-12 at 09:57 +0100, Hannes Reinecke wrote: >> From: Hannes Reinecke <hare@suse.com> >> >> When a device is probed asynchronously del_gendisk() might be called >> before the async probing was run, causing del_gendisk() to crash >> due to uninitialized sysfs objects. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> block/genhd.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/block/genhd.c b/block/genhd.c >> index c2223f1..cc40d95 100644 >> --- a/block/genhd.c >> +++ b/block/genhd.c >> @@ -697,6 +697,9 @@ void del_gendisk(struct gendisk *disk) >> struct disk_part_iter piter; >> struct hd_struct *part; >> >> + if (!(disk->flags & GENHD_FL_UP)) >> + return; >> + >> blk_integrity_del(disk); >> disk_del_events(disk); > > Hello Hannes, > > Thank you for having published your approach for increasing disk probing > concurrency. Your approach looks interesting to me. However, I don't think > that patches 1/4..3/4 are sufficient to avoid races between e.g. > device_add_disk() and del_gendisk(). As far as I know no locks are held > around the device_add_disk() and del_gendisk() calls. Does that mean that > del_gendisk() can call e.g. blk_integrity_del() before device_add_disk() has > called blk_integrity_add()? > In principle, yes. However, the overall idea here is that device_add_disk() and del_gendisk() are enclosed within upper layer procedures, which themselves provide additional locking. In our case the sd driver provided synchronisation guarantees ensuring that device_add_disk() and del_gendisk() doesn't run concurrently. if one is really concerned we could convert disk->flags to a bitmask, and use atomic bitmask modification; that should avoid any concurrency issues. However, I'm not sure if it's worth it. Cheers, Hannes
On Wed, 2017-12-13 at 08:36 +0100, Hannes Reinecke wrote: > On 12/12/2017 05:57 PM, Bart Van Assche wrote: > > On Tue, 2017-12-12 at 09:57 +0100, Hannes Reinecke wrote: > > > From: Hannes Reinecke <hare@suse.com> > > > > > > When a device is probed asynchronously del_gendisk() might be called > > > before the async probing was run, causing del_gendisk() to crash > > > due to uninitialized sysfs objects. > > > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > > --- > > > block/genhd.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/block/genhd.c b/block/genhd.c > > > index c2223f1..cc40d95 100644 > > > --- a/block/genhd.c > > > +++ b/block/genhd.c > > > @@ -697,6 +697,9 @@ void del_gendisk(struct gendisk *disk) > > > struct disk_part_iter piter; > > > struct hd_struct *part; > > > > > > + if (!(disk->flags & GENHD_FL_UP)) > > > + return; > > > + > > > blk_integrity_del(disk); > > > disk_del_events(disk); > > > > Hello Hannes, > > > > Thank you for having published your approach for increasing disk probing > > concurrency. Your approach looks interesting to me. However, I don't think > > that patches 1/4..3/4 are sufficient to avoid races between e.g. > > device_add_disk() and del_gendisk(). As far as I know no locks are held > > around the device_add_disk() and del_gendisk() calls. Does that mean that > > del_gendisk() can call e.g. blk_integrity_del() before device_add_disk() has > > called blk_integrity_add()? > > In principle, yes. However, the overall idea here is that device_add_disk() > and del_gendisk() are enclosed within upper layer procedures, which themselves > provide additional locking. In our case the sd driver provided synchronisation > guarantees ensuring that device_add_disk() and del_gendisk() doesn't run > concurrently. > > if one is really concerned we could convert disk->flags to a bitmask, and use > atomic bitmask modification; that should avoid any concurrency issues. Hello Hannes, Regarding the scenario explained in a previous e-mail: what guarantees that the device_add_disk() call in sd_probe_async() does not happen concurrently with the device_unregister() call from __scsi_remove_device()? Thanks, Bart.
diff --git a/block/genhd.c b/block/genhd.c index c2223f1..cc40d95 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -697,6 +697,9 @@ void del_gendisk(struct gendisk *disk) struct disk_part_iter piter; struct hd_struct *part; + if (!(disk->flags & GENHD_FL_UP)) + return; + blk_integrity_del(disk); disk_del_events(disk);