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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8c3e2920-41a2-40df-9cbd-79c381aa7b4c@linux.microsoft.com>
Date: Thu, 20 Mar 2025 13:47:21 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "K . Y . Srinivasan" <kys@...rosoft.com>,
 Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
 Dexuan Cui <decui@...rosoft.com>,
 Stephen Hemminger <stephen@...workplumber.org>,
 linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...nel.org, Saurabh Sengar <ssengar@...ux.microsoft.com>,
 Michael Kelley <mhklinux@...look.com>
Subject: Re: [PATCH v2] uio_hv_generic: Fix sysfs creation path for ring
 buffer



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