From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id ED165A0C4B; Tue, 27 Jul 2021 16:25:29 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8289D410F0; Tue, 27 Jul 2021 16:25:29 +0200 (CEST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id D7766410ED for ; Tue, 27 Jul 2021 16:25:27 +0200 (CEST) Received: by mail-pj1-f51.google.com with SMTP id m1so18016886pjv.2 for ; Tue, 27 Jul 2021 07:25:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=banduracyber-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=vAQmaUk1EoDtA5M8975Ojs1DkZZHXfSBSZdCeOqquYs=; b=jCn6xJtSNs5p0Cfv0eia3mrcj8BuhQVuJ+ICAuMIx8Yf84xGIKHkRTsvkc2Swrfs3I nA2+PdH7CMGpn9jHY7+B9DvQ8wMn8KgKDS60LxxfDO7YO2RZTah5xIErF6Kx5xmi1qrp KR5LTMtM8F1H1gVBo5G0v4Ywcb0iBxKY1coBA+bADGaiqO04/N5rkSZo6b1IlBPkxZ2+ WYodg5NEqtqybLUJifusYyvZjoeXRKHK6RVI1/cECuDVQHk1mUEnjVlBynQv2ZPXqlJa eL+5nHcMQlm2aS/uDCBQVqhdgvyMzAPdiaauZ6rPVFZx7bnjhh2urFSVaPd06cN2wzJt CIaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=vAQmaUk1EoDtA5M8975Ojs1DkZZHXfSBSZdCeOqquYs=; b=Vu7fxAsN6m4XWT9S/tboUGOglC2ZBmonRoqSg+OPk9LqdqTumNcGa2BS24bRyoQ9D6 Y5V6vzAr/7hFEADIM1TLaULM40RB9tsy2xIh+233O47onTz/hQ1qpgBz1PPQov0ERLoO IFi2+KfHMC9Xijgh4SwLlVnobbIteXrLnCTpVfNXMKxC3BP2N3+FXMLza6lJVqk1RQ/2 ExRrZEFOsB9DbSPOOn6zzIOGQAS50KJbCoHPc40jhrmXU5GkAL/suEI9Ssg0rM3WD9aQ E6okUBUTIGic+aYFqD7yzIM3N856y3HifFfRvoeWDHAoG/2Mm1mG2BkpzfL3wkgcHZs/ J7XA== X-Gm-Message-State: AOAM532rP1eYX+L3Hl3pdrfNEpjPrCRxbRSpdpWHivtdajAbklvy7UbA 2xwnppmnWl57F1VTivaexNYXvQ== X-Google-Smtp-Source: ABdhPJx+BfYx7Gm7g5WZ23AzOC0p9xF6kpojrwFCNWqYt8C4C8MaruHHwMcHvjAG3tb+p1GDeHtaiw== X-Received: by 2002:a17:90a:4d0b:: with SMTP id c11mr627770pjg.82.1627395926894; Tue, 27 Jul 2021 07:25:26 -0700 (PDT) Received: from [10.100.100.62] ([174.79.238.18]) by smtp.gmail.com with ESMTPSA id m1sm4146067pfc.36.2021.07.27.07.25.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Jul 2021 07:25:25 -0700 (PDT) To: Long Li , Stephen Hemminger Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210726170040.25155-1-jonathan.erb@banduracyber.com> From: Jonathan Erb Message-ID: <2a6599ac-6227-b92f-b543-419bd548f1f2@banduracyber.com> Date: Tue, 27 Jul 2021 10:25:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> --- >> 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