Re: [PATCH v2] uio_hv_generic: Fix sysfs creation path for ring buffer

[Naman Jain <namjain@linux.microsoft.com>]

On 3/18/2025 6:58 PM, Greg Kroah-Hartman wrote:
> On Tue, Mar 18, 2025 at 11:45:58AM +0530, Naman Jain wrote:
>> 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.
> 
> 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.
> 

I should have done that. I'll take care of it in next version.

>> Problem path:
>> vmbus_device_register
>>      device_register
>>          uio_hv_generic probe
>>                      sysfs_create_bin_file (fails here)
> 
> Why does it fail?

It fails because device kobj is not yet initialized. Will add more details.

> 
>>          kset_create_and_add (dependency)
>>          vmbus_add_channel_kobj (dependency)
> 
> I don't understand this "graph", sorry.
> 

I will revise the commit msg accordingly. Thanks.

>> +/*
>> + * 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 <sthemmin@microsoft.com>
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.

> 
>> +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

 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.

Regards,
Naman