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

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

On 3/19/2025 7:54 PM, Greg Kroah-Hartman wrote:
> On Wed, Mar 19, 2025 at 07:05:56PM +0530, Naman Jain wrote:
>> On 3/18/2025 6:58 PM, Greg Kroah-Hartman wrote:
>>>> +/*
>>>> + * 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.
> 
> I mean you are adding a comment here that is NOT in the correct
> kerneldoc fomat.

Thanks, I get it now. I'll make the changes.

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

I'll add the necessary comments to highlight these problems.

Thanks for reviewing.

Regards,
Naman