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] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <DS2PR21MB51816F322B9D204B75D9A4B6CE4AA@DS2PR21MB5181.namprd21.prod.outlook.com>
Date: Sat, 12 Jul 2025 20:00:35 +0000
From: Long Li <longli@...rosoft.com>
To: Naman Jain <namjain@...ux.microsoft.com>, KY Srinivasan
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Michael Kelley
	<mhklinux@...look.com>
CC: "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>, Saurabh Sengar <ssengar@...ux.microsoft.com>
Subject: RE: [PATCH v4] tools/hv: fcopy: Fix irregularities with size of ring
 buffer

> Subject: Re: [PATCH v4] tools/hv: fcopy: Fix irregularities with size of ring buffer
>
>
>
> On 7/11/2025 11:38 AM, 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>
> > Reviewed-by: Saurabh Sengar <ssengar@...ux.microsoft.com>

Reviewed-by: Long Li <longli@...rosoft.com>

> > ---
>
> Noticed that I missed adding change logs (again). Adding them now.
>
> Changes since v3:
> https://lore.kern/
> el.org%2Fall%2F20250708080319.3904-1-
> namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com
> %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011
> db47%7C1%7C0%7C638878113544016320%7CUnknown%7CTWFpbGZsb3d8eyJ
> FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF
> pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=loiQdbSCzLluXmFECiIrrHbbK9
> fqxJBbLTSAB26%2Bd0k%3D&reserved=0
> * Added a goto label for freeing desc memory. (Saurabh)
> * Avoided declaring device path twice by using FCOPY_DEVICE_PATH(subdir)
> (Saurabh)
> * Removed extra len variable assignment in main() (Saurabh)
> * added Reviewed-by tag from Saurabh
>
> Changes since v2:
> https://lore.kern/
> el.org%2Fall%2F20250701104837.3006-1-
> namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com
> %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011
> db47%7C1%7C0%7C638878113544051861%7CUnknown%7CTWFpbGZsb3d8eyJ
> FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF
> pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=PZ9GdNL7%2FD6NfxZPhCOX
> 7zQ2DbspDgQs%2BS75PFGAFG4%3D&reserved=0
> * Removed fallback mechanism to default size, to keep fcopy behavior consistent
> (Long's suggestion). If ring sysfs file is not present for some reason, things are
> already bad and its the right thing for fcopy to abort.
>
> Changes since v1:
> https://lore.kern/
> el.org%2Fall%2F20250620070618.3097-1-
> namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com
> %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011
> db47%7C1%7C0%7C638878113544063634%7CUnknown%7CTWFpbGZsb3d8eyJ
> FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF
> pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=LQsSaTjbam4DPAzMQmy1%
> 2BMB0%2BKb6L7d3xKkM954mpls%3D&reserved=0
>
> * Removed unnecessary type casting in malloc for desc variable (Olaf)
> * Added retry mechanisms to avoid potential race conditions (Michael)
> * Moved the logic to fetch ring size to a later part in main (Michael)
>
>
>
> >   tools/hv/hv_fcopy_uio_daemon.c | 91 ++++++++++++++++++++++++++++++--
> --
> >   1 file changed, 81 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/hv/hv_fcopy_uio_daemon.c
> > b/tools/hv/hv_fcopy_uio_daemon.c index 0198321d14a2..7d9bcb066d3f
> > 100644
> > --- a/tools/hv/hv_fcopy_uio_daemon.c
> > +++ b/tools/hv/hv_fcopy_uio_daemon.c
> > @@ -35,7 +35,10 @@
> >   #define WIN8_SRV_MINOR            1
> >   #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_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)
> >
> >   #define FCOPY_VER_COUNT           1
> >   static const int fcopy_versions[] = { @@ -47,9 +50,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;
> > +
> > +   /* 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;
> > +                   }
> > +           }
> > +   }
> >
> > -static unsigned char desc[HV_RING_SIZE];
> > +   if (!ring_size && retry_count == 0) {
> > +           retry_count = 1;
> > +           rewinddir(dir);
> > +           usleep(100 * 1000); /* Wait 100ms and retry once */
> > +           goto retry_once;
> > +   }
> > +
> > +   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 +462,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,7 +493,20 @@ int main(int argc, char *argv[])
> >     openlog("HV_UIO_FCOPY", 0, LOG_USER);
> >     syslog(LOG_INFO, "starting; pid is:%d", getpid());
> >
> > -   fcopy_get_first_folder(FCOPY_UIO, uio_name);
> > +   ring_size = get_ring_buffer_size();
> > +   if (!ring_size) {
> > +           ret = -ENODEV;
> > +           goto exit;
> > +   }
> > +
> > +   desc = malloc(ring_size * sizeof(unsigned char));
> > +   if (!desc) {
> > +           syslog(LOG_ERR, "malloc failed for desc buffer");
> > +           ret = -ENOMEM;
> > +           goto exit;
> > +   }
> > +
> > +   fcopy_get_first_folder(FCOPY_UIO_PATH, uio_name);
> >     snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
> >     fcopy_fd = open(uio_dev_path, O_RDWR);
> >
> > @@ -445,17 +514,17 @@ int main(int argc, char *argv[])
> >             syslog(LOG_ERR, "open %s failed; error: %d %s",
> >                    uio_dev_path, errno, strerror(errno));
> >             ret = fcopy_fd;
> > -           goto exit;
> > +           goto free_desc;
> >     }
> >
> > -   ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
> > +   ring = vmbus_uio_map(&fcopy_fd, ring_size);
> >     if (!ring) {
> >             ret = errno;
> >             syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret,
> strerror(ret));
> >             goto close;
> >     }
> > -   vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
> > -   vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
> > +   vmbus_br_setup(&txbr, ring, ring_size);
> > +   vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
> >
> >     rxbr.vbr->imask = 0;
> >
> > @@ -472,7 +541,7 @@ int main(int argc, char *argv[])
> >                     goto close;
> >             }
> >
> > -           len = HV_RING_SIZE;
> > +           len = ring_size;
> >             ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
> >             if (unlikely(ret <= 0)) {
> >                     /* This indicates a failure to communicate (or worse) */
> @@
> > -492,6 +561,8 @@ int main(int argc, char *argv[])
> >     }
> >   close:
> >     close(fcopy_fd);
> > +free_desc:
> > +   free(desc);
> >   exit:
> >     return ret;
> >   }
> >
> > base-commit: b551c4e2a98a177a06148cf16505643cd2108386


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ