[PATCH 0/3] close udev startup race condition for several devices

[PATCH 0/3] close udev startup race condition for several devices

[Jeffle Xu]

The upstream commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 ("block:
genhd: add 'groups' argument to device_add_disk") and the following
patches fix a race condition of udev for several devices, including
nvme, aoe, zram and virtio.

The stable tree commit 9e07f4e243791e00a4086ad86e573705cf7b2c65("zram:
close udev startup race condition as default groups") only fixes zram,
leaving other devices unfixed.

This udev race issue indeed makes trouble. We recently found that this
issue can cause missing '/dev/disk/by-id/XXXX' symlink of virtio-blk
devices on 4.19.

Be noted that this patch set follows the idea of stable commit
9e07f4e243791e00a4086ad86e573705cf7b2c65 ("zram: close udev startup race
condition as default groups") of merging the preparation patch (commit
fef912bf860e) and the fixing patch (commit 98af4d4df889).

Jeffle Xu (3):
  virtio-blk: close udev startup race condition as default groups
  aoe: close udev startup race condition as default groups
  nvme: close udev startup race condition as default groups

 drivers/block/aoe/aoe.h       |   1 -
 drivers/block/aoe/aoeblk.c    |  20 +++----
 drivers/block/aoe/aoedev.c    |   1 -
 drivers/block/virtio_blk.c    |  67 +++++++++++++---------
 drivers/nvme/host/core.c      |  20 +++----
 drivers/nvme/host/lightnvm.c  | 105 ++++++++++++++--------------------
 drivers/nvme/host/multipath.c |  10 +---
 drivers/nvme/host/nvme.h      |  10 +---
 8 files changed, 103 insertions(+), 131 deletions(-)

-- 
2.27.0

[PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[Jeffle Xu]

commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 upstream.

Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
as default groups"), this is a merge of [1, 2], since [1] may be too
large size to be merged into -stable tree.

This issue has been introduced since v2.6.36 by [3].

[1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
[2] e982c4d0a29b, virtio-blk: modernize sysfs attribute creation
[3] a5eb9e4ff18a, virtio_blk: Add 'serial' attribute to virtio-blk devices (v2)

Cc: Hannes Reinecke <hare@suse.de>
Cc: stable@vger.kernel.org # 4.4+
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 drivers/block/virtio_blk.c | 67 ++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 075523777a4a..52e3327581f9 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -423,8 +423,8 @@ static int minor_to_index(int minor)
 	return minor >> PART_BITS;
 }
 
-static ssize_t virtblk_serial_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
+static ssize_t serial_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 	int err;
@@ -443,7 +443,7 @@ static ssize_t virtblk_serial_show(struct device *dev,
 	return err;
 }
 
-static DEVICE_ATTR(serial, 0444, virtblk_serial_show, NULL);
+static DEVICE_ATTR_RO(serial);
 
 /* The queue's logical block size must be set before calling this */
 static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize)
@@ -619,8 +619,8 @@ static const char *const virtblk_cache_types[] = {
 };
 
 static ssize_t
-virtblk_cache_type_store(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
+cache_type_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 	struct virtio_blk *vblk = disk->private_data;
@@ -638,8 +638,7 @@ virtblk_cache_type_store(struct device *dev, struct device_attribute *attr,
 }
 
 static ssize_t
-virtblk_cache_type_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
+cache_type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 	struct virtio_blk *vblk = disk->private_data;
@@ -649,12 +648,38 @@ virtblk_cache_type_show(struct device *dev, struct device_attribute *attr,
 	return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]);
 }
 
-static const struct device_attribute dev_attr_cache_type_ro =
-	__ATTR(cache_type, 0444,
-	       virtblk_cache_type_show, NULL);
-static const struct device_attribute dev_attr_cache_type_rw =
-	__ATTR(cache_type, 0644,
-	       virtblk_cache_type_show, virtblk_cache_type_store);
+static DEVICE_ATTR_RW(cache_type);
+
+static struct attribute *virtblk_attrs[] = {
+	&dev_attr_serial.attr,
+	&dev_attr_cache_type.attr,
+	NULL,
+};
+
+static umode_t virtblk_attrs_are_visible(struct kobject *kobj,
+		struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct gendisk *disk = dev_to_disk(dev);
+	struct virtio_blk *vblk = disk->private_data;
+	struct virtio_device *vdev = vblk->vdev;
+
+	if (a == &dev_attr_cache_type.attr &&
+	    !virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
+		return S_IRUGO;
+
+	return a->mode;
+}
+
+static const struct attribute_group virtblk_attr_group = {
+	.attrs = virtblk_attrs,
+	.is_visible = virtblk_attrs_are_visible,
+};
+
+static const struct attribute_group *virtblk_attr_groups[] = {
+	&virtblk_attr_group,
+	NULL,
+};
 
 static int virtblk_init_request(struct blk_mq_tag_set *set, struct request *rq,
 		unsigned int hctx_idx, unsigned int numa_node)
@@ -858,24 +883,10 @@ static int virtblk_probe(struct virtio_device *vdev)
 	virtblk_update_capacity(vblk, false);
 	virtio_device_ready(vdev);
 
+	disk_to_dev(vblk->disk)->groups = virtblk_attr_groups;
 	device_add_disk(&vdev->dev, vblk->disk);
-	err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
-	if (err)
-		goto out_del_disk;
-
-	if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
-		err = device_create_file(disk_to_dev(vblk->disk),
-					 &dev_attr_cache_type_rw);
-	else
-		err = device_create_file(disk_to_dev(vblk->disk),
-					 &dev_attr_cache_type_ro);
-	if (err)
-		goto out_del_disk;
 	return 0;
 
-out_del_disk:
-	del_gendisk(vblk->disk);
-	blk_cleanup_queue(vblk->disk->queue);
 out_free_tags:
 	blk_mq_free_tag_set(&vblk->tag_set);
 out_put_disk:
-- 
2.27.0

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[Greg KH]

On Sun, Feb 07, 2021 at 07:46:54PM +0800, Jeffle Xu wrote:
> commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
> commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 upstream.
> 
> Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
> as default groups"), this is a merge of [1, 2], since [1] may be too
> large size to be merged into -stable tree.

Why is it too big?

> This issue has been introduced since v2.6.36 by [3].
> 
> [1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
> [2] e982c4d0a29b, virtio-blk: modernize sysfs attribute creation
> [3] a5eb9e4ff18a, virtio_blk: Add 'serial' attribute to virtio-blk devices (v2)

What userspace tools are now hitting this issue?  If it's a real
problem, let's take the real commits, right?

Same for the other patches in this series.

thanks,

greg k-h

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[Sasha Levin]

On Sun, Feb 07, 2021 at 12:56:49PM +0100, Greg KH wrote:
>On Sun, Feb 07, 2021 at 07:46:54PM +0800, Jeffle Xu wrote:
>> commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
>> commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 upstream.
>>
>> Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
>> as default groups"), this is a merge of [1, 2], since [1] may be too
>> large size to be merged into -stable tree.
>
>Why is it too big?

I took a "merged" fix from Minchan (over 2 years ago!) not because the
related genhd changed are "too big" but rather "too invasive". When I
reviewed that patched it seemed to me that the additional changes it
needed for backport weren't worth it as most of the code it touches is
irrelevant for this fix.

But if now we're going to be getting more fixes that rely on that genhd
patch then let's revert the previous fix and take all the patches
separately.

-- 
Thanks,
Sasha

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[JeffleXu]

On 2/7/21 7:56 PM, Greg KH wrote:
> On Sun, Feb 07, 2021 at 07:46:54PM +0800, Jeffle Xu wrote:
>> commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
>> commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 upstream.
>>
>> Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
>> as default groups"), this is a merge of [1, 2], since [1] may be too
>> large size to be merged into -stable tree.
> 
> Why is it too big?
> 
>> This issue has been introduced since v2.6.36 by [3].
>>
>> [1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
>> [2] e982c4d0a29b, virtio-blk: modernize sysfs attribute creation
>> [3] a5eb9e4ff18a, virtio_blk: Add 'serial' attribute to virtio-blk devices (v2)
> 
> What userspace tools are now hitting this issue?  If it's a real
> problem, let's take the real commits, right?
> 
> Same for the other patches in this series.
> 

udevd hits this issue. systemd-udevd is responsible for creating
symlinks, such as '/dev/disk/by-id/XXXX' when receiving KOBJ_ADD uevent
from kernel space. For virtio-blk devices, udevd will read
'/sys/devices/pciXXXX/block/serial' to acquire the corresponding serial
id of the virtio-blk device.

Without this fixing patch, '/sys/devices/pciXXXX/block/serial' is
created after the KOBJ_ADD uevent. Thus when udevd received KOBJ_ADD
uevent, it may find that '/sys/devices/pciXXXX/block/serial' doesn't
exist at that time, and finally failed to create '/dev/disk/by-id/XXXX'
symlink.

I found several similar posts on internet, complaining
'/dev/disk/by-id/XXXX' is not created for virtio-blk devices. This issue
indeed is caused by some sort of race condition. I also found that
systemd-239 doesn't have this issue, while systemd-219 has. But we
didn't find the exact fixing commit for this issue for systemd-239 though.

-- 
Thanks,
Jeffle

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[JeffleXu]

Hi all,

Would you please evaluate if these should be fixed in stable tree, at
least for the virtio-blk scenario [1] ?

[1] commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 ("virtio-blk:
modernize sysfs attribute creation")

On 2/8/21 9:40 AM, JeffleXu wrote:
> 
> 
> On 2/7/21 7:56 PM, Greg KH wrote:
>> On Sun, Feb 07, 2021 at 07:46:54PM +0800, Jeffle Xu wrote:
>>> commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
>>> commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 upstream.
>>>
>>> Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
>>> as default groups"), this is a merge of [1, 2], since [1] may be too
>>> large size to be merged into -stable tree.
>>
>> Why is it too big?
>>
>>> This issue has been introduced since v2.6.36 by [3].
>>>
>>> [1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
>>> [2] e982c4d0a29b, virtio-blk: modernize sysfs attribute creation
>>> [3] a5eb9e4ff18a, virtio_blk: Add 'serial' attribute to virtio-blk devices (v2)
>>
>> What userspace tools are now hitting this issue?  If it's a real
>> problem, let's take the real commits, right?
>>
>> Same for the other patches in this series.
>>
> 
> udevd hits this issue. systemd-udevd is responsible for creating
> symlinks, such as '/dev/disk/by-id/XXXX' when receiving KOBJ_ADD uevent
> from kernel space. For virtio-blk devices, udevd will read
> '/sys/devices/pciXXXX/block/serial' to acquire the corresponding serial
> id of the virtio-blk device.
> 
> Without this fixing patch, '/sys/devices/pciXXXX/block/serial' is
> created after the KOBJ_ADD uevent. Thus when udevd received KOBJ_ADD
> uevent, it may find that '/sys/devices/pciXXXX/block/serial' doesn't
> exist at that time, and finally failed to create '/dev/disk/by-id/XXXX'
> symlink.
> 
> I found several similar posts on internet, complaining
> '/dev/disk/by-id/XXXX' is not created for virtio-blk devices. This issue
> indeed is caused by some sort of race condition. I also found that
> systemd-239 doesn't have this issue, while systemd-219 has. But we
> didn't find the exact fixing commit for this issue for systemd-239 though.
> 

-- 
Thanks,
Jeffle

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Wed, Feb 17, 2021 at 09:12:38PM +0800, JeffleXu wrote:
> Hi all,
> 
> Would you please evaluate if these should be fixed in stable tree, at
> least for the virtio-blk scenario [1] ?

What is "these"?

> [1] commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 ("virtio-blk:
> modernize sysfs attribute creation")

Do you want this backported?  To where?  Why?  If so, where is the
working backport that you have properly tested?

totally confused,

greg k-h

Re: [PATCH 1/3] virtio-blk: close udev startup race condition as default groups

[JeffleXu]

On 2/17/21 9:26 PM, Greg KH wrote:
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> http://daringfireball.net/2007/07/on_top

Sorry for the inconvenience. I reply in the topmost because I reply to
the email of myself and want to discuss something overall, i.e. if this
issue needs to be fixed in stable tree.


> On Wed, Feb 17, 2021 at 09:12:38PM +0800, JeffleXu wrote:
>> Hi all,
>>
>> Would you please evaluate if these should be fixed in stable tree, at
>> least for the virtio-blk scenario [1] ?
> 
> What is "these"?

I think I have clarified in the previous mail [1]. And yes it was
already one week ago and the context seems a little confusing here.
Sorry for that. In short, the symlink file '/dev/disk/by-id/XXXX' can't
be created for virtio-blk devices, which could be fixed by [2].

> 
>> [1] commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 ("virtio-blk:
>> modernize sysfs attribute creation")
> 
> Do you want this backported?

Yes, better to have it backported. I can maintain the fix as a private
patch in my 4.19 repository. I request to backport it into 4.19 stable
tree, bacause I think 4.19 stable tree may also suffers this issue.


>  To where?  

At least 4.19 stable tree, though all code previous 4.20 may also
suffers, since this is fixed in 4.20 upstream.


> Why?

Explained in [1].


> If so, where is the working backport that you have properly tested?

I want to backport the upstream patch (commit fef912bf860e and
e982c4d0a29b).

Sasha ever picked up another patch ([3]) from the same upstream patch
set [4], and manually reorganized a little. The reason is explained in [5].

These two patches (commit fef912bf860e and e982c4d0a29b) could be
directly applied to 4.19 stable tree. But to backport these two patches,
like Sasha said in [5], we need to revert the previous patch that Sasha
backported, and apply the upstream version.

I'm not sure if I shall send the patch (since I'm not the author of the
upstream patch), or the maintainer apply the patch directly.



[1] https://www.spinics.net/lists/stable/msg442203.html
[2] commit e982c4d0a29b1d61fbe7716a8dcf8984936d6730 ("virtio-blk:
modernize sysfs attribute creation")
[3]
https://patchwork.kernel.org/project/linux-block/patch/20180905070053.26239-5-hare@suse.de/
[4]
https://patchwork.kernel.org/project/linux-block/cover/20180905070053.26239-1-hare@suse.de/
[5] https://www.spinics.net/lists/stable/msg442196.html

-- 
Thanks,
Jeffle

[PATCH 2/3] aoe: close udev startup race condition as default groups

[Jeffle Xu]

commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
commit 95cf7809bf9169fec4e4b7bb24b8069d8f354f96 upstream.

Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
as default groups"), this is a merge of [1, 2], since [1] may be too
large size to be merged into -stable tree.

[1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
[2] 95cf7809bf91, aoe: register default groups with device_add_disk()

Cc: Hannes Reinecke <hare@suse.de>
Cc: stable@vger.kernel.org # 4.4+
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 drivers/block/aoe/aoe.h    |  1 -
 drivers/block/aoe/aoeblk.c | 20 +++++++-------------
 drivers/block/aoe/aoedev.c |  1 -
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index c0ebda1283cc..015c68017a1c 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -201,7 +201,6 @@ int aoeblk_init(void);
 void aoeblk_exit(void);
 void aoeblk_gdalloc(void *);
 void aoedisk_rm_debugfs(struct aoedev *d);
-void aoedisk_rm_sysfs(struct aoedev *d);
 
 int aoechr_init(void);
 void aoechr_exit(void);
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 429ebb84b592..769f3a10054e 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -177,10 +177,15 @@ static struct attribute *aoe_attrs[] = {
 	NULL,
 };
 
-static const struct attribute_group attr_group = {
+static const struct attribute_group aoe_attr_group = {
 	.attrs = aoe_attrs,
 };
 
+static const struct attribute_group *aoe_attr_groups[] = {
+	&aoe_attr_group,
+	NULL,
+};
+
 static const struct file_operations aoe_debugfs_fops = {
 	.open = aoe_debugfs_open,
 	.read = seq_read,
@@ -219,17 +224,6 @@ aoedisk_rm_debugfs(struct aoedev *d)
 	d->debugfs = NULL;
 }
 
-static int
-aoedisk_add_sysfs(struct aoedev *d)
-{
-	return sysfs_create_group(&disk_to_dev(d->gd)->kobj, &attr_group);
-}
-void
-aoedisk_rm_sysfs(struct aoedev *d)
-{
-	sysfs_remove_group(&disk_to_dev(d->gd)->kobj, &attr_group);
-}
-
 static int
 aoeblk_open(struct block_device *bdev, fmode_t mode)
 {
@@ -417,8 +411,8 @@ aoeblk_gdalloc(void *vp)
 
 	spin_unlock_irqrestore(&d->lock, flags);
 
+	disk_to_dev(gd)->groups = aoe_attr_groups;
 	add_disk(gd);
-	aoedisk_add_sysfs(d);
 	aoedisk_add_debugfs(d);
 
 	spin_lock_irqsave(&d->lock, flags);
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index 41060e9cedf2..f29a140cdbc1 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -275,7 +275,6 @@ freedev(struct aoedev *d)
 	del_timer_sync(&d->timer);
 	if (d->gd) {
 		aoedisk_rm_debugfs(d);
-		aoedisk_rm_sysfs(d);
 		del_gendisk(d->gd);
 		put_disk(d->gd);
 		blk_cleanup_queue(d->blkq);
-- 
2.27.0

[PATCH 3/3] nvme: close udev startup race condition as default groups

[Jeffle Xu]

commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 upstream.
commit 33b14f67a4e1eabd219fd6543da8f15ed86b641c upstream.

Similar to commit 9e07f4e24379 ("zram: close udev startup race condition
as default groups"), this is a merge of [1, 2], since [1] may be too
large size to be merged into -stable tree.

[1] fef912bf860e, block: genhd: add 'groups' argument to device_add_disk
[2] 33b14f67a4e1, nvme: register ns_id attributes as default sysfs groups

Cc: Hannes Reinecke <hare@suse.de>
Cc: stable@vger.kernel.org # 4.4+
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 drivers/nvme/host/core.c      |  20 +++----
 drivers/nvme/host/lightnvm.c  | 105 ++++++++++++++--------------------
 drivers/nvme/host/multipath.c |  10 +---
 drivers/nvme/host/nvme.h      |  10 +---
 4 files changed, 57 insertions(+), 88 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b633ea40430e..52d07784c1f5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2842,6 +2842,14 @@ const struct attribute_group nvme_ns_id_attr_group = {
 	.is_visible	= nvme_ns_id_attrs_are_visible,
 };
 
+const struct attribute_group *nvme_ns_id_attr_groups[] = {
+	&nvme_ns_id_attr_group,
+#ifdef CONFIG_NVM
+	&nvme_nvm_attr_group,
+#endif
+	NULL,
+};
+
 #define nvme_show_str_function(field)						\
 static ssize_t  field##_show(struct device *dev,				\
 			    struct device_attribute *attr, char *buf)		\
@@ -3211,14 +3219,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 	nvme_get_ctrl(ctrl);
 
+	disk_to_dev(ns->disk)->groups = nvme_ns_id_attr_groups;
 	device_add_disk(ctrl->device, ns->disk);
-	if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvme_ns_id_attr_group))
-		pr_warn("%s: failed to create sysfs group for identification\n",
-			ns->disk->disk_name);
-	if (ns->ndev && nvme_nvm_register_sysfs(ns))
-		pr_warn("%s: failed to register lightnvm sysfs group for identification\n",
-			ns->disk->disk_name);
 
 	nvme_mpath_add_disk(ns, id);
 	nvme_fault_inject_init(ns);
@@ -3252,10 +3254,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	synchronize_srcu(&ns->head->srcu); /* wait for concurrent submissions */
 
 	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvme_ns_id_attr_group);
-		if (ns->ndev)
-			nvme_nvm_unregister_sysfs(ns);
 		del_gendisk(ns->disk);
 		blk_cleanup_queue(ns->queue);
 		if (blk_get_integrity(ns->disk))
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index a69553e75f38..d10257b9c523 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -1193,10 +1193,29 @@ static NVM_DEV_ATTR_12_RO(multiplane_modes);
 static NVM_DEV_ATTR_12_RO(media_capabilities);
 static NVM_DEV_ATTR_12_RO(max_phys_secs);
 
-static struct attribute *nvm_dev_attrs_12[] = {
+/* 2.0 values */
+static NVM_DEV_ATTR_20_RO(groups);
+static NVM_DEV_ATTR_20_RO(punits);
+static NVM_DEV_ATTR_20_RO(chunks);
+static NVM_DEV_ATTR_20_RO(clba);
+static NVM_DEV_ATTR_20_RO(ws_min);
+static NVM_DEV_ATTR_20_RO(ws_opt);
+static NVM_DEV_ATTR_20_RO(maxoc);
+static NVM_DEV_ATTR_20_RO(maxocpu);
+static NVM_DEV_ATTR_20_RO(mw_cunits);
+static NVM_DEV_ATTR_20_RO(write_typ);
+static NVM_DEV_ATTR_20_RO(write_max);
+static NVM_DEV_ATTR_20_RO(reset_typ);
+static NVM_DEV_ATTR_20_RO(reset_max);
+
+static struct attribute *nvm_dev_attrs[] = {
+	/* version agnostic attrs */
 	&dev_attr_version.attr,
 	&dev_attr_capabilities.attr,
+	&dev_attr_read_typ.attr,
+	&dev_attr_read_max.attr,
 
+	/* 1.2 attrs */
 	&dev_attr_vendor_opcode.attr,
 	&dev_attr_device_mode.attr,
 	&dev_attr_media_manager.attr,
@@ -1211,8 +1230,6 @@ static struct attribute *nvm_dev_attrs_12[] = {
 	&dev_attr_page_size.attr,
 	&dev_attr_hw_sector_size.attr,
 	&dev_attr_oob_sector_size.attr,
-	&dev_attr_read_typ.attr,
-	&dev_attr_read_max.attr,
 	&dev_attr_prog_typ.attr,
 	&dev_attr_prog_max.attr,
 	&dev_attr_erase_typ.attr,
@@ -1221,33 +1238,7 @@ static struct attribute *nvm_dev_attrs_12[] = {
 	&dev_attr_media_capabilities.attr,
 	&dev_attr_max_phys_secs.attr,
 
-	NULL,
-};
-
-static const struct attribute_group nvm_dev_attr_group_12 = {
-	.name		= "lightnvm",
-	.attrs		= nvm_dev_attrs_12,
-};
-
-/* 2.0 values */
-static NVM_DEV_ATTR_20_RO(groups);
-static NVM_DEV_ATTR_20_RO(punits);
-static NVM_DEV_ATTR_20_RO(chunks);
-static NVM_DEV_ATTR_20_RO(clba);
-static NVM_DEV_ATTR_20_RO(ws_min);
-static NVM_DEV_ATTR_20_RO(ws_opt);
-static NVM_DEV_ATTR_20_RO(maxoc);
-static NVM_DEV_ATTR_20_RO(maxocpu);
-static NVM_DEV_ATTR_20_RO(mw_cunits);
-static NVM_DEV_ATTR_20_RO(write_typ);
-static NVM_DEV_ATTR_20_RO(write_max);
-static NVM_DEV_ATTR_20_RO(reset_typ);
-static NVM_DEV_ATTR_20_RO(reset_max);
-
-static struct attribute *nvm_dev_attrs_20[] = {
-	&dev_attr_version.attr,
-	&dev_attr_capabilities.attr,
-
+	/* 2.0 attrs */
 	&dev_attr_groups.attr,
 	&dev_attr_punits.attr,
 	&dev_attr_chunks.attr,
@@ -1258,8 +1249,6 @@ static struct attribute *nvm_dev_attrs_20[] = {
 	&dev_attr_maxocpu.attr,
 	&dev_attr_mw_cunits.attr,
 
-	&dev_attr_read_typ.attr,
-	&dev_attr_read_max.attr,
 	&dev_attr_write_typ.attr,
 	&dev_attr_write_max.attr,
 	&dev_attr_reset_typ.attr,
@@ -1268,44 +1257,38 @@ static struct attribute *nvm_dev_attrs_20[] = {
 	NULL,
 };
 
-static const struct attribute_group nvm_dev_attr_group_20 = {
-	.name		= "lightnvm",
-	.attrs		= nvm_dev_attrs_20,
-};
-
-int nvme_nvm_register_sysfs(struct nvme_ns *ns)
+static umode_t nvm_dev_attrs_visible(struct kobject *kobj,
+				     struct attribute *attr, int index)
 {
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct gendisk *disk = dev_to_disk(dev);
+	struct nvme_ns *ns = disk->private_data;
 	struct nvm_dev *ndev = ns->ndev;
-	struct nvm_geo *geo = &ndev->geo;
+	struct device_attribute *dev_attr =
+		container_of(attr, typeof(*dev_attr), attr);
 
 	if (!ndev)
-		return -EINVAL;
-
-	switch (geo->major_ver_id) {
-	case 1:
-		return sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_12);
-	case 2:
-		return sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_20);
-	}
-
-	return -EINVAL;
-}
+		return 0;
 
-void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
-{
-	struct nvm_dev *ndev = ns->ndev;
-	struct nvm_geo *geo = &ndev->geo;
+	if (dev_attr->show == nvm_dev_attr_show)
+		return attr->mode;
 
-	switch (geo->major_ver_id) {
+	switch (ndev->geo.major_ver_id) {
 	case 1:
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_12);
+		if (dev_attr->show == nvm_dev_attr_show_12)
+			return attr->mode;
 		break;
 	case 2:
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_20);
+		if (dev_attr->show == nvm_dev_attr_show_20)
+			return attr->mode;
 		break;
 	}
+
+	return 0;
 }
+
+const struct attribute_group nvme_nvm_attr_group = {
+	.name		= "lightnvm",
+	.attrs		= nvm_dev_attrs,
+	.is_visible	= nvm_dev_attrs_visible,
+};
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e71075338ff5..82274ce67c4e 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -314,11 +314,8 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 		return;
 
 	if (!(head->disk->flags & GENHD_FL_UP)) {
+		disk_to_dev(head->disk)->groups = nvme_ns_id_attr_groups;
 		device_add_disk(&head->subsys->dev, head->disk);
-		if (sysfs_create_group(&disk_to_dev(head->disk)->kobj,
-				&nvme_ns_id_attr_group))
-			dev_warn(&head->subsys->dev,
-				 "failed to create id group.\n");
 	}
 
 	synchronize_srcu(&ns->head->srcu);
@@ -541,11 +538,8 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 	if (!head->disk)
 		return;
-	if (head->disk->flags & GENHD_FL_UP) {
-		sysfs_remove_group(&disk_to_dev(head->disk)->kobj,
-				   &nvme_ns_id_attr_group);
+	if (head->disk->flags & GENHD_FL_UP)
 		del_gendisk(head->disk);
-	}
 	blk_set_queue_dying(head->disk->queue);
 	/* make sure all pending bios are cleaned up */
 	kblockd_schedule_work(&head->requeue_work);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9c2e7a151e40..276975506709 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -464,7 +464,7 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
 int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
 		void *log, size_t size, u64 offset);
 
-extern const struct attribute_group nvme_ns_id_attr_group;
+extern const struct attribute_group *nvme_ns_id_attr_groups[];
 extern const struct block_device_operations nvme_ns_head_ops;
 
 #ifdef CONFIG_NVME_MULTIPATH
@@ -589,8 +589,7 @@ static inline void nvme_mpath_update_disk_size(struct gendisk *disk)
 void nvme_nvm_update_nvm_info(struct nvme_ns *ns);
 int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node);
 void nvme_nvm_unregister(struct nvme_ns *ns);
-int nvme_nvm_register_sysfs(struct nvme_ns *ns);
-void nvme_nvm_unregister_sysfs(struct nvme_ns *ns);
+extern const struct attribute_group nvme_nvm_attr_group;
 int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd, unsigned long arg);
 #else
 static inline void nvme_nvm_update_nvm_info(struct nvme_ns *ns) {};
@@ -601,11 +600,6 @@ static inline int nvme_nvm_register(struct nvme_ns *ns, char *disk_name,
 }
 
 static inline void nvme_nvm_unregister(struct nvme_ns *ns) {};
-static inline int nvme_nvm_register_sysfs(struct nvme_ns *ns)
-{
-	return 0;
-}
-static inline void nvme_nvm_unregister_sysfs(struct nvme_ns *ns) {};
 static inline int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd,
 							unsigned long arg)
 {
-- 
2.27.0

Re: [PATCH 0/3] close udev startup race condition for several devices

[JeffleXu]

Forgot to mention that this patch set shall be directly applied to 4.19
stable, though this issue should be fixed for all currently maintained
stable trees, from stable #4.4+.

-- 
Thanks,
Jeffle

On 2/7/21 7:46 PM, Jeffle Xu wrote:
> The upstream commit fef912bf860e8e7e48a2bfb978a356bba743a8b7 ("block:
> genhd: add 'groups' argument to device_add_disk") and the following
> patches fix a race condition of udev for several devices, including
> nvme, aoe, zram and virtio.
> 
> The stable tree commit 9e07f4e243791e00a4086ad86e573705cf7b2c65("zram:
> close udev startup race condition as default groups") only fixes zram,
> leaving other devices unfixed.
> 
> This udev race issue indeed makes trouble. We recently found that this
> issue can cause missing '/dev/disk/by-id/XXXX' symlink of virtio-blk
> devices on 4.19.
> 
> Be noted that this patch set follows the idea of stable commit
> 9e07f4e243791e00a4086ad86e573705cf7b2c65 ("zram: close udev startup race
> condition as default groups") of merging the preparation patch (commit
> fef912bf860e) and the fixing patch (commit 98af4d4df889).
> 
> Jeffle Xu (3):
>   virtio-blk: close udev startup race condition as default groups
>   aoe: close udev startup race condition as default groups
>   nvme: close udev startup race condition as default groups
> 
>  drivers/block/aoe/aoe.h       |   1 -
>  drivers/block/aoe/aoeblk.c    |  20 +++----
>  drivers/block/aoe/aoedev.c    |   1 -
>  drivers/block/virtio_blk.c    |  67 +++++++++++++---------
>  drivers/nvme/host/core.c      |  20 +++----
>  drivers/nvme/host/lightnvm.c  | 105 ++++++++++++++--------------------
>  drivers/nvme/host/multipath.c |  10 +---
>  drivers/nvme/host/nvme.h      |  10 +---
>  8 files changed, 103 insertions(+), 131 deletions(-)
>