| 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:
<KUZP153MB14443BE3D49BA0F5FD8F305DBE48A@KUZP153MB1444.APCP153.PROD.OUTLOOK.COM>
Date: Thu, 10 Jul 2025 12:19:58 +0000
From: Saurabh Singh Sengar <ssengar@...rosoft.com>
To: Naman Jain <namjain@...ux.microsoft.com>, Saurabh Singh Sengar
<ssengar@...ux.microsoft.com>
CC: KY Srinivasan <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Long Li
<longli@...rosoft.com>, Michael Kelley <mhklinux@...look.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Olaf Hering
<olaf@...fle.de>
Subject: RE: [PATCH v3] tools/hv: fcopy: Fix irregularities with size of ring
buffer
> On 7/10/2025 11:54 AM, Naman Jain wrote:
> >
> >
> > On 7/9/2025 4:52 PM, Saurabh Singh Sengar wrote:
> >> On Tue, Jul 08, 2025 at 01:33:19PM +0530, Naman Jain wrote:
> >>> Size of ring buffer, as defined in uio_hv_generic driver, is no longer
> >>> fixed to 16 KB. This creates a problem in fcopy, since this size was
> >>> hardcoded. With the change in place to make ring sysfs node actually
> >>> reflect the size of underlying ring buffer, it is safe to get the size
> >>> of ring sysfs file and use it for ring buffer size in fcopy daemon.
> >>> Fix the issue of disparity in ring buffer size, by making it dynamic
> >>> in fcopy uio daemon.
> >>>
> >>> Cc: stable@...r.kernel.org
> >>> Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
> >>> Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
> >>> ---
> >>> tools/hv/hv_fcopy_uio_daemon.c | 82
> +++++++++++++++++++++++++++++++---
> >>> 1 file changed, 75 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/
> >>> hv_fcopy_uio_daemon.c
> >>> index 0198321d14a2..5388ee1ebf4d 100644
> >>> --- a/tools/hv/hv_fcopy_uio_daemon.c
> >>> +++ b/tools/hv/hv_fcopy_uio_daemon.c
> >>> @@ -36,6 +36,7 @@
> >>> #define WIN8_SRV_VERSION (WIN8_SRV_MAJOR << 16 |
> WIN8_SRV_MINOR)
> >>> #define FCOPY_UIO "/sys/bus/vmbus/devices/
> >>> eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
> >>> +#define FCOPY_CHANNELS_PATH "/sys/bus/vmbus/devices/
> >>> eb765408-105f-49b6-b4aa-c123b64d17d4/channels"
> >>
> >> We can use a single path up to the device ID and then append either
> >> 'uio' or 'channels' using
> >> two separate variables.
> >
> > I am planning to use it like this, please let me know if it is OK.
> >
> > +#define FCOPY_DEVICE_PATH(subdir) "/sys/bus/vmbus/devices/
> > eb765408-105f-49b6-b4aa-c123b64d17d4/"#subdir
> > +#define FCOPY_UIO_PATH FCOPY_DEVICE_PATH(uio)
> > +#define FCOPY_CHANNELS_PATH FCOPY_DEVICE_PATH(channels)
>
> As per your suggestion, using it like this avoids the need to change
> hard coded device path at two places, and it looks better from code
> re-usability POV. No functional changes.
>
> I will make this change in next version.
>
>
> >
> >>
> >>> #define FCOPY_VER_COUNT 1
> >>> static const int fcopy_versions[] = {
> >>> @@ -47,9 +48,62 @@ static const int fw_versions[] = {
> >>> UTIL_FW_VERSION
> >>> };
> >>> -#define HV_RING_SIZE 0x4000 /* 16KB ring buffer size */
> >>> +static uint32_t get_ring_buffer_size(void)
> >>> +{
> >>> + char ring_path[PATH_MAX];
> >>> + DIR *dir;
> >>> + struct dirent *entry;
> >>> + struct stat st;
> >>> + uint32_t ring_size = 0;
> >>> + int retry_count = 0;
> >>> -static unsigned char desc[HV_RING_SIZE];
> >>> + /* Find the channel directory */
> >>> + dir = opendir(FCOPY_CHANNELS_PATH);
> >>> + if (!dir) {
> >>> + usleep(100 * 1000); /* Avoid race with kernel, wait 100ms
> >>> and retry once */
> >>> + dir = opendir(FCOPY_CHANNELS_PATH);
> >>> + if (!dir) {
> >>> + syslog(LOG_ERR, "Failed to open channels directory: %s",
> >>> strerror(errno));
> >>> + return 0;
> >>> + }
> >>> + }
> >>> +
> >>> +retry_once:
> >>> + while ((entry = readdir(dir)) != NULL) {
> >>> + if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") !=
> >>> 0 &&
> >>> + strcmp(entry->d_name, "..") != 0) {
> >>> + snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
> >>> + FCOPY_CHANNELS_PATH, entry->d_name);
> >>> +
> >>> + if (stat(ring_path, &st) == 0) {
> >>> + /*
> >>> + * stat returns size of Tx, Rx rings combined,
> >>> + * so take half of it for individual ring size.
> >>> + */
> >>> + ring_size = (uint32_t)st.st_size / 2;
> >>> + syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
> >>> + ring_path, ring_size);
> >>> + break;
> >>> + }
> >>> + }
> >>> + }
> >>> +
> >>> + if (!ring_size && retry_count == 0) {
> >>> + retry_count = 1;
> >>> + rewinddir(dir);
> >>> + usleep(100 * 1000); /* Wait 100ms and retry once */
> >>> + goto retry_once;
> >>
> >> Is this retry solving any real problem ?
> >
> > Yes, these two retry mechanism are added to avoid race conditions with
> > creation of channels dir, numbered channels inside channels directory.
> > More in patch 1 comment by Michael.
> >
> https://lore.k/
> ernel.org%2Fall%2F&data=05%7C02%7Cssengar%40microsoft.com%7C5db27
> 7813c2d41ca6d7308ddbf98930f%7C72f988bf86f141af91ab2d7cd011db47%7C
> 1%7C0%7C638877384276448917%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0e
> U1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCI
> sIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=MXfEStja1KkKSXjvh%2Fn%2Frp
> QyF1jCwOwmnI8KQb5O3qk%3D&reserved=0
> >
> SN6PR02MB41574C54FFDE0D3F3B7A5649D47CA@...PR02MB4157.namprd
> 02.prod.outlook.com/
> >
> >>
> >>> + }
> >>> +
> >>> + closedir(dir);
> >>> +
> >>> + if (!ring_size)
> >>> + syslog(LOG_ERR, "Could not determine ring size");
> >>> +
> >>> + return ring_size;
> >>> +}
> >>> +
> >>> +static unsigned char *desc;
> >>> static int target_fd;
> >>> static char target_fname[PATH_MAX];
> >>> @@ -406,7 +460,7 @@ int main(int argc, char *argv[])
> >>> int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
> >>> struct vmbus_br txbr, rxbr;
> >>> void *ring;
> >>> - uint32_t len = HV_RING_SIZE;
> >>> + uint32_t ring_size, len;
> >>> char uio_name[NAME_MAX] = {0};
> >>> char uio_dev_path[PATH_MAX] = {0};
> >>> @@ -437,6 +491,20 @@ int main(int argc, char *argv[])
> >>> openlog("HV_UIO_FCOPY", 0, LOG_USER);
> >>> syslog(LOG_INFO, "starting; pid is:%d", getpid());
> >>> + ring_size = get_ring_buffer_size();
> >>> + if (!ring_size) {
> >>> + ret = -ENODEV;
> >>> + goto exit;
> >>> + }
> >>> +
> >>> + len = ring_size;
> >>
> >> Do we need this ?
> >
> > Yes, because len is being used as a temporary variable for storing
> > ring_size, and it is modified when we pass it with reference in
> > rte_vmbus_chan_recv_raw. In order to avoid calculating ring sizes again,
> > we need to keep ring_size separate.
>
> I misinterpreted your query. We can remove this line, since len is
> reinitialized to ring_size before using again in main() function.
post fixing these,
Reviewed-by: Saurabh Sengar <ssengar@...ux.microsoft.com>
- Saurabh
Powered by blists - more mailing lists