| 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: <c7e1c5cc-4f80-4425-8afe-88e0801c574e@linux.microsoft.com>
Date: Thu, 10 Jul 2025 15:30:10 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
Cc: "K . Y . 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-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.kernel.org/all/
> SN6PR02MB41574C54FFDE0D3F3B7A5649D47CA@...PR02MB4157.namprd02.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.
>
>>
>>> + desc = malloc(ring_size * sizeof(unsigned char));
>>> + if (!desc) {
>>> + syslog(LOG_ERR, "malloc failed for desc buffer");
>>> + ret = -ENOMEM;
>>> + goto exit;
>>> + }
>>
>> This memory is not being freed anywhere. While I agree that
>> freeing memory at
>> program exit may not have much practical value, we can easily address
>> this by adding a goto label for cleanup, this will keep all the
>> static code
>> analyzers happy.
>>
>
> Sure. Will add it.
>
>>> +
>>> fcopy_get_first_folder(FCOPY_UIO, uio_name);
>>> snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
>>> fcopy_fd = open(uio_dev_path, O_RDWR);
>>> @@ -448,14 +516,14 @@ int main(int argc, char *argv[])
>>> goto exit;
>>> }
>>> - 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 +540,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) */
>>>
>>> base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
>>> --
>>> 2.34.1
>
> Regards,
> Naman
Powered by blists - more mailing lists