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

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxx>
> 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.

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