[v2] bus/vmbus: Fix crash when handling packets in secondary process
Checks
Commit Message
Have secondary processes construct their own copy of primary channel with
own mappings.
Remove vmbus_channel primary ptr from struct mapped_vmbus_resource as its
not used.
Populate virtual memory address "addr" in struct rte_mem_resource for
secondary processes as netvsc will attempt to reference it thus causing
a crash. It was initialized for primary processes but not for secondary.
Cc: stable@dpdk.org
Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com>
---
v2:
* Remove unnecessary check for NULL pointer before call to rte_free()
per reviwer comment.
drivers/bus/vmbus/private.h | 1 -
drivers/bus/vmbus/vmbus_channel.c | 4 +---
drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
3 files changed, 10 insertions(+), 9 deletions(-)
Comments
> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
> secondary process
>
> Have secondary processes construct their own copy of primary channel with
> own mappings.
>
> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
> as its not used.
>
> Populate virtual memory address "addr" in struct rte_mem_resource for
> secondary processes as netvsc will attempt to reference it thus causing a
> crash. It was initialized for primary processes but not for secondary.
> Cc: stable@dpdk.org
>
> Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com>
> ---
> v2:
> * Remove unnecessary check for NULL pointer before call to rte_free() per
> reviwer comment.
>
> drivers/bus/vmbus/private.h | 1 -
> drivers/bus/vmbus/vmbus_channel.c | 4 +---
> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
> index 528d60a42f..746212bd5f 100644
> --- a/drivers/bus/vmbus/private.h
> +++ b/drivers/bus/vmbus/private.h
> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>
> rte_uuid_t id;
> int nb_maps;
> - struct vmbus_channel *primary;
> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
> char path[PATH_MAX];
> };
> diff --git a/drivers/bus/vmbus/vmbus_channel.c
> b/drivers/bus/vmbus/vmbus_channel.c
> index f67f1c438a..119b9b367e 100644
> --- a/drivers/bus/vmbus/vmbus_channel.c
> +++ b/drivers/bus/vmbus/vmbus_channel.c
> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
> *device,
>
> err = vmbus_chan_create(device, device->relid, 0,
> device->monitor_id, new_chan);
> - if (!err) {
> + if (!err)
> device->primary = *new_chan;
> - uio_res->primary = *new_chan;
> - }
>
> return err;
> }
> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
> b/drivers/bus/vmbus/vmbus_common_uio.c
> index 8582e32c1d..83c56b6fa2 100644
> --- a/drivers/bus/vmbus/vmbus_common_uio.c
> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
> *dev)
> fd, offset,
> uio_res->maps[i].size, 0);
>
> - if (mapaddr == uio_res->maps[i].addr)
> + if (mapaddr == uio_res->maps[i].addr) {
> + dev->resource[i].addr = mapaddr;
> continue; /* successful map */
> + }
>
> if (mapaddr == MAP_FAILED)
> VMBUS_LOG(ERR,
> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
> *dev)
> /* fd is not needed in secondary process, close it */
> close(fd);
>
> - dev->primary = uio_res->primary;
> - if (!dev->primary) {
> - VMBUS_LOG(ERR, "missing primary channel");
> + if (vmbus_chan_create(dev, dev->relid, 0,
> + dev->monitor_id, &dev->primary)) {
> + VMBUS_LOG(ERR, "cannot create primary channel");
> return -1;
> }
Looking at this closer, I don't think it will work for subchannels in the secondary process.
The code after is:
STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
VMBUS_LOG(ERR, "cannot map secondary subchan");
return -1;
}
}
Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
Thanks,
Long
>
> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
> rte_vmbus_device *dev)
> return;
>
> /* secondary processes - just free maps */
> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> + rte_free(dev->primary);
> return vmbus_uio_unmap(uio_res);
> + }
>
> TAILQ_REMOVE(uio_res_list, uio_res, next);
>
> --
> 2.17.1
Long,
I suspect that this has not tripped me up because I'm only using queue
per TX/RX path. But yes, for multiple queues it looks to be a problem.
Jonathan
On 7/26/21 6:16 PM, Long Li wrote:
>> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>> secondary process
>>
>> Have secondary processes construct their own copy of primary channel with
>> own mappings.
>>
>> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
>> as its not used.
>>
>> Populate virtual memory address "addr" in struct rte_mem_resource for
>> secondary processes as netvsc will attempt to reference it thus causing a
>> crash. It was initialized for primary processes but not for secondary.
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com>
>> ---
>> v2:
>> * Remove unnecessary check for NULL pointer before call to rte_free() per
>> reviwer comment.
>>
>> drivers/bus/vmbus/private.h | 1 -
>> drivers/bus/vmbus/vmbus_channel.c | 4 +---
>> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
>> 3 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
>> index 528d60a42f..746212bd5f 100644
>> --- a/drivers/bus/vmbus/private.h
>> +++ b/drivers/bus/vmbus/private.h
>> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>>
>> rte_uuid_t id;
>> int nb_maps;
>> - struct vmbus_channel *primary;
>> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>> char path[PATH_MAX];
>> };
>> diff --git a/drivers/bus/vmbus/vmbus_channel.c
>> b/drivers/bus/vmbus/vmbus_channel.c
>> index f67f1c438a..119b9b367e 100644
>> --- a/drivers/bus/vmbus/vmbus_channel.c
>> +++ b/drivers/bus/vmbus/vmbus_channel.c
>> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
>> *device,
>>
>> err = vmbus_chan_create(device, device->relid, 0,
>> device->monitor_id, new_chan);
>> - if (!err) {
>> + if (!err)
>> device->primary = *new_chan;
>> - uio_res->primary = *new_chan;
>> - }
>>
>> return err;
>> }
>> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
>> b/drivers/bus/vmbus/vmbus_common_uio.c
>> index 8582e32c1d..83c56b6fa2 100644
>> --- a/drivers/bus/vmbus/vmbus_common_uio.c
>> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
>> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>> *dev)
>> fd, offset,
>> uio_res->maps[i].size, 0);
>>
>> - if (mapaddr == uio_res->maps[i].addr)
>> + if (mapaddr == uio_res->maps[i].addr) {
>> + dev->resource[i].addr = mapaddr;
>> continue; /* successful map */
>> + }
>>
>> if (mapaddr == MAP_FAILED)
>> VMBUS_LOG(ERR,
>> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>> *dev)
>> /* fd is not needed in secondary process, close it */
>> close(fd);
>>
>> - dev->primary = uio_res->primary;
>> - if (!dev->primary) {
>> - VMBUS_LOG(ERR, "missing primary channel");
>> + if (vmbus_chan_create(dev, dev->relid, 0,
>> + dev->monitor_id, &dev->primary)) {
>> + VMBUS_LOG(ERR, "cannot create primary channel");
>> return -1;
>> }
> Looking at this closer, I don't think it will work for subchannels in the secondary process.
>
> The code after is:
>
> STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
> if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
> VMBUS_LOG(ERR, "cannot map secondary subchan");
> return -1;
> }
> }
>
> Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
>
> Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
>
> Thanks,
> Long
>
>
>> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
>> rte_vmbus_device *dev)
>> return;
>>
>> /* secondary processes - just free maps */
>> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>> + rte_free(dev->primary);
>> return vmbus_uio_unmap(uio_res);
>> + }
>>
>> TAILQ_REMOVE(uio_res_list, uio_res, next);
>>
>> --
>> 2.17.1
Would it be possible to resolve the lack of subchannels in secondary
processes as follows:
1. Create the following function in vmbus_uio.c to obtain the count of
subchannels created by the primary:
int vmbus_uio_get_num_subchan(struct vmbus_channel *primary)
{
const struct rte_vmbus_device *dev = primary->device;
char chan_path[PATH_MAX];
struct dirent *ent;
DIR *chan_dir;
int err;
int subchan_cnt = 0;
snprintf(chan_path, sizeof(chan_path),
"%s/%s/channels",
SYSFS_VMBUS_DEVICES, dev->device.name);
chan_dir = opendir(chan_path);
if (!chan_dir) {
VMBUS_LOG(ERR, "cannot open %s: %s",
chan_path, strerror(errno));
return 0;
}
while ((ent = readdir(chan_dir))) {
unsigned long relid, subid, monid;
char *endp;
if (ent->d_name[0] == '.')
continue;
errno = 0;
relid = strtoul(ent->d_name, &endp, 0);
if (*endp || errno != 0 || relid > UINT16_MAX) {
VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
ent->d_name);
continue;
}
subchan_cnt++;
}
closedir(chan_dir);
//Less one for primary channel
return subchan_cnt - 1;
}
2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
/* fd is not needed in secondary process, close it */
close(fd);
if (vmbus_chan_create(dev, dev->relid, 0,
dev->monitor_id, &dev->primary)) {
VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
for (i = 0; i < subchannels_cnt; i++) {
if(vmbus_uio_get_subchan(dev->primary, &chan))
return -1;
STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
}
return 0;
}
In this way the secondary processes detect the number of subchannels
created by the primary, then perform their own mappings as needed. All
this can occur before rte_eal_init has completed.
Jonathan
On 7/26/21 6:16 PM, Long Li wrote:
>> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>> secondary process
>>
>> Have secondary processes construct their own copy of primary channel with
>> own mappings.
>>
>> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
>> as its not used.
>>
>> Populate virtual memory address "addr" in struct rte_mem_resource for
>> secondary processes as netvsc will attempt to reference it thus causing a
>> crash. It was initialized for primary processes but not for secondary.
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com>
>> ---
>> v2:
>> * Remove unnecessary check for NULL pointer before call to rte_free() per
>> reviwer comment.
>>
>> drivers/bus/vmbus/private.h | 1 -
>> drivers/bus/vmbus/vmbus_channel.c | 4 +---
>> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
>> 3 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
>> index 528d60a42f..746212bd5f 100644
>> --- a/drivers/bus/vmbus/private.h
>> +++ b/drivers/bus/vmbus/private.h
>> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>>
>> rte_uuid_t id;
>> int nb_maps;
>> - struct vmbus_channel *primary;
>> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>> char path[PATH_MAX];
>> };
>> diff --git a/drivers/bus/vmbus/vmbus_channel.c
>> b/drivers/bus/vmbus/vmbus_channel.c
>> index f67f1c438a..119b9b367e 100644
>> --- a/drivers/bus/vmbus/vmbus_channel.c
>> +++ b/drivers/bus/vmbus/vmbus_channel.c
>> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
>> *device,
>>
>> err = vmbus_chan_create(device, device->relid, 0,
>> device->monitor_id, new_chan);
>> - if (!err) {
>> + if (!err)
>> device->primary = *new_chan;
>> - uio_res->primary = *new_chan;
>> - }
>>
>> return err;
>> }
>> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
>> b/drivers/bus/vmbus/vmbus_common_uio.c
>> index 8582e32c1d..83c56b6fa2 100644
>> --- a/drivers/bus/vmbus/vmbus_common_uio.c
>> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
>> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>> *dev)
>> fd, offset,
>> uio_res->maps[i].size, 0);
>>
>> - if (mapaddr == uio_res->maps[i].addr)
>> + if (mapaddr == uio_res->maps[i].addr) {
>> + dev->resource[i].addr = mapaddr;
>> continue; /* successful map */
>> + }
>>
>> if (mapaddr == MAP_FAILED)
>> VMBUS_LOG(ERR,
>> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>> *dev)
>> /* fd is not needed in secondary process, close it */
>> close(fd);
>>
>> - dev->primary = uio_res->primary;
>> - if (!dev->primary) {
>> - VMBUS_LOG(ERR, "missing primary channel");
>> + if (vmbus_chan_create(dev, dev->relid, 0,
>> + dev->monitor_id, &dev->primary)) {
>> + VMBUS_LOG(ERR, "cannot create primary channel");
>> return -1;
>> }
> Looking at this closer, I don't think it will work for subchannels in the secondary process.
>
> The code after is:
>
> STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
> if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
> VMBUS_LOG(ERR, "cannot map secondary subchan");
> return -1;
> }
> }
>
> Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
>
> Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
>
> Thanks,
> Long
>
>
>> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
>> rte_vmbus_device *dev)
>> return;
>>
>> /* secondary processes - just free maps */
>> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>> + rte_free(dev->primary);
>> return vmbus_uio_unmap(uio_res);
>> + }
>>
>> TAILQ_REMOVE(uio_res_list, uio_res, next);
>>
>> --
>> 2.17.1
I think the code is on the right track.
Instead of using vmbus_uio_get_num_subchan() and calling vmbus_uio_get_subchan() on each channel, you can just create a new function vmbus_uio_get_secondary_subchan(). This function goes through all subchannels and map ring buffers to the same addresses used by the primary process.
In the original code, vmbus_uio_map_secondary_subchan() has a check for "if (mapaddr == ring_buf)". If the address is mapped to somewhere else, the address won't work for the secondary process. So we need to keep this check.
From: Stephen Hemminger <sthemmin@microsoft.com>
Sent: Wednesday, August 11, 2021 8:26 AM
To: Jonathan Erb <jonathan.erb@banduracyber.com>; Long Li <longli@microsoft.com>
Cc: dev@dpdk.org; stable@dpdk.org
Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process
Looks fine, only comments would be to keep to original style and add check if primary channel not found?
From: Jonathan Erb <jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>>
Sent: Monday, August 9, 2021 9:07 AM
To: Long Li <longli@microsoft.com<mailto:longli@microsoft.com>>; Stephen Hemminger <sthemmin@microsoft.com<mailto:sthemmin@microsoft.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; stable@dpdk.org<mailto:stable@dpdk.org>
Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process
You don't often get email from jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
Would it be possible to resolve the lack of subchannels in secondary processes as follows:
1. Create the following function in vmbus_uio.c to obtain the count of subchannels created by the primary:
int vmbus_uio_get_num_subchan(struct vmbus_channel *primary)
{
const struct rte_vmbus_device *dev = primary->device;
char chan_path[PATH_MAX];
struct dirent *ent;
DIR *chan_dir;
int err;
int subchan_cnt = 0;
snprintf(chan_path, sizeof(chan_path),
"%s/%s/channels",
SYSFS_VMBUS_DEVICES, dev->device.name);
chan_dir = opendir(chan_path);
if (!chan_dir) {
VMBUS_LOG(ERR, "cannot open %s: %s",
chan_path, strerror(errno));
return 0;
}
while ((ent = readdir(chan_dir))) {
unsigned long relid, subid, monid;
char *endp;
if (ent->d_name[0] == '.')
continue;
errno = 0;
relid = strtoul(ent->d_name, &endp, 0);
if (*endp || errno != 0 || relid > UINT16_MAX) {
VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
ent->d_name);
continue;
}
subchan_cnt++;
}
closedir(chan_dir);
//Less one for primary channel
return subchan_cnt - 1;
}
2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
/* fd is not needed in secondary process, close it */
close(fd);
if (vmbus_chan_create(dev, dev->relid, 0,
dev->monitor_id, &dev->primary)) {
VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
for (i = 0; i < subchannels_cnt; i++) {
if(vmbus_uio_get_subchan(dev->primary, &chan))
return -1;
STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
}
return 0;
}
In this way the secondary processes detect the number of subchannels created by the primary, then perform their own mappings as needed. All this can occur before rte_eal_init has completed.
Jonathan
On 7/26/21 6:16 PM, Long Li wrote:
Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
secondary process
Have secondary processes construct their own copy of primary channel with
own mappings.
Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
as its not used.
Populate virtual memory address "addr" in struct rte_mem_resource for
secondary processes as netvsc will attempt to reference it thus causing a
crash. It was initialized for primary processes but not for secondary.
Cc: stable@dpdk.org<mailto:stable@dpdk.org>
Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>
---
v2:
* Remove unnecessary check for NULL pointer before call to rte_free() per
reviwer comment.
drivers/bus/vmbus/private.h | 1 -
drivers/bus/vmbus/vmbus_channel.c | 4 +---
drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 528d60a42f..746212bd5f 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
rte_uuid_t id;
int nb_maps;
- struct vmbus_channel *primary;
struct vmbus_map maps[VMBUS_MAX_RESOURCE];
char path[PATH_MAX];
};
diff --git a/drivers/bus/vmbus/vmbus_channel.c
b/drivers/bus/vmbus/vmbus_channel.c
index f67f1c438a..119b9b367e 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
*device,
err = vmbus_chan_create(device, device->relid, 0,
device->monitor_id, new_chan);
- if (!err) {
+ if (!err)
device->primary = *new_chan;
- uio_res->primary = *new_chan;
- }
return err;
}
diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
b/drivers/bus/vmbus/vmbus_common_uio.c
index 8582e32c1d..83c56b6fa2 100644
--- a/drivers/bus/vmbus/vmbus_common_uio.c
+++ b/drivers/bus/vmbus/vmbus_common_uio.c
@@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
*dev)
fd, offset,
uio_res->maps[i].size, 0);
- if (mapaddr == uio_res->maps[i].addr)
+ if (mapaddr == uio_res->maps[i].addr) {
+ dev->resource[i].addr = mapaddr;
continue; /* successful map */
+ }
if (mapaddr == MAP_FAILED)
VMBUS_LOG(ERR,
@@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
*dev)
/* fd is not needed in secondary process, close it */
close(fd);
- dev->primary = uio_res->primary;
- if (!dev->primary) {
- VMBUS_LOG(ERR, "missing primary channel");
+ if (vmbus_chan_create(dev, dev->relid, 0,
+ dev->monitor_id, &dev->primary)) {
+ VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
Looking at this closer, I don't think it will work for subchannels in the secondary process.
The code after is:
STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
VMBUS_LOG(ERR, "cannot map secondary subchan");
return -1;
}
}
Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
Thanks,
Long
@@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
rte_vmbus_device *dev)
return;
/* secondary processes - just free maps */
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ rte_free(dev->primary);
return vmbus_uio_unmap(uio_res);
+ }
TAILQ_REMOVE(uio_res_list, uio_res, next);
--
2.17.1
The first part makes sense. There are several ways to iterate over all
the subchannels.
I'm probably not fully understanding the second part.
vmbus_uio_map_secondary_subchan() wants to map to br->vbr but that does
not appear to be initialized anywhere. In fact, it looks like we should
set br->vbr to mapaddr which is what vmbus_uio_map_subchan() does.
Jonathan
On 8/11/21 6:06 PM, Long Li wrote:
>
> I think the code is on the right track.
>
> Instead of using vmbus_uio_get_num_subchan() and calling
> vmbus_uio_get_subchan() on each channel, you can just create a new
> function vmbus_uio_get_secondary_subchan(). This function goes through
> all subchannels and map ring buffers to the same addresses used by the
> primary process.
>
> In the original code, vmbus_uio_map_secondary_subchan() has a check
> for “if (mapaddr == ring_buf)”. If the address is mapped to somewhere
> else, the address won’t work for the secondary process. So we need to
> keep this check.
>
> *From:* Stephen Hemminger <sthemmin@microsoft.com>
> *Sent:* Wednesday, August 11, 2021 8:26 AM
> *To:* Jonathan Erb <jonathan.erb@banduracyber.com>; Long Li
> <longli@microsoft.com>
> *Cc:* dev@dpdk.org; stable@dpdk.org
> *Subject:* RE: [PATCH v2] bus/vmbus: Fix crash when handling packets
> in secondary process
>
> Looks fine, only comments would be to keep to original style and add
> check if primary channel not found?
>
> *From:* Jonathan Erb <jonathan.erb@banduracyber.com
> <mailto:jonathan.erb@banduracyber.com>>
> *Sent:* Monday, August 9, 2021 9:07 AM
> *To:* Long Li <longli@microsoft.com <mailto:longli@microsoft.com>>;
> Stephen Hemminger <sthemmin@microsoft.com <mailto:sthemmin@microsoft.com>>
> *Cc:* dev@dpdk.org <mailto:dev@dpdk.org>; stable@dpdk.org
> <mailto:stable@dpdk.org>
> *Subject:* Re: [PATCH v2] bus/vmbus: Fix crash when handling packets
> in secondary process
>
>
>
>
> You don't often get email from jonathan.erb@banduracyber.com
> <mailto:jonathan.erb@banduracyber.com>. Learn why this is important
> <http://aka.ms/LearnAboutSenderIdentification>
>
>
>
> Would it be possible to resolve the lack of subchannels in secondary
> processes as follows:
>
> 1. Create the following function in vmbus_uio.c to obtain the count of
> subchannels created by the primary:
>
> int vmbus_uio_get_num_subchan(struct vmbus_channel *primary)
> {
>
> const struct rte_vmbus_device *dev = primary->device;
> char chan_path[PATH_MAX];
> struct dirent *ent;
> DIR *chan_dir;
> int err;
> int subchan_cnt = 0;
>
> snprintf(chan_path, sizeof(chan_path),
> "%s/%s/channels",
> SYSFS_VMBUS_DEVICES, dev->device.name);
>
> chan_dir = opendir(chan_path);
> if (!chan_dir) {
> VMBUS_LOG(ERR, "cannot open %s: %s",
> chan_path, strerror(errno));
> return 0;
> }
>
> while ((ent = readdir(chan_dir))) {
> unsigned long relid, subid, monid;
> char *endp;
>
> if (ent->d_name[0] == '.')
> continue;
>
> errno = 0;
> relid = strtoul(ent->d_name, &endp, 0);
> if (*endp || errno != 0 || relid > UINT16_MAX) {
> VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
> ent->d_name);
> continue;
> }
> subchan_cnt++;
> }
>
> closedir(chan_dir);
> //Less one for primary channel
> return subchan_cnt - 1;
>
> }
>
> 2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
>
> /* fd is not needed in secondary process, close it */
> close(fd);
>
> if (vmbus_chan_create(dev, dev->relid, 0,
> dev->monitor_id, &dev->primary)) {
> VMBUS_LOG(ERR, "cannot create primary channel");
> return -1;
> }
>
> int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
> for (i = 0; i < subchannels_cnt; i++) {
> if(vmbus_uio_get_subchan(dev->primary, &chan))
> return -1;
> STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
> }
> return 0;
> }
>
> In this way the secondary processes detect the number of subchannels
> created by the primary, then perform their own mappings as needed. All
> this can occur before rte_eal_init has completed.
>
> Jonathan
>
> On 7/26/21 6:16 PM, Long Li wrote:
>
> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>
> secondary process
>
> Have secondary processes construct their own copy of primary channel with
>
> own mappings.
>
> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
>
> as its not used.
>
> Populate virtual memory address "addr" in struct rte_mem_resource for
>
> secondary processes as netvsc will attempt to reference it thus causing a
>
> crash. It was initialized for primary processes but not for secondary.
>
> Cc:stable@dpdk.org <mailto:stable@dpdk.org>
>
> Signed-off-by: Jonathan Erb<jonathan.erb@banduracyber.com> <mailto:jonathan.erb@banduracyber.com>
>
> ---
>
> v2:
>
> * Remove unnecessary check for NULL pointer before call to rte_free() per
>
> reviwer comment.
>
> drivers/bus/vmbus/private.h | 1 -
>
> drivers/bus/vmbus/vmbus_channel.c | 4 +---
>
> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
>
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
>
> index 528d60a42f..746212bd5f 100644
>
> --- a/drivers/bus/vmbus/private.h
>
> +++ b/drivers/bus/vmbus/private.h
>
> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>
> rte_uuid_t id;
>
> int nb_maps;
>
> - struct vmbus_channel *primary;
>
> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>
> char path[PATH_MAX];
>
> };
>
> diff --git a/drivers/bus/vmbus/vmbus_channel.c
>
> b/drivers/bus/vmbus/vmbus_channel.c
>
> index f67f1c438a..119b9b367e 100644
>
> --- a/drivers/bus/vmbus/vmbus_channel.c
>
> +++ b/drivers/bus/vmbus/vmbus_channel.c
>
> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
>
> *device,
>
> err = vmbus_chan_create(device, device->relid, 0,
>
> device->monitor_id, new_chan);
>
> - if (!err) {
>
> + if (!err)
>
> device->primary = *new_chan;
>
> - uio_res->primary = *new_chan;
>
> - }
>
> return err;
>
> }
>
> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
>
> b/drivers/bus/vmbus/vmbus_common_uio.c
>
> index 8582e32c1d..83c56b6fa2 100644
>
> --- a/drivers/bus/vmbus/vmbus_common_uio.c
>
> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
>
> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>
> *dev)
>
> fd, offset,
>
> uio_res->maps[i].size, 0);
>
> - if (mapaddr == uio_res->maps[i].addr)
>
> + if (mapaddr == uio_res->maps[i].addr) {
>
> + dev->resource[i].addr = mapaddr;
>
> continue; /* successful map */
>
> + }
>
> if (mapaddr == MAP_FAILED)
>
> VMBUS_LOG(ERR,
>
> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>
> *dev)
>
> /* fd is not needed in secondary process, close it */
>
> close(fd);
>
> - dev->primary = uio_res->primary;
>
> - if (!dev->primary) {
>
> - VMBUS_LOG(ERR, "missing primary channel");
>
> + if (vmbus_chan_create(dev, dev->relid, 0,
>
> + dev->monitor_id, &dev->primary)) {
>
> + VMBUS_LOG(ERR, "cannot create primary channel");
>
> return -1;
>
> }
>
> Looking at this closer, I don't think it will work for subchannels in the secondary process.
>
> The code after is:
>
> STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
>
> if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
>
> VMBUS_LOG(ERR, "cannot map secondary subchan");
>
> return -1;
>
> }
>
> }
>
> Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
>
> Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
>
> Thanks,
>
> Long
>
> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
>
> rte_vmbus_device *dev)
>
> return;
>
> /* secondary processes - just free maps */
>
> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>
> + rte_free(dev->primary);
>
> return vmbus_uio_unmap(uio_res);
>
> + }
>
> TAILQ_REMOVE(uio_res_list, uio_res, next);
>
> --
>
> 2.17.1
>
In the 2nd part, the mapped addresses for the sub-channels in the second process should be the same as those mapped addresses in the primary process.
You are correct on seting br->vbr to mapaddr. (To the same addresses used by the primary process) This can be done by recording the mapped addresses for all the sub-channels in the primary process to vmbus_tailq. Later when the secondary process re-creates those channels, it can call vmbus_uio_find_resource() to find out the addresses for the same sub-channels mapped by the primary process.
From: Jonathan Erb <jonathan.erb@banduracyber.com>
Sent: Friday, August 13, 2021 3:10 PM
To: Long Li <longli@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>
Cc: dev@dpdk.org; stable@dpdk.org
Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process
The first part makes sense. There are several ways to iterate over all the subchannels.
I'm probably not fully understanding the second part. vmbus_uio_map_secondary_subchan() wants to map to br->vbr but that does not appear to be initialized anywhere. In fact, it looks like we should set br->vbr to mapaddr which is what vmbus_uio_map_subchan() does.
Jonathan
On 8/11/21 6:06 PM, Long Li wrote:
I think the code is on the right track.
Instead of using vmbus_uio_get_num_subchan() and calling vmbus_uio_get_subchan() on each channel, you can just create a new function vmbus_uio_get_secondary_subchan(). This function goes through all subchannels and map ring buffers to the same addresses used by the primary process.
In the original code, vmbus_uio_map_secondary_subchan() has a check for "if (mapaddr == ring_buf)". If the address is mapped to somewhere else, the address won't work for the secondary process. So we need to keep this check.
From: Stephen Hemminger <sthemmin@microsoft.com><mailto:sthemmin@microsoft.com>
Sent: Wednesday, August 11, 2021 8:26 AM
To: Jonathan Erb <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>; Long Li <longli@microsoft.com><mailto:longli@microsoft.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; stable@dpdk.org<mailto:stable@dpdk.org>
Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process
Looks fine, only comments would be to keep to original style and add check if primary channel not found?
From: Jonathan Erb <jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>>
Sent: Monday, August 9, 2021 9:07 AM
To: Long Li <longli@microsoft.com<mailto:longli@microsoft.com>>; Stephen Hemminger <sthemmin@microsoft.com<mailto:sthemmin@microsoft.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; stable@dpdk.org<mailto:stable@dpdk.org>
Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process
You don't often get email from jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
Would it be possible to resolve the lack of subchannels in secondary processes as follows:
1. Create the following function in vmbus_uio.c to obtain the count of subchannels created by the primary:
int vmbus_uio_get_num_subchan(struct vmbus_channel *primary)
{
const struct rte_vmbus_device *dev = primary->device;
char chan_path[PATH_MAX];
struct dirent *ent;
DIR *chan_dir;
int err;
int subchan_cnt = 0;
snprintf(chan_path, sizeof(chan_path),
"%s/%s/channels",
SYSFS_VMBUS_DEVICES, dev->device.name);
chan_dir = opendir(chan_path);
if (!chan_dir) {
VMBUS_LOG(ERR, "cannot open %s: %s",
chan_path, strerror(errno));
return 0;
}
while ((ent = readdir(chan_dir))) {
unsigned long relid, subid, monid;
char *endp;
if (ent->d_name[0] == '.')
continue;
errno = 0;
relid = strtoul(ent->d_name, &endp, 0);
if (*endp || errno != 0 || relid > UINT16_MAX) {
VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
ent->d_name);
continue;
}
subchan_cnt++;
}
closedir(chan_dir);
//Less one for primary channel
return subchan_cnt - 1;
}
2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
/* fd is not needed in secondary process, close it */
close(fd);
if (vmbus_chan_create(dev, dev->relid, 0,
dev->monitor_id, &dev->primary)) {
VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
for (i = 0; i < subchannels_cnt; i++) {
if(vmbus_uio_get_subchan(dev->primary, &chan))
return -1;
STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
}
return 0;
}
In this way the secondary processes detect the number of subchannels created by the primary, then perform their own mappings as needed. All this can occur before rte_eal_init has completed.
Jonathan
On 7/26/21 6:16 PM, Long Li wrote:
Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
secondary process
Have secondary processes construct their own copy of primary channel with
own mappings.
Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
as its not used.
Populate virtual memory address "addr" in struct rte_mem_resource for
secondary processes as netvsc will attempt to reference it thus causing a
crash. It was initialized for primary processes but not for secondary.
Cc: stable@dpdk.org<mailto:stable@dpdk.org>
Signed-off-by: Jonathan Erb <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>
---
v2:
* Remove unnecessary check for NULL pointer before call to rte_free() per
reviwer comment.
drivers/bus/vmbus/private.h | 1 -
drivers/bus/vmbus/vmbus_channel.c | 4 +---
drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 528d60a42f..746212bd5f 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
rte_uuid_t id;
int nb_maps;
- struct vmbus_channel *primary;
struct vmbus_map maps[VMBUS_MAX_RESOURCE];
char path[PATH_MAX];
};
diff --git a/drivers/bus/vmbus/vmbus_channel.c
b/drivers/bus/vmbus/vmbus_channel.c
index f67f1c438a..119b9b367e 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
*device,
err = vmbus_chan_create(device, device->relid, 0,
device->monitor_id, new_chan);
- if (!err) {
+ if (!err)
device->primary = *new_chan;
- uio_res->primary = *new_chan;
- }
return err;
}
diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
b/drivers/bus/vmbus/vmbus_common_uio.c
index 8582e32c1d..83c56b6fa2 100644
--- a/drivers/bus/vmbus/vmbus_common_uio.c
+++ b/drivers/bus/vmbus/vmbus_common_uio.c
@@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
*dev)
fd, offset,
uio_res->maps[i].size, 0);
- if (mapaddr == uio_res->maps[i].addr)
+ if (mapaddr == uio_res->maps[i].addr) {
+ dev->resource[i].addr = mapaddr;
continue; /* successful map */
+ }
if (mapaddr == MAP_FAILED)
VMBUS_LOG(ERR,
@@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
*dev)
/* fd is not needed in secondary process, close it */
close(fd);
- dev->primary = uio_res->primary;
- if (!dev->primary) {
- VMBUS_LOG(ERR, "missing primary channel");
+ if (vmbus_chan_create(dev, dev->relid, 0,
+ dev->monitor_id, &dev->primary)) {
+ VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
Looking at this closer, I don't think it will work for subchannels in the secondary process.
The code after is:
STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
VMBUS_LOG(ERR, "cannot map secondary subchan");
return -1;
}
}
Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good.
Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking.
Thanks,
Long
@@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
rte_vmbus_device *dev)
return;
/* secondary processes - just free maps */
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ rte_free(dev->primary);
return vmbus_uio_unmap(uio_res);
+ }
TAILQ_REMOVE(uio_res_list, uio_res, next);
--
2.17.1
I posted a patch to the mailing list.
http://patchwork.dpdk.org/project/dpdk/patch/1630389404-13468-1-git-send-email-longli@linuxonhyperv.com/
Jonathan, please let me know if this has fixed the crash you are seeing.
Long
> Subject: Re: [dpdk-dev] [PATCH v2] bus/vmbus: Fix crash when handling packets
> in secondary process
>
> In the 2nd part, the mapped addresses for the sub-channels in the second
> process should be the same as those mapped addresses in the primary process.
>
> You are correct on seting br->vbr to mapaddr. (To the same addresses used by
> the primary process) This can be done by recording the mapped addresses for all
> the sub-channels in the primary process to vmbus_tailq. Later when the
> secondary process re-creates those channels, it can call
> vmbus_uio_find_resource() to find out the addresses for the same sub-channels
> mapped by the primary process.
>
>
> From: Jonathan Erb <jonathan.erb@banduracyber.com>
> Sent: Friday, August 13, 2021 3:10 PM
> To: Long Li <longli@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in
> secondary process
>
>
> The first part makes sense. There are several ways to iterate over all the
> subchannels.
>
> I'm probably not fully understanding the second part.
> vmbus_uio_map_secondary_subchan() wants to map to br->vbr but that does
> not appear to be initialized anywhere. In fact, it looks like we should set br->vbr
> to mapaddr which is what vmbus_uio_map_subchan() does.
>
> Jonathan
> On 8/11/21 6:06 PM, Long Li wrote:
> I think the code is on the right track.
>
> Instead of using vmbus_uio_get_num_subchan() and calling
> vmbus_uio_get_subchan() on each channel, you can just create a new function
> vmbus_uio_get_secondary_subchan(). This function goes through all
> subchannels and map ring buffers to the same addresses used by the primary
> process.
>
> In the original code, vmbus_uio_map_secondary_subchan() has a check for "if
> (mapaddr == ring_buf)". If the address is mapped to somewhere else, the
> address won't work for the secondary process. So we need to keep this check.
>
> From: Stephen Hemminger
> <sthemmin@microsoft.com><mailto:sthemmin@microsoft.com>
> Sent: Wednesday, August 11, 2021 8:26 AM
> To: Jonathan Erb
> <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>;
> Long Li <longli@microsoft.com><mailto:longli@microsoft.com>
> Cc: dev@dpdk.org<mailto:dev@dpdk.org>;
> stable@dpdk.org<mailto:stable@dpdk.org>
> Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in
> secondary process
>
> Looks fine, only comments would be to keep to original style and add check if
> primary channel not found?
>
> From: Jonathan Erb
> <jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>>
> Sent: Monday, August 9, 2021 9:07 AM
> To: Long Li <longli@microsoft.com<mailto:longli@microsoft.com>>; Stephen
> Hemminger <sthemmin@microsoft.com<mailto:sthemmin@microsoft.com>>
> Cc: dev@dpdk.org<mailto:dev@dpdk.org>;
> stable@dpdk.org<mailto:stable@dpdk.org>
> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in
> secondary process
>
> You don't often get email from
> jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>.
> Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
>
> Would it be possible to resolve the lack of subchannels in secondary processes
> as follows:
>
>
>
> 1. Create the following function in vmbus_uio.c to obtain the count of
> subchannels created by the primary:
> int vmbus_uio_get_num_subchan(struct vmbus_channel *primary) {
>
> const struct rte_vmbus_device *dev = primary->device;
> char chan_path[PATH_MAX];
> struct dirent *ent;
> DIR *chan_dir;
> int err;
> int subchan_cnt = 0;
>
> snprintf(chan_path, sizeof(chan_path),
> "%s/%s/channels",
> SYSFS_VMBUS_DEVICES, dev->device.name);
>
> chan_dir = opendir(chan_path);
> if (!chan_dir) {
> VMBUS_LOG(ERR, "cannot open %s: %s",
> chan_path, strerror(errno));
> return 0;
> }
>
> while ((ent = readdir(chan_dir))) {
> unsigned long relid, subid, monid;
> char *endp;
>
> if (ent->d_name[0] == '.')
> continue;
>
> errno = 0;
> relid = strtoul(ent->d_name, &endp, 0);
> if (*endp || errno != 0 || relid > UINT16_MAX) {
> VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
> ent->d_name);
> continue;
> }
> subchan_cnt++;
> }
>
> closedir(chan_dir);
> //Less one for primary channel
> return subchan_cnt - 1;
>
> }
>
>
>
> 2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
> /* fd is not needed in secondary process, close it */
> close(fd);
>
> if (vmbus_chan_create(dev, dev->relid, 0,
> dev->monitor_id, &dev->primary)) {
> VMBUS_LOG(ERR, "cannot create primary channel");
> return -1;
> }
>
> int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
> for (i = 0; i < subchannels_cnt; i++) {
> if(vmbus_uio_get_subchan(dev->primary, &chan))
> return -1;
> STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
> }
> return 0;
> }
>
>
>
> In this way the secondary processes detect the number of subchannels created
> by the primary, then perform their own mappings as needed. All this can occur
> before rte_eal_init has completed.
>
>
>
> Jonathan
>
>
>
>
> On 7/26/21 6:16 PM, Long Li wrote:
>
> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>
> secondary process
>
>
>
> Have secondary processes construct their own copy of primary channel with
>
> own mappings.
>
>
>
> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
>
> as its not used.
>
>
>
> Populate virtual memory address "addr" in struct rte_mem_resource for
>
> secondary processes as netvsc will attempt to reference it thus causing a
>
> crash. It was initialized for primary processes but not for secondary.
>
> Cc: stable@dpdk.org<mailto:stable@dpdk.org>
>
>
>
> Signed-off-by: Jonathan Erb
> <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>
>
> ---
>
> v2:
>
> * Remove unnecessary check for NULL pointer before call to rte_free() per
>
> reviwer comment.
>
>
>
> drivers/bus/vmbus/private.h | 1 -
>
> drivers/bus/vmbus/vmbus_channel.c | 4 +---
>
> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
>
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
>
>
> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
>
> index 528d60a42f..746212bd5f 100644
>
> --- a/drivers/bus/vmbus/private.h
>
> +++ b/drivers/bus/vmbus/private.h
>
> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>
>
>
> rte_uuid_t id;
>
> int nb_maps;
>
> - struct vmbus_channel *primary;
>
> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>
> char path[PATH_MAX];
>
> };
>
> diff --git a/drivers/bus/vmbus/vmbus_channel.c
>
> b/drivers/bus/vmbus/vmbus_channel.c
>
> index f67f1c438a..119b9b367e 100644
>
> --- a/drivers/bus/vmbus/vmbus_channel.c
>
> +++ b/drivers/bus/vmbus/vmbus_channel.c
>
> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
>
> *device,
>
>
>
> err = vmbus_chan_create(device, device->relid, 0,
>
> device->monitor_id, new_chan);
>
> - if (!err) {
>
> + if (!err)
>
> device->primary = *new_chan;
>
> - uio_res->primary = *new_chan;
>
> - }
>
>
>
> return err;
>
> }
>
> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
>
> b/drivers/bus/vmbus/vmbus_common_uio.c
>
> index 8582e32c1d..83c56b6fa2 100644
>
> --- a/drivers/bus/vmbus/vmbus_common_uio.c
>
> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
>
> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>
> *dev)
>
> fd, offset,
>
> uio_res->maps[i].size, 0);
>
>
>
> - if (mapaddr == uio_res->maps[i].addr)
>
> + if (mapaddr == uio_res->maps[i].addr) {
>
> + dev->resource[i].addr = mapaddr;
>
> continue; /* successful map */
>
> + }
>
>
>
> if (mapaddr == MAP_FAILED)
>
> VMBUS_LOG(ERR,
>
> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>
> *dev)
>
> /* fd is not needed in secondary process, close it */
>
> close(fd);
>
>
>
> - dev->primary = uio_res->primary;
>
> - if (!dev->primary) {
>
> - VMBUS_LOG(ERR, "missing primary channel");
>
> + if (vmbus_chan_create(dev, dev->relid, 0,
>
> + dev->monitor_id, &dev->primary)) {
>
> + VMBUS_LOG(ERR, "cannot create primary channel");
>
> return -1;
>
> }
>
>
>
> Looking at this closer, I don't think it will work for subchannels in the secondary
> process.
>
>
>
> The code after is:
>
>
>
> STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
>
> if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
>
> VMBUS_LOG(ERR, "cannot map secondary subchan");
>
> return -1;
>
> }
>
> }
>
>
>
> Because at this time, the primary channel is just created, "subchannel_list"
> should be NULL. The secondary process ends up running without subchannels. In
> the primary process, "subchannel_list" are populated when it calls
> hn_dev_configure(), so it is good.
>
>
>
> Sorry I didn't spot this earlier. The best way to fix this is to use rte memory
> functions for allocating vmbus device. In this way they can be properly mapped
> to the secondary process. But rte memory functions are not available at the
> time vmbus device is probed. I haven't been able to find a good way to fix this.
> Will keep looking.
>
>
>
> Thanks,
>
> Long
>
>
>
>
>
>
>
> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
>
> rte_vmbus_device *dev)
>
> return;
>
>
>
> /* secondary processes - just free maps */
>
> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>
> + rte_free(dev->primary);
>
> return vmbus_uio_unmap(uio_res);
>
> + }
>
>
>
> TAILQ_REMOVE(uio_res_list, uio_res, next);
>
>
>
> --
>
> 2.17.1
>
Long,
I have applied the patch and have verified that I can successfully send
packets on queue 1 in a secondary process without issue. I believe this
exercises the use of vmbus subchannels in secondary processes.
Seems fine to me.
Thanks.
On 8/31/21 2:01 AM, Long Li wrote:
> I posted a patch to the mailing list.
>
> http://patchwork.dpdk.org/project/dpdk/patch/1630389404-13468-1-git-send-email-longli@linuxonhyperv.com/
>
> Jonathan, please let me know if this has fixed the crash you are seeing.
>
> Long
>
>> Subject: Re: [dpdk-dev] [PATCH v2] bus/vmbus: Fix crash when handling packets
>> in secondary process
>>
>> In the 2nd part, the mapped addresses for the sub-channels in the second
>> process should be the same as those mapped addresses in the primary process.
>>
>> You are correct on seting br->vbr to mapaddr. (To the same addresses used by
>> the primary process) This can be done by recording the mapped addresses for all
>> the sub-channels in the primary process to vmbus_tailq. Later when the
>> secondary process re-creates those channels, it can call
>> vmbus_uio_find_resource() to find out the addresses for the same sub-channels
>> mapped by the primary process.
>>
>>
>> From: Jonathan Erb <jonathan.erb@banduracyber.com>
>> Sent: Friday, August 13, 2021 3:10 PM
>> To: Long Li <longli@microsoft.com>; Stephen Hemminger
>> <sthemmin@microsoft.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>> secondary process
>>
>>
>> The first part makes sense. There are several ways to iterate over all the
>> subchannels.
>>
>> I'm probably not fully understanding the second part.
>> vmbus_uio_map_secondary_subchan() wants to map to br->vbr but that does
>> not appear to be initialized anywhere. In fact, it looks like we should set br->vbr
>> to mapaddr which is what vmbus_uio_map_subchan() does.
>>
>> Jonathan
>> On 8/11/21 6:06 PM, Long Li wrote:
>> I think the code is on the right track.
>>
>> Instead of using vmbus_uio_get_num_subchan() and calling
>> vmbus_uio_get_subchan() on each channel, you can just create a new function
>> vmbus_uio_get_secondary_subchan(). This function goes through all
>> subchannels and map ring buffers to the same addresses used by the primary
>> process.
>>
>> In the original code, vmbus_uio_map_secondary_subchan() has a check for "if
>> (mapaddr == ring_buf)". If the address is mapped to somewhere else, the
>> address won't work for the secondary process. So we need to keep this check.
>>
>> From: Stephen Hemminger
>> <sthemmin@microsoft.com><mailto:sthemmin@microsoft.com>
>> Sent: Wednesday, August 11, 2021 8:26 AM
>> To: Jonathan Erb
>> <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>;
>> Long Li <longli@microsoft.com><mailto:longli@microsoft.com>
>> Cc: dev@dpdk.org<mailto:dev@dpdk.org>;
>> stable@dpdk.org<mailto:stable@dpdk.org>
>> Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>> secondary process
>>
>> Looks fine, only comments would be to keep to original style and add check if
>> primary channel not found?
>>
>> From: Jonathan Erb
>> <jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>>
>> Sent: Monday, August 9, 2021 9:07 AM
>> To: Long Li <longli@microsoft.com<mailto:longli@microsoft.com>>; Stephen
>> Hemminger <sthemmin@microsoft.com<mailto:sthemmin@microsoft.com>>
>> Cc: dev@dpdk.org<mailto:dev@dpdk.org>;
>> stable@dpdk.org<mailto:stable@dpdk.org>
>> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>> secondary process
>>
>> You don't often get email from
>> jonathan.erb@banduracyber.com<mailto:jonathan.erb@banduracyber.com>.
>> Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
>>
>> Would it be possible to resolve the lack of subchannels in secondary processes
>> as follows:
>>
>>
>>
>> 1. Create the following function in vmbus_uio.c to obtain the count of
>> subchannels created by the primary:
>> int vmbus_uio_get_num_subchan(struct vmbus_channel *primary) {
>>
>> const struct rte_vmbus_device *dev = primary->device;
>> char chan_path[PATH_MAX];
>> struct dirent *ent;
>> DIR *chan_dir;
>> int err;
>> int subchan_cnt = 0;
>>
>> snprintf(chan_path, sizeof(chan_path),
>> "%s/%s/channels",
>> SYSFS_VMBUS_DEVICES, dev->device.name);
>>
>> chan_dir = opendir(chan_path);
>> if (!chan_dir) {
>> VMBUS_LOG(ERR, "cannot open %s: %s",
>> chan_path, strerror(errno));
>> return 0;
>> }
>>
>> while ((ent = readdir(chan_dir))) {
>> unsigned long relid, subid, monid;
>> char *endp;
>>
>> if (ent->d_name[0] == '.')
>> continue;
>>
>> errno = 0;
>> relid = strtoul(ent->d_name, &endp, 0);
>> if (*endp || errno != 0 || relid > UINT16_MAX) {
>> VMBUS_LOG(NOTICE, "not a valid channel relid: %s",
>> ent->d_name);
>> continue;
>> }
>> subchan_cnt++;
>> }
>>
>> closedir(chan_dir);
>> //Less one for primary channel
>> return subchan_cnt - 1;
>>
>> }
>>
>>
>>
>> 2. Then change the bottom of vmbus_uio_map_secondary() to be simply:
>> /* fd is not needed in secondary process, close it */
>> close(fd);
>>
>> if (vmbus_chan_create(dev, dev->relid, 0,
>> dev->monitor_id, &dev->primary)) {
>> VMBUS_LOG(ERR, "cannot create primary channel");
>> return -1;
>> }
>>
>> int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);
>> for (i = 0; i < subchannels_cnt; i++) {
>> if(vmbus_uio_get_subchan(dev->primary, &chan))
>> return -1;
>> STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);
>> }
>> return 0;
>> }
>>
>>
>>
>> In this way the secondary processes detect the number of subchannels created
>> by the primary, then perform their own mappings as needed. All this can occur
>> before rte_eal_init has completed.
>>
>>
>>
>> Jonathan
>>
>>
>>
>>
>> On 7/26/21 6:16 PM, Long Li wrote:
>>
>> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in
>>
>> secondary process
>>
>>
>>
>> Have secondary processes construct their own copy of primary channel with
>>
>> own mappings.
>>
>>
>>
>> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource
>>
>> as its not used.
>>
>>
>>
>> Populate virtual memory address "addr" in struct rte_mem_resource for
>>
>> secondary processes as netvsc will attempt to reference it thus causing a
>>
>> crash. It was initialized for primary processes but not for secondary.
>>
>> Cc: stable@dpdk.org<mailto:stable@dpdk.org>
>>
>>
>>
>> Signed-off-by: Jonathan Erb
>> <jonathan.erb@banduracyber.com><mailto:jonathan.erb@banduracyber.com>
>>
>> ---
>>
>> v2:
>>
>> * Remove unnecessary check for NULL pointer before call to rte_free() per
>>
>> reviwer comment.
>>
>>
>>
>> drivers/bus/vmbus/private.h | 1 -
>>
>> drivers/bus/vmbus/vmbus_channel.c | 4 +---
>>
>> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++-----
>>
>> 3 files changed, 10 insertions(+), 9 deletions(-)
>>
>>
>>
>> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
>>
>> index 528d60a42f..746212bd5f 100644
>>
>> --- a/drivers/bus/vmbus/private.h
>>
>> +++ b/drivers/bus/vmbus/private.h
>>
>> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
>>
>>
>>
>> rte_uuid_t id;
>>
>> int nb_maps;
>>
>> - struct vmbus_channel *primary;
>>
>> struct vmbus_map maps[VMBUS_MAX_RESOURCE];
>>
>> char path[PATH_MAX];
>>
>> };
>>
>> diff --git a/drivers/bus/vmbus/vmbus_channel.c
>>
>> b/drivers/bus/vmbus/vmbus_channel.c
>>
>> index f67f1c438a..119b9b367e 100644
>>
>> --- a/drivers/bus/vmbus/vmbus_channel.c
>>
>> +++ b/drivers/bus/vmbus/vmbus_channel.c
>>
>> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device
>>
>> *device,
>>
>>
>>
>> err = vmbus_chan_create(device, device->relid, 0,
>>
>> device->monitor_id, new_chan);
>>
>> - if (!err) {
>>
>> + if (!err)
>>
>> device->primary = *new_chan;
>>
>> - uio_res->primary = *new_chan;
>>
>> - }
>>
>>
>>
>> return err;
>>
>> }
>>
>> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c
>>
>> b/drivers/bus/vmbus/vmbus_common_uio.c
>>
>> index 8582e32c1d..83c56b6fa2 100644
>>
>> --- a/drivers/bus/vmbus/vmbus_common_uio.c
>>
>> +++ b/drivers/bus/vmbus/vmbus_common_uio.c
>>
>> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>>
>> *dev)
>>
>> fd, offset,
>>
>> uio_res->maps[i].size, 0);
>>
>>
>>
>> - if (mapaddr == uio_res->maps[i].addr)
>>
>> + if (mapaddr == uio_res->maps[i].addr) {
>>
>> + dev->resource[i].addr = mapaddr;
>>
>> continue; /* successful map */
>>
>> + }
>>
>>
>>
>> if (mapaddr == MAP_FAILED)
>>
>> VMBUS_LOG(ERR,
>>
>> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device
>>
>> *dev)
>>
>> /* fd is not needed in secondary process, close it */
>>
>> close(fd);
>>
>>
>>
>> - dev->primary = uio_res->primary;
>>
>> - if (!dev->primary) {
>>
>> - VMBUS_LOG(ERR, "missing primary channel");
>>
>> + if (vmbus_chan_create(dev, dev->relid, 0,
>>
>> + dev->monitor_id, &dev->primary)) {
>>
>> + VMBUS_LOG(ERR, "cannot create primary channel");
>>
>> return -1;
>>
>> }
>>
>>
>>
>> Looking at this closer, I don't think it will work for subchannels in the secondary
>> process.
>>
>>
>>
>> The code after is:
>>
>>
>>
>> STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
>>
>> if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
>>
>> VMBUS_LOG(ERR, "cannot map secondary subchan");
>>
>> return -1;
>>
>> }
>>
>> }
>>
>>
>>
>> Because at this time, the primary channel is just created, "subchannel_list"
>> should be NULL. The secondary process ends up running without subchannels. In
>> the primary process, "subchannel_list" are populated when it calls
>> hn_dev_configure(), so it is good.
>>
>>
>>
>> Sorry I didn't spot this earlier. The best way to fix this is to use rte memory
>> functions for allocating vmbus device. In this way they can be properly mapped
>> to the secondary process. But rte memory functions are not available at the
>> time vmbus device is probed. I haven't been able to find a good way to fix this.
>> Will keep looking.
>>
>>
>>
>> Thanks,
>>
>> Long
>>
>>
>>
>>
>>
>>
>>
>> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct
>>
>> rte_vmbus_device *dev)
>>
>> return;
>>
>>
>>
>> /* secondary processes - just free maps */
>>
>> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>
>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>>
>> + rte_free(dev->primary);
>>
>> return vmbus_uio_unmap(uio_res);
>>
>> + }
>>
>>
>>
>> TAILQ_REMOVE(uio_res_list, uio_res, next);
>>
>>
>>
>> --
>>
>> 2.17.1
>>
@@ -42,7 +42,6 @@ struct mapped_vmbus_resource {
rte_uuid_t id;
int nb_maps;
- struct vmbus_channel *primary;
struct vmbus_map maps[VMBUS_MAX_RESOURCE];
char path[PATH_MAX];
};
@@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device *device,
err = vmbus_chan_create(device, device->relid, 0,
device->monitor_id, new_chan);
- if (!err) {
+ if (!err)
device->primary = *new_chan;
- uio_res->primary = *new_chan;
- }
return err;
}
@@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
fd, offset,
uio_res->maps[i].size, 0);
- if (mapaddr == uio_res->maps[i].addr)
+ if (mapaddr == uio_res->maps[i].addr) {
+ dev->resource[i].addr = mapaddr;
continue; /* successful map */
+ }
if (mapaddr == MAP_FAILED)
VMBUS_LOG(ERR,
@@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
/* fd is not needed in secondary process, close it */
close(fd);
- dev->primary = uio_res->primary;
- if (!dev->primary) {
- VMBUS_LOG(ERR, "missing primary channel");
+ if (vmbus_chan_create(dev, dev->relid, 0,
+ dev->monitor_id, &dev->primary)) {
+ VMBUS_LOG(ERR, "cannot create primary channel");
return -1;
}
@@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct rte_vmbus_device *dev)
return;
/* secondary processes - just free maps */
- if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ rte_free(dev->primary);
return vmbus_uio_unmap(uio_res);
+ }
TAILQ_REMOVE(uio_res_list, uio_res, next);