uio_hv_generic: Fix sysfs creation path for ring buffer

On regular bootup, devices get registered to vmbus first, so when uio_hv_generic driver for a particular device type is probed, the device is already initialized and added, so sysfs creation in uio_hv_generic probe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to vmbus. However this time, the uio_hv_generic driver is already registered to probe for that device and in this case sysfs creation is tried before the device gets initialized completely.

Fix this by moving the core logic of sysfs creation for ring buffer, from uio_hv_generic to HyperV's vmbus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow. While at it, configure size of ring sysfs based on ring buffer's actual size and not 2MB default.

Problem path:
vmbus_device_register
device_register
uio_hv_generic probe
sysfs_create_bin_file (fails here)

kset_create_and_add (dependency)
vmbus_add_channel_kobj (dependency)

+/*
+ * hv_create_ring_sysfs - create ring sysfs entry corresponding to ring buffers for a channel
+ */

Quoted 9 lines · Naman Jain

On regular bootup, devices get registered to vmbus first, so when uio_hv_generic driver for a particular device type is probed, the device is already initialized and added, so sysfs creation in uio_hv_generic probe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to vmbus. However this time, the uio_hv_generic driver is already registered to probe for that device and in this case sysfs creation is tried before the device gets initialized completely.

Quoted 6 lines

Fix this by moving the core logic of sysfs creation for ring buffer, from uio_hv_generic to HyperV's vmbus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow. While at it, configure size of ring sysfs based on ring buffer's actual size and not 2MB default.

When you say stuff like "while at it..." that's a huge hint that the patch should be broken up into smaller pieces and made a patch series.

Quoted 5 lines

Problem path:
vmbus_device_register
device_register
uio_hv_generic probe
sysfs_create_bin_file (fails here)

Why does it fail?

Quoted 3 lines

kset_create_and_add (dependency)
vmbus_add_channel_kobj (dependency)

I don't understand this "graph", sorry.

Quoted 3 lines

+/*
+ * hv_create_ring_sysfs - create ring sysfs entry corresponding to ring buffers for a channel
+ */

Kerneldoc?

drivers/hv/vmbus_drv.c static const struct kobj_type vmbus_chan_ktype = { · line 1917

Quoted 12 lines

drivers/hv/vmbus_drv.c +11 -0

+int hv_create_ring_sysfs(struct vmbus_channel *channel,
+			 int (*hv_mmap_ring_buffer)(struct vmbus_channel *channel,
+						    struct vm_area_struct *vma))
+{
+	struct kobject *kobj = &channel->kobj;
+
+	channel->mmap_ring_buffer = hv_mmap_ring_buffer;
+	channel->ring_sysfs_visible = true;
+	return sysfs_update_group(kobj, &vmbus_chan_group);
+}
+EXPORT_SYMBOL_GPL(hv_create_ring_sysfs);

You just raced userspace and created a file without telling it that it showed up, right? Something still feels really wrong here.

greg k-h

Quoted 5 lines · Greg Kroah-Hartman

Quoted 3 lines

+/*
+ * hv_create_ring_sysfs - create ring sysfs entry corresponding to ring buffers for a channel
+ */

Kerneldoc?

Documentation of the ring sysfs is present here -
Documentation/ABI/stable/sysfs-bus-vmbus

What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
Date:           January. 2018
KernelVersion:  4.16
Contact:        Stephen Hemminger <[email protected]>
Description:    Binary file created by uio_hv_generic for ring buffer
Users:          Userspace drivers

I should probably change the description, to reflect that it's visibility is controlled by uio_hv_generic, but it's created by hyperV drivers.

Please correct me if I misunderstood your comment.

Quoted 15 lines

drivers/hv/vmbus_drv.c static const struct kobj_type vmbus_chan_ktype = { · line 1917

Quoted 11 lines

drivers/hv/vmbus_drv.c +11 -0

+int hv_create_ring_sysfs(struct vmbus_channel *channel,
+			 int (*hv_mmap_ring_buffer)(struct vmbus_channel *channel,
+						    struct vm_area_struct *vma))
+{
+	struct kobject *kobj = &channel->kobj;
+
+	channel->mmap_ring_buffer = hv_mmap_ring_buffer;
+	channel->ring_sysfs_visible = true;
+	return sysfs_update_group(kobj, &vmbus_chan_group);
+}
+EXPORT_SYMBOL_GPL(hv_create_ring_sysfs);

You just raced userspace and created a file without telling it that it showed up, right? Something still feels really wrong here.

Quoted 1 line

greg k-h

From use-case POV, we needed to have uio_hv_generic control the visibility of this ring sysfs node, because the same device (HV_NIC) is used by either hv_netvsc or uio_hv_generic at any given point of time. We didn't want to expose ring buffer through sysfs when hv_netvsc is using it. That's the reason why uio_hv_generic was creating sysfs in the first place.

DPDK, which uses this ring sysfs, checks for its presence for primary channel but for secondary channels, it additionally does mmap() of this ring. That's where it becomes more important, not to expose ring buffer when uio_hv_generic is not bind to the device.

DPDK runs after uio_hv_generic probe is complete, so I am not sure if this race can happen, in practice. I'll try to gather more information around it.

Quoted 17 lines · Naman Jain

Quoted 5 lines · Greg Kroah-Hartman

Quoted 3 lines

+/*
+ * hv_create_ring_sysfs - create ring sysfs entry corresponding to ring buffers for a channel
+ */

Kerneldoc?

Documentation of the ring sysfs is present here -
Documentation/ABI/stable/sysfs-bus-vmbus

What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
Date:           January. 2018
KernelVersion:  4.16
Contact:        Stephen Hemminger <[email protected]>
Description:    Binary file created by uio_hv_generic for ring buffer
Users:          Userspace drivers

Quoted 4 lines

I should probably change the description, to reflect that it's visibility is controlled by uio_hv_generic, but it's created by hyperV drivers.

Please correct me if I misunderstood your comment.

I mean you are adding a comment here that is NOT in the correct kerneldoc fomat.

Quoted 24 lines

Quoted 16 lines

drivers/hv/vmbus_drv.c static const struct kobj_type vmbus_chan_ktype = { · line 1917

Quoted 12 lines

drivers/hv/vmbus_drv.c +11 -0

+int hv_create_ring_sysfs(struct vmbus_channel *channel,
+			 int (*hv_mmap_ring_buffer)(struct vmbus_channel *channel,
+						    struct vm_area_struct *vma))
+{
+	struct kobject *kobj = &channel->kobj;
+
+	channel->mmap_ring_buffer = hv_mmap_ring_buffer;
+	channel->ring_sysfs_visible = true;
+	return sysfs_update_group(kobj, &vmbus_chan_group);
+}
+EXPORT_SYMBOL_GPL(hv_create_ring_sysfs);

You just raced userspace and created a file without telling it that it showed up, right? Something still feels really wrong here.

Quoted 1 line

greg k-h

From use-case POV, we needed to have uio_hv_generic control the visibility
of this ring sysfs node, because the same device (HV_NIC) is used by either hv_netvsc or uio_hv_generic at any given point of time. We didn't want to expose ring buffer through sysfs when hv_netvsc is using it. That's the reason why uio_hv_generic was creating sysfs in the first place.

Quoted 7 lines

DPDK, which uses this ring sysfs, checks for its presence for primary channel but for secondary channels, it additionally does mmap() of this ring. That's where it becomes more important, not to expose ring buffer when uio_hv_generic is not bind to the device.

DPDK runs after uio_hv_generic probe is complete, so I am not sure if this race can happen, in practice. I'll try to gather more information around it.

Please do, and document the heck out of why you are doing it this way and why there is no such race or issue, and why no one else should ever copy this pattern as an example of what to do for other drivers.

thanks,

greg k-h