Re: [dpdk-stable] [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process

[Jonathan Erb <jonathan.erb@banduracyber.com>]

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
>>