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 5B092A0C43; Thu, 12 Aug 2021 00:06:35 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C68D44014D; Thu, 12 Aug 2021 00:06:34 +0200 (CEST) Received: from NAM10-MW2-obe.outbound.protection.outlook.com (mail-mw2nam10on2138.outbound.protection.outlook.com [40.107.94.138]) by mails.dpdk.org (Postfix) with ESMTP id 1958F40042; Thu, 12 Aug 2021 00:06:33 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=WIuw5NBIz0kPd89j3kBuepHNo5kAj5MxRCyuCCwve1ONTHZlE2sFxDIkc8lzX+iE1h9PXCZpzxZPMn6D2Hv/V+PousYmavbCJMXiQs8je0KIM1VaFeYHxUZQH9Lv/MAHk8Zj4ipGCfMjU9ZRiLMFa05YeYzgQxcHsjb4gBIgdojKnoI6cU3BLVepYsBdHZw0OJOBSDQ5XGsCRTHq1+K5W0kmx31M3mK3eRqtqvouUAKft3wKNPy6T/G+OBhNk0Q9QdILaPtSiytdciermRaMVnM+Bd2YAz5jiepjcaaMKOLJEcViKtKeN1z1wjjviUC+3aAK4YlMTVQQA6Gmia7SZw== 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=AuiG00iRDm6W8WpfZfn1mkG5YfwFmeleID9YRKwyJzI=; b=lwBijCp7elzQWUpVcwdK2jiMd613uYwHU8x5E7wB22a6RZ0aWIwIr8OeS2kCPlfd7tEVntui5uouurBQ9af5CUDshWJOK+U8L45TexyYGf+3z1w9Fv89Ar8qycpptcBd7kCXBux/Iw2wQxpNUb5OrHRsRnAiBAlsqXGkOyhoIuk8hEPrvgGjjt0ypjm11H8/4bK/zNorRZuvBDGHRcKLwgABRJL01cJZh07hmKJ2CKcirfD806YNjoVgupwFe5kJvJkm0eznkidXsZOBh5IZ7svM8NFCwOcSRKBhd14ocr7kji37X7UW7tSgq1mgML10P/kXyroxNWUYnsJp3mh/KQ== 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=AuiG00iRDm6W8WpfZfn1mkG5YfwFmeleID9YRKwyJzI=; b=DNzPf+af3Bl4yNExpVZCHK9D60nyTota446QFjg2laaH5QENwhRvZl/USH54VYBwVxuv2aoE8OkNKjsKHWvTSC/pI8WJWjZY/qo6x9rglHCyJBm4g/DHL6KhY7JYO+S3Ea6bPLYlQb6l5sTWTAAEq1kO/8e6CDUCK+VJERJQqMg= Received: from DM6PR21MB1513.namprd21.prod.outlook.com (2603:10b6:5:25c::19) by DM5PR2101MB1045.namprd21.prod.outlook.com (2603:10b6:4:9e::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4436.4; Wed, 11 Aug 2021 22:06:30 +0000 Received: from DM6PR21MB1513.namprd21.prod.outlook.com ([fe80::59e9:5eea:fd16:c67b]) by DM6PR21MB1513.namprd21.prod.outlook.com ([fe80::59e9:5eea:fd16:c67b%5]) with mapi id 15.20.4436.005; Wed, 11 Aug 2021 22:06:30 +0000 From: Long Li To: Stephen Hemminger , Jonathan Erb CC: "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process Thread-Index: AQHXgj/5re0yRIwOMkiz0jbUl3USsKtVyxeggBWhpoCAAxkKgIAAaLiw Date: Wed, 11 Aug 2021 22:06:30 +0000 Message-ID: References: <20210726170040.25155-1-jonathan.erb@banduracyber.com> In-Reply-To: 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=218d6f35-e65d-48a4-ac71-434402406f06; 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-11T15:23:40Z; 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: ec81a613-f31a-4511-8b5c-08d95d1445ec x-ms-traffictypediagnostic: DM5PR2101MB1045: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3513; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 3kGTBCvmWGE9nQwmSyEr6up+lY/siCsdtExTXmRlHEcqnvhryiQsjJ55k7AMfgIEAIJg/y7vUo+BGF7zbm9u1yVcBumfgkN1kh2BXrUJmt2IToNpf3z9L7V2GQBPjGbtpcHVTw9QxIn4VC/xknK4tR6z3qciOXHrRa1XrG4FG9AdFSojqc6arCwa4IoJyU3qliiEOV4Bne7b07MSXFRf7uR4kNzVOhK0bZfvByDLjrds0mbvZLHqKrc8y7p8EU3agUniRrLVRyZEb7bSVIbCDcZ8mDHXvHhoNqa0MSLTTxeh+5AzE/usNK/PkT3GQMrC5B6Lc6y0WsH+PixrBhPuAOT2ozZeq7bpm4aLw/dAS81Xqf4VbXhWjtq8aIjaP62fOvZnvOVRi009lUhvs4gdsh7M9vwdftoovzDvaRi6DCp2kfAA4+x4RwPD5/63hbtAJzHQAaKvvAlbuzqBs2NjSRbSTZ8ILAM8UmnOQrdYpCmgB/PSe6rEmTG/uyNbVoHbqsFxVS3NG0cl/E06i2SA0Aps3cLmBTb0tcrt1IedcDvBw7X7Vg46Ia33oVnVtHKaPIRVs1WB97laosQo+tXRaf8/z/OrgTBGr2zwEMZUboAnKwDwBexWPackoaSrPb1b9Fns87lMAjkaA9p503Hr/MJU6atqnxRcPU4ofrGjon2VuJE7vTLmj9LY8sHeP3Jg8o+GLS564Vjic40w4Xsgz2T65/CiprV78y4cOoCM33dqgmqdfw9ulPpWl89Bp1iG x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR21MB1513.namprd21.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(54906003)(26005)(122000001)(110136005)(9686003)(38070700005)(8990500004)(38100700002)(52536014)(4326008)(8676002)(5660300002)(316002)(82960400001)(82950400001)(55016002)(66946007)(186003)(166002)(76116006)(7696005)(66556008)(66446008)(66476007)(64756008)(508600001)(6506007)(53546011)(71200400001)(8936002)(83380400001)(86362001)(10290500003)(2906002)(33656002)(10090945008); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?YnZESt4Oke48Lfaz+awhb56zGRLBqKGSt1zAiEu1qJr074kkyH00ENxYm8hy?= =?us-ascii?Q?yJxl/xGBCV9jwdu2bUVjJkHZDFjTcg/YdPlRlYvecvnHHLhROvByaox0UjjL?= =?us-ascii?Q?LLbqjwV23NJTQkUjbilN3tNdFbi91C1UH/TYKaYz25WLj02JnGjoqzrUG+oR?= =?us-ascii?Q?nA+9pRkXq3HN0WD0EV0wcLtUFE0ngoULVOcT4gRX0uiZZqIcW+vFI2YlyPPl?= =?us-ascii?Q?uYwwfOxkY4TYZrd+jJDb002UQq+YQN83/WLKSrD8nOBiD4T6Mc2xqtyoob5O?= =?us-ascii?Q?Ls/Ct3yJdDTYtCv/nyTw4z3+/m+im56M4decObe+4S6Wu9ajTliJJszcOgCy?= =?us-ascii?Q?z0XvbovkwmXNSODWX8Dw/gcjRpWWPXCc8QIc8+YY+fJAh89ziPBwsttVztjC?= =?us-ascii?Q?KH2C2cXJWbpoHCjLwTKjFSGRJnRLPy1AMvxNFrYlasRlTF5WWlX21/IDAAjT?= =?us-ascii?Q?wa7qZQvjJpW1H4FBKuZV2O7UVNgp+NeAH4N8bDk+VetamDbEqcJtr09cx2BN?= =?us-ascii?Q?y0xCxKR9xVFy7EE8bt8Kv1bZCy7xBRrvgbnQh9FEXZ548h/sPKfdqFNMTljh?= =?us-ascii?Q?8X7CdI9935yDx7Fxexji2OC5tW8jHUOjpHMcdPJ0iJqSariu26iKB5jkWZRE?= =?us-ascii?Q?HoLRM/oI3FCSDP5ZJ4IFf8gVbxq/SVCc1Yf6aGELKbnW+e9li4zir2SO42Gv?= =?us-ascii?Q?fGfP9/2xakV2ZlZ+itRxISBy4dV9jXqtAjquJ2M88YBZRsdaYOXSm2HaC9AK?= =?us-ascii?Q?lpQMRZCkpfqwPii+7XEJjk669aHSjY/ZGsah8037b5Ick83wux0e5kF3fX4o?= =?us-ascii?Q?/TxY8RNutnYRGc7gE1aR46UyOdXPcoaqvgpOGjbfYdCAHzo/eitRIcv0D8lz?= =?us-ascii?Q?UFNd6u2VCU7hM0jgdD+KapsreBDLelGOt6b2BeDLt8IeocOiI5J/wANEObVO?= =?us-ascii?Q?xIdlbVkzno/GtlqcVI73ZwWxV0AotiPgQpqxBub5UdA7fludT+B2w1vMV7uJ?= =?us-ascii?Q?YhWVoLsEEMujmbeHYSD7G+ZAHFaAqwvK7/WhLap3FOPNIKnhUxm9H7DUrD30?= =?us-ascii?Q?4R7d6SZ09yb1NKHNYgVQYfIOoFj2iU1m1dDr7RIxWZDfMSs77iotGeb50We+?= =?us-ascii?Q?QimrE6yxA5DjrbQ0EDUeO6hzBykZTRP40SXyu+s8MSCB8OPm/Vv1Svu/GxD3?= =?us-ascii?Q?XmYY9GeMd+X47WqTcqSMTV/SmPC6IB8Z+auXpXixqfdkoGCadasb9p8L6NBF?= =?us-ascii?Q?mMscq9G7/FZEiWKp4s00heuC0qax7pcVB7IqUzCEn2MhwzUdHgUdk/Uk2IOG?= =?us-ascii?Q?CpM=3D?= MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR21MB1513.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ec81a613-f31a-4511-8b5c-08d95d1445ec X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Aug 2021 22:06:30.2917 (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: Atz9YTpwrOo/RLoGce/n5ufqZkb3ntCY3rsEXhnyf185XLj655MxlqPm6B2PvKOOLHfITYY4YrpauK/983qNTg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR2101MB1045 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" 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