| 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
| ||
|
Message-ID: <46572a83-2ed0-47a4-8f28-211f56a48a16@linux.microsoft.com> Date: Tue, 15 Apr 2025 21:41:50 +0530 From: Naman Jain <namjain@...ux.microsoft.com> To: Michael Kelley <mhklinux@...look.com>, "K . Y . Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Stephen Hemminger <stephen@...workplumber.org> Cc: "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> Subject: Re: [PATCH v4 0/2] uio_hv_generic: Fix ring buffer sysfs creation path On 4/15/2025 9:38 PM, Michael Kelley wrote: > From: Naman Jain <namjain@...ux.microsoft.com> Sent: Wednesday, April 9, 2025 11:09 PM >> >> Hi, >> This patch series aims to address the sysfs creation issue for the ring >> buffer by reorganizing the code. Additionally, it updates the ring sysfs >> size to accurately reflect the actual ring buffer size, rather than a >> fixed static value. >> >> PFB change logs: >> >> Changes since v3: >> https://lore.kernel.org/all/20250328052745.1417-1-namjain@linux.microsoft.com/ >> * Addressed Michael's comments regarding handling of return value of >> sysfs_update_group in uio_hv_generic. >> >> Changes since v2: >> https://lore.kernel.org/all/20250318061558.3294-1-namjain@linux.microsoft.com/ >> Addressed Greg's comments: >> * Split the original patch into two. >> * Updated the commit message to explain the problem scenario. >> * Added comments for new APIs in the kerneldoc format. >> * Highlighted potential race conditions and explained why sysfs should not be created in >> the >> driver probe. >> >> * Made minor changes to how the sysfs_update_group return value is handled. >> >> Changes since v1: >> https://lore.kernel.org/all/20250225052001.2225-1-namjain@linux.microsoft.com/ >> * Fixed race condition in setting channel->mmap_ring_buffer by >> introducing a new variable for visibility of sysfs (addressed Greg's >> comments) >> * Used binary attribute fields instead of regular ones for initializing attribute_group. >> * Make size of ring sysfs dynamic based on actual ring buffer's size. >> * Preferred to keep mmap function in uio_hv_generic to give more control over ring's >> mmap functionality, since this is specific to uio_hv_generic driver. >> * Remove spurious warning during sysfs creation in uio_hv_generic probe. >> * Added comments in a couple of places. >> >> Changes since RFC patch: >> https://lore.kernel.org/all/20250214064351.8994-1-namjain@linux.microsoft.com/ >> * Different approach to solve the problem is proposed (credits to >> Michael Kelley). >> * Core logic for sysfs creation moved out of uio_hv_generic, to VMBus >> drivers where rest of the sysfs attributes for a VMBus channel >> are defined. (addressed Greg's comments) >> * Used attribute groups instead of sysfs_create* functions, and bundled >> ring attribute with other attributes for the channel sysfs. >> >> Error logs: >> >> [ 35.574120] ------------[ cut here ]------------ >> [ 35.574122] WARNING: CPU: 0 PID: 10 at fs/sysfs/file.c:591 sysfs_create_bin_file+0x81/0x90 >> [ 35.574168] Workqueue: hv_pri_chan vmbus_add_channel_work >> [ 35.574172] RIP: 0010:sysfs_create_bin_file+0x81/0x90 >> [ 35.574197] Call Trace: >> [ 35.574199] <TASK> >> [ 35.574200] ? show_regs+0x69/0x80 >> [ 35.574217] ? __warn+0x8d/0x130 >> [ 35.574220] ? sysfs_create_bin_file+0x81/0x90 >> [ 35.574222] ? report_bug+0x182/0x190 >> [ 35.574225] ? handle_bug+0x5b/0x90 >> [ 35.574244] ? exc_invalid_op+0x19/0x70 >> [ 35.574247] ? asm_exc_invalid_op+0x1b/0x20 >> [ 35.574252] ? sysfs_create_bin_file+0x81/0x90 >> [ 35.574255] hv_uio_probe+0x1e7/0x410 [uio_hv_generic] >> [ 35.574271] vmbus_probe+0x3b/0x90 >> [ 35.574275] really_probe+0xf4/0x3b0 >> [ 35.574279] __driver_probe_device+0x8a/0x170 >> [ 35.574282] driver_probe_device+0x23/0xc0 >> [ 35.574285] __device_attach_driver+0xb5/0x140 >> [ 35.574288] ? __pfx___device_attach_driver+0x10/0x10 >> [ 35.574291] bus_for_each_drv+0x86/0xe0 >> [ 35.574294] __device_attach+0xc1/0x200 >> [ 35.574297] device_initial_probe+0x13/0x20 >> [ 35.574315] bus_probe_device+0x99/0xa0 >> [ 35.574318] device_add+0x647/0x870 >> [ 35.574320] ? hrtimer_init+0x28/0x70 >> [ 35.574323] device_register+0x1b/0x30 >> [ 35.574326] vmbus_device_register+0x83/0x130 >> [ 35.574328] vmbus_add_channel_work+0x135/0x1a0 >> [ 35.574331] process_one_work+0x177/0x340 >> [ 35.574348] worker_thread+0x2b2/0x3c0 >> [ 35.574350] kthread+0xe3/0x1f0 >> [ 35.574353] ? __pfx_worker_thread+0x10/0x10 >> [ 35.574356] ? __pfx_kthread+0x10/0x10 >> >> Regards, >> Naman >> >> Naman Jain (2): >> uio_hv_generic: Fix sysfs creation path for ring buffer >> Drivers: hv: Make the sysfs node size for the ring buffer dynamic >> >> drivers/hv/hyperv_vmbus.h | 6 ++ >> drivers/hv/vmbus_drv.c | 119 ++++++++++++++++++++++++++++++++++- >> drivers/uio/uio_hv_generic.c | 39 +++++------- >> include/linux/hyperv.h | 6 ++ >> 4 files changed, 147 insertions(+), 23 deletions(-) >> > > I've tested this series with linux-next20250411. Did the basics of binding the > uio_hv_generic driver to the FCOPY device. The "ring" sysfs attribute shows up > correctly, and with the expected size of 32 KiB. Did the same with a synthetic > networking device, and the "ring" attribute has the expected size of 4 MiB. In both > cases opened and mmap'ed the "ring" attribute, then read from the ring to > verify accessibility. Did not do any ring operations or create any subchannels. > > Did some test hackery to create the conditions where hv_create_ring_sysfs() > could fail because the subdirectory under "channels" had not yet been created. > hv_uio_probe() correctly ignores the error. When the subdirectory under > "channels" is created later in vmbus_device_register(), the "ring" attribute > appears with the correct size. > > Just saw Greg KH's comment about running checkpatch.pl. I did not see any > errors from checkpatch.pl. Maybe that comment should have been directed > to a different patch? > > For the series: > > Reviewed-by: Michael Kelley <mhklinux@...look.com> > Tested-by: Michael Kelley <mhklinux@...look.com> Thank you so much Michael for reviewing and testing this thoroughly. Really appreciate it. Regards, Naman
Powered by blists - more mailing lists