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 613EBA0C4D; Tue, 17 Aug 2021 22:24:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EDC6540DF5; Tue, 17 Aug 2021 22:24:06 +0200 (CEST) Received: from outbound.mail.eo.outlook.com (mail-oln040093008011.outbound.protection.outlook.com [40.93.8.11]) by mails.dpdk.org (Postfix) with ESMTP id 1ACED4014E; Tue, 17 Aug 2021 22:24:05 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PQ+m14YtIe+p6oA6+noVWsWb+hfpKQSGjeUc3wu9D/yRZb6cNbhNbBHpFA73EpoefX7LkVS+5R6P07FVx9bEWo/VOUapmTJQ6qSgFH1nrcuzCZTuuTDJ+qb0SSoooaYQ0c6bLW52SQQnNwmmH3mLpF7sTu1TeqYd6tN6m9KCgcVX8+614jw1WeS2m0hWxiU1z9WuYnFzOG0jj9mI5aoFH+KGg61KUPbWV8/m9ficY93fotSMq/qYyqDRLh1A5BOt2qCnMQugT3nzLrfJyaDDE8u1NuxdmaTM0V7WcfsVBgme9kS1IEXdKyHzx04+azEeTiLaP61GlxkAGvKa18B8pw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Z+fCS3KUKhl1nVVZWeo22sft95R0ZEHE4gdsbNRqXtg=; b=aVmBPcpYCtB1mW8LhwP86F3CVGo6OotH9ATWxE0ZU7hDS8OU2VR2wSFbtNYHN6NF5/yksgVgmU6kkxMIH1JBeuwtCqCS8CbNPMGAKMowRrcKM4GS7JBw3FO2mGGkL0sdHSn7TcqYgb12//bHWWqbNyj8EfCd5ErtEcDrsgeCmo/QDAoWa8iJJqqYspVxpogbe7GYtOMrEmTS3HLztzI6Fm6Nq/CbTeSrSa6ZejMJVNjH1Jq9jXHnAeH5cTH/MleJ1Wish9fb7S4F5ty8PVjzWs+u+7lM1uM8aRT0IAAElSEuswRntIokPColBSYwKZghQJ1NF+K9/v4kjk0Vl9ngBQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Z+fCS3KUKhl1nVVZWeo22sft95R0ZEHE4gdsbNRqXtg=; b=WCNzQLwIBmmYikkMqWE6KdhN5RxiPfBClF2KRnpQk5mj1IkFZdxf2A2bxgLkl4EnBWgHwuz1gEskkpLKZvvLOHZKddGh0c9Cq/CDmLs/oS0xcYE9NeDd1gmkyz+of7mzSK9qgJ4Wm53B7EA+dWQlBntJw3+sOf2Ov1nNpuq388o= Received: from BY5PR21MB1506.namprd21.prod.outlook.com (2603:10b6:a03:23d::12) by BY5PR21MB1396.namprd21.prod.outlook.com (2603:10b6:a03:23d::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4457.4; Tue, 17 Aug 2021 20:24:01 +0000 Received: from BY5PR21MB1506.namprd21.prod.outlook.com ([fe80::75cf:f11d:d80d:dfd4]) by BY5PR21MB1506.namprd21.prod.outlook.com ([fe80::75cf:f11d:d80d:dfd4%2]) with mapi id 15.20.4436.016; Tue, 17 Aug 2021 20:24:01 +0000 From: Long Li To: Jonathan Erb , Stephen Hemminger CC: "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process Thread-Index: AQHXgj/5re0yRIwOMkiz0jbUl3USsKtVyxeggBWhpoCAAxkKgIAAaLiwgAMs+4CABiNZAA== Date: Tue, 17 Aug 2021 20:24:01 +0000 Message-ID: References: <20210726170040.25155-1-jonathan.erb@banduracyber.com> <4387890b-e4da-9a93-a142-dfd976b291b0@banduracyber.com> In-Reply-To: <4387890b-e4da-9a93-a142-dfd976b291b0@banduracyber.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ActionId=f5277805-92a5-459c-bf3e-16259fb4b066; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=true; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=Internal; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2021-08-17T19:54:17Z; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47; authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=microsoft.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: a248de06-a2b6-4d6c-ee2b-08d961bcf39f x-ms-traffictypediagnostic: BY5PR21MB1396: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3044; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: yqBvnPbjJmaaE6D5hIyuG7W1OkKe7xadSK2hSv4ig4AGhBlVSbF90LlJtGTMaYiQoePgjlhiEeVuJ+e4AE3sovCrq9c85F6znsNn2cBzVSLWW1A+rcvjSc4wZejQH4kcCD7hoG71I+oSHiV2OD21os9znZgYQkmm3nGuawl2jTGp3H//zfYUXEk+i+ff7wfLT8fiNkGDxoH2J8717V1emkKeWxIiswbiajsRSwXK5agnPh0HHJVHbvtfvDBJD/ZIOD6c2xxhktXNc8nRe0IKTpKb3h98JDePEDHWIupehWJyBr1gJc6npoCsHSybZuUXOQLIuHCNOT3NOvQAEPPVzquWsXP3TjVziCXrCJaESjjxEGey1rvxWiJB7SVE2J3nLtoWBBiCzGWB9Qy+0IX0XQ4vo1Cmn0N4rJodCFK/7fMuOVnVjrlR791Rue/G0baHEG9XZoGYlQOSyj8U56FlNF0spDNsqiWhyRJgGVnQkCG3W+vR6RhXuxddr9Y2BYtel7diTXwHrLMvzq5wECbV0tY264LYFdrNEefIPnGSE2NLqtWmP673cVk/i2E0lLrRGhbx7bAxl4YyZaGOel6WI9/bkRKUBiuhIAgj9AV056yioQy8jIPSMP6hgJZcIvANdiJAdsHfrOW1OFGLCU9Q911ozP6dpcgtXOPjQ6y0OJlVMxazk5Yd5UoNUU/nMmdLcJXoaOvQ70PkVD7d8qWgh+Dod4QgdfieRsGM/qEEo933jfoxz/nSf5LlZsCPY2Mu x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BY5PR21MB1506.namprd21.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(54906003)(86362001)(26005)(83380400001)(33656002)(110136005)(316002)(122000001)(38100700002)(8676002)(186003)(5660300002)(82960400001)(2906002)(82950400001)(7696005)(8990500004)(166002)(55016002)(66946007)(53546011)(9686003)(64756008)(66446008)(71200400001)(66476007)(66556008)(76116006)(4326008)(52536014)(10290500003)(6506007)(6636002)(38070700005)(508600001)(8936002)(10090945008); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?RNjZHwwXVnHKySh63/HKarZK6muXXAbautUc1qgY6OwpWcSRChvgTI7vY1t7?= =?us-ascii?Q?G8EWfmtsZf5Li9GfXC6558gKCbX+6gkyGeOyRFEJa10wJRMkVyKXeqpv6k70?= =?us-ascii?Q?XGti93LBkY04B8T2ueCgzbph7fq4YWAuDjTXcY9rUDCcVcABfpr9uIGLj21P?= =?us-ascii?Q?l9BQHvp089ebJ3qjxGMJFLcs20vOYVX32ei8wMW2KVMmQURsork8GF4pSJgE?= =?us-ascii?Q?eAkTRysNViHkqnj1UHc/D2NUAuTpTpbdE7NGMA9a681xvMMQsJpRjVXGGLgP?= =?us-ascii?Q?us5GSCbmRz9DXLKe3m2V5OZtTPR8taZD3ogIiNEeN7AgKhB/aY4Ubv2Jty3Y?= =?us-ascii?Q?ynefQeJcxLhsVnEytQwjDebHo1P1GZYGM6SfL91tVMK0l7e5Nsia47PVq852?= =?us-ascii?Q?GLmoJ3wU4iyqEDUELe7lm9js529j0/Nh2Iclo8UBbLfRXwVcbvicM2InEqm0?= =?us-ascii?Q?UIcafMUE8gYmtviL7gTzWDczZh606FX/H+lh58UnMFmB+aAuvLHvirneZTY8?= =?us-ascii?Q?dg5x9iRSFp2eUt8sHzFRaPoJmGrrPHDz4YayBgV9D/0jJDycs2I3wxDXSOYM?= =?us-ascii?Q?rloGcHuBiGUxkQbIieYNIPSQnXfW+IXEE+mS2Mg3VMAy0EBIj+Q1scO1/WC2?= =?us-ascii?Q?ZYT2bd3y58zpOY2nhGH74G8fGTwvTdqfV8lNb99qd3hd4r6U5/kTI5/035QU?= =?us-ascii?Q?Xi3zOpefD77gmbfOwWgbmn+5YSkTanodXXwI70nWJ9ZR8RowWJYr7/X2CpPQ?= =?us-ascii?Q?Ki7mPmlYGdPRQtclvrOj7TJbP7s22bx13vsSX+HzZPP9eEo4vrl/P9UIHlzm?= =?us-ascii?Q?jMpojgNBpb3q+/ylIULSM9zTOqbCwOtzoPCwPvn0M++Ugu+cMChL3fFHAXtU?= =?us-ascii?Q?Q5A4aQ/pRnuRUNDFenKyjN3F/DJ9cw9elkGmeR/UVeohYG4LF4Nfelo9WydM?= =?us-ascii?Q?osRvbNFkebTbNMSAAPT1kidr4UAyau40+apCL2WxgBbw18g1OPKezYu6y5A9?= =?us-ascii?Q?bdhGPikXezVaT5NQ49rBO36z5IrIU+oC4e3TSlVKZ0EjVPq/zCiyTp3aojA9?= =?us-ascii?Q?FS0NatUjjDze6t5C7nAHiolziCUTVj8wU3gJULRfS7cmYBuAj4fSRlwL/Vwd?= =?us-ascii?Q?eh4c57UDfyoIAimuPIAF3muGvYwUtJNnk2o2S/ozwHMI6IpELkOpKlZNTaPY?= =?us-ascii?Q?MfG4h04dHXP6GGHH0iVCiE8aoG1nM3FRtz0fd8qwbGW+sHElpBEuGin+zlPR?= =?us-ascii?Q?5UYFfB5ffJXfjnQ2/EkPgUAOgt2Uoj7IvfEUznYhytdlb53NuxIFdW/hZ8GK?= =?us-ascii?Q?WvYKp3/rMLgx+OnlDAYNN93I?= MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BY5PR21MB1506.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: a248de06-a2b6-4d6c-ee2b-08d961bcf39f X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Aug 2021 20:24:01.8218 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Pl+XdDF9DESFeLS6dzYXtTRsS7JuTxsTGWw5FYsGu6yG2mFQh7P2RJZ0ikRf2kU2Qwcn3BQrbkiJCBqbM6aQ5w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR21MB1396 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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" In the 2nd part, the mapped addresses for the sub-channels in the second pr= ocess 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 b= y the primary process) This can be done by recording the mapped addresses f= or all the sub-channels in the primary process to vmbus_tailq. Later when t= he 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 th= e primary process. From: Jonathan Erb Sent: Friday, August 13, 2021 3:10 PM To: Long Li ; Stephen Hemminger Cc: dev@dpdk.org; stable@dpdk.org Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in secon= dary 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_seconda= ry_subchan() wants to map to br->vbr but that does not appear to be initial= ized anywhere. In fact, it looks like we should set br->vbr to mapaddr whic= h 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_subc= han() on each channel, you can just create a new function vmbus_uio_get_sec= ondary_subchan(). This function goes through all subchannels and map ring b= uffers to the same addresses used by the primary process. In the original code, vmbus_uio_map_secondary_subchan() has a check for "if= (mapaddr =3D=3D ring_buf)". If the address is mapped to somewhere else, th= e address won't work for the secondary process. So we need to keep this che= ck. From: Stephen Hemminger Sent: Wednesday, August 11, 2021 8:26 AM To: Jonathan Erb ; Long Li Cc: dev@dpdk.org; stable@dpdk.org Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in secon= dary process Looks fine, only comments would be to keep to original style and add check = if primary channel not found? From: Jonathan Erb > Sent: Monday, August 9, 2021 9:07 AM To: Long Li >; Stephen He= mminger > Cc: dev@dpdk.org; stable@dpdk.org Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in secon= dary process You don't often get email from jonathan.erb@banduracyber.com. Learn why this is important Would it be possible to resolve the lack of subchannels in secondary proces= ses as follows: 1. Create the following function in vmbus_uio.c to obtain the count of subc= hannels created by the primary: int vmbus_uio_get_num_subchan(struct vmbus_channel *primary) { const struct rte_vmbus_device *dev =3D primary->device; char chan_path[PATH_MAX]; struct dirent *ent; DIR *chan_dir; int err; int subchan_cnt =3D 0; snprintf(chan_path, sizeof(chan_path), "%s/%s/channels", SYSFS_VMBUS_DEVICES, dev->device.name); chan_dir =3D opendir(chan_path); if (!chan_dir) { VMBUS_LOG(ERR, "cannot open %s: %s", chan_path, strerror(errno)); return 0; } while ((ent =3D readdir(chan_dir))) { unsigned long relid, subid, monid; char *endp; if (ent->d_name[0] =3D=3D '.') continue; errno =3D 0; relid =3D strtoul(ent->d_name, &endp, 0); if (*endp || errno !=3D 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 =3D vmbus_uio_get_num_subchan(dev->primary); for (i =3D 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 create= d by the primary, then perform their own mappings as needed. All this can o= ccur 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 --- 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 =3D vmbus_chan_create(device, device->relid, 0, device->monitor_id, new_chan); - if (!err) { + if (!err) device->primary =3D *new_chan; - uio_res->primary =3D *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 =3D=3D uio_res->maps[i].addr) + if (mapaddr =3D=3D uio_res->maps[i].addr) { + dev->resource[i].addr =3D mapaddr; continue; /* successful map */ + } if (mapaddr =3D=3D 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 =3D 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 s= econdary process. The code after is: STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) { if (vmbus_uio_map_secondary_subchan(dev, chan) !=3D 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 me= mory functions for allocating vmbus device. In this way they can be proper= ly mapped to the secondary process. But rte memory functions are not availa= ble 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() !=3D RTE_PROC_PRIMARY) + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) { + rte_free(dev->primary); return vmbus_uio_unmap(uio_res); + } TAILQ_REMOVE(uio_res_list, uio_res, next); -- 2.17.1