On 2021/12/20 5:00, Luis Chamberlain wrote: > On Fri, Dec 17, 2021 at 07:37:43PM +0900, Tetsuo Handa wrote: >> I think we can apply this patch as-is... > > Unfortunately I don't think so, don't we end up still with a race > in between the first part of device_add() and the kobject_add() > which adds the kobject to generic layer and in return enables the > disk_release() call for the disk? I count 5 error paths in between > including kobject_add() which can fail as well. I can't catch which path you are talking about. Will you explain more details using call trace (or line numbers in https://elixir.bootlin.com/linux/v5.16-rc6/source/block/genhd.c#L397 ) ? > > If correct then something like the following may be needed: > > diff --git a/block/genhd.c b/block/genhd.c > index 3c139a1b6f04..08ab7ce63e57 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -539,9 +539,10 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > out_device_del: > device_del(ddev); > out_disk_release_events: > - disk_release_events(disk); > + if (!kobject_alive(&ddev->kobj)) > + disk_release_events(disk); > out_free_ext_minor: > - if (disk->major == BLOCK_EXT_MAJOR) > + if (!kobject_alive(&ddev->kobj) && disk->major == BLOCK_EXT_MAJOR) How can kobject_alive() matter? The minor id which was stored into disk->first_minor is allocated by https://elixir.bootlin.com/linux/v5.16-rc6/source/block/genhd.c#L432 , and that minor id is copied to bdev->bd_dev (which https://elixir.bootlin.com/linux/v5.16-rc6/source/block/bdev.c#L415 will release) by https://elixir.bootlin.com/linux/v5.16-rc6/source/block/genhd.c#L511 , doesn't it? Unless there is a location (except genhd.c#L511) which copies that minor id to bdev->bd_dev (which bdev.c#L415 will release), it is correct to unconditionally undo allocation by genhd.c#L432 at genhd.c#L546 . Will you explain what path you are talking about?
