linux-kernel - RE: [PATCH] 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:
 <CH3PR21MB4646756CF7B6EF5205E88F27BFD12@CH3PR21MB4646.namprd21.prod.outlook.com>
Date: Tue, 11 Mar 2025 17:48:31 +0000
From: Dexuan Cui <decui@...rosoft.com>
To: Naman Jain <namjain@...ux.microsoft.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>
CC: KY Srinivasan <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>, Stephen Hemminger <stephen@...workplumber.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...nel.org" <stable@...nel.org>, Saurabh Sengar
	<ssengar@...ux.microsoft.com>, Michael Kelley <mhklinux@...look.com>, Long Li
	<longli@...rosoft.com>
Subject: RE: [PATCH] uio_hv_generic: Fix sysfs creation path for ring buffer

> From: Naman Jain <namjain@...ux.microsoft.com>
> Sent: Monday, March 10, 2025 9:45 PM
> > [...]
> > Hi Greg,
> > I understand this is deviating from the discussions that we have had
> > till now, but I wanted to kindly request your opinion on the following
> > approach, which came up in one of our internal discussions.
> >
> > By moving the sysfs creation logic from hv_uio_probe to hv_uio_open, I
> > believe we can address this problem. Here are some of the benefits of
> > this approach:
> >
> > * This approach involves minimal changes and provides a localized
> > solution.

I prefer the "Approach 1" below, which requires only 1/10 of the changes
of "Approach 2".

> > * Since the use-case of ring sysfs is specific to uio_hv_generic and
> > DPDK, this will give us the flexibility to modify it based on
> > requirements. For example, ring_buffer_bin_attr.size should depend on
> > the ring buffer's allocated size, which is easier to manage if the
> > current code resides in uio_hv_generic.

The 'ring' sysfs file is used by DPDK, the user-mode driver for fcopy
(tools/hv/hv_fcopy_uio_daemon.c) and other out-of-tree user-mode drivers.

Before the hv_uio_generic driver and the user-mode driver load, the
hv_vmbus driver doesn't know the correct size of the 'ring'; currently the
patch of "Approach 2" hardcodes ring_buffer_bin_attr.size to 4MB, which
is incorrect for the Fcopy VMBus device and other VMBus devices.

 "Approach 1" also uses a hardcoded 4MB, but it can't be easily fixed by
adding one line in hv_uio_probe(). With "Approach 2", the fix would be
difficult as we would need to pass one more param 'size' from the driver
hv_uio_generic to hv_vmbus.

The patch of "Approach 2" already passes two params from hv_uio_generic
to hv_vmbus: 'ring_sysfs_visible'  and 'mmap_ring_buffer', and adds and
exports 2 APIs to hv_uio_generic.

With Approach 1, we can avoid all the trouble.

> > * The use-case of DPDK is such that at any given time, either the
> > hv_netvsc kernel driver or the userspace (DPDK) will be controlling this
> > HV_NIC device. We do not want to expose the ring buffer to userspace
> > when hv_netvsc is using the device. This is where the "awareness" of the
> > current user comes into play, and we need a way to control the
> > visibility of the "ring" sysfs from uio_hv_generic.
> >
> >
> > Alternatively, I could focus on resolving the race condition you
> > mentioned and proceed with refining the patch. This approach addresses
> > most of the general practice concerns you highlighted.
> >
> > Regards,
> > Naman
> 
> Hello Greg,
> Here are the two approaches that we discussed.
> Can you please suggest the approach which looks better to you
> for next version.
> 
> **Approach 1: move sysfs creation to hv_uio_open**
>  [...]
> **Approach 2: Move sysfs creation to hyperv drivers**

In general, indeed we would want to avoid manipulating sysfs and kobj directly,
but here IMO calling sysfs_create_bin_file() in hv_uio_generic is reasonable as
hv_uio_generic is the only user of the 'ring' sysfs file, and it has more info about
the 'ring' (i.e. the correct size; when the 'ring' is used); I prefer "Approach 1"
 since the patch is much smaller and cleaner.

Greg, can we have your opinions?

Thanks,
Dexuan