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 08F834548B; Tue, 18 Jun 2024 11:49:30 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DF0E940DD5; Tue, 18 Jun 2024 11:49:29 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0b-0016f401.pphosted.com [67.231.156.173]) by mails.dpdk.org (Postfix) with ESMTP id 4952D40684 for ; Tue, 18 Jun 2024 11:49:28 +0200 (CEST) Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 45I6bkPn017645; Tue, 18 Jun 2024 02:49:20 -0700 Received: from nam12-mw2-obe.outbound.protection.outlook.com (mail-mw2nam12lp2045.outbound.protection.outlook.com [104.47.66.45]) by mx0b-0016f401.pphosted.com (PPS) with ESMTPS id 3ysafh92n9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 18 Jun 2024 02:49:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kOVhFRMeCcAdN5L9w8K3kHV11LjOwyDiy1YA89FnbzWJncEElqZpiH2iaYqKxRnhx1qolrv1HGbFWjaA+vQjyd/3uQZsp2WNm0sgV4a2ckXP2VIZLHWEawhk7qFU9dqod2CUKZIV0Z2nzMEuE8Gltd0gYbyWkE4hWya8aW9GdZ7/Cx+zcL1+PxjOZNhZgBXdH9MuEgoD47MoSWs6PE0haUx1zXwUEehUc8a1nI0yB1QVGjps8KD2BQvoex80vGnHEtzC5FMM4+ZV2GuJSryCEkmyySFgw6T5bq6BZRsgcENxySYIzG5a+mJMfC9REq3DNNyft+WLXdr+Qz6M4BOC4A== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=o8n4sXYk2hdBO3MZZPOXrohCCSpr4HYEajLQqcLCjhI=; b=AtOKbxy3o8+JcepiTfc64QD4IoHfxR5q6koaKPt8v6kR1SUB5+akkq0A/UJFBwZ04Q0BTzKEWp6BN7UAJRXvJBGbpLjH2Z+rm+j6Dp1fxcAGgCYINkF+DME2Rxlf8CwUDyMdWd6jVZI0t3geoqcfgA4E81Yx2+3xYU9nJzBbo1Vvyte8YYPJriMuTkJZu4NGqzK5lridb+MiOOcF9PEWiKqdSUOvsCRYqzObzGkh977bbanucUuLazQGwMQd7pUEEhfl/rxh5B9Nmp+FHbLTsmfoUM0uzC0z24zzCvd2iK3bk1IDntcbuL/dB7pMbpK/ySmIJ+iRWoSZiD/MayImBg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=o8n4sXYk2hdBO3MZZPOXrohCCSpr4HYEajLQqcLCjhI=; b=uyF/SYBQTcwd9kcKxYHdPHIDVYDjEsnTYsjysPDBABXsGoZFNHfH1R6DYhx2CX5vemcLJp/N/2wHoSBg5RHUN1dHoFpCKyma64q0AMoFSE4PL0Vx0Vdq+4bEh1OoyyGaD4siKr6sr5AReNQwwKh0hcEHkwsv5UXpw9em6T5tMTM= Received: from PH0PR18MB5071.namprd18.prod.outlook.com (2603:10b6:510:16b::15) by PH0PR18MB4687.namprd18.prod.outlook.com (2603:10b6:510:ca::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7677.31; Tue, 18 Jun 2024 09:49:14 +0000 Received: from PH0PR18MB5071.namprd18.prod.outlook.com ([fe80::25e:7:91b:8f1c]) by PH0PR18MB5071.namprd18.prod.outlook.com ([fe80::25e:7:91b:8f1c%5]) with mapi id 15.20.7677.030; Tue, 18 Jun 2024 09:49:14 +0000 From: Kiran Kumar Kokkilagadda To: Robin Jarry , "dev@dpdk.org" , Jerin Jacob , Nithin Kumar Dabilpuram , Zhirun Yan Subject: RE: [EXTERNAL] [PATCH v3] graph: avoid id collisions Thread-Topic: [EXTERNAL] [PATCH v3] graph: avoid id collisions Thread-Index: AQHawWFGVotU66+GAUaUiRkzWxHeYrHNRnAg Date: Tue, 18 Jun 2024 09:49:14 +0000 Message-ID: References: <20240618092324.54166-2-rjarry@redhat.com> In-Reply-To: <20240618092324.54166-2-rjarry@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: PH0PR18MB5071:EE_|PH0PR18MB4687:EE_ x-ms-office365-filtering-correlation-id: 9193bb69-b39a-4f8d-9b62-08dc8f7be9b0 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; ARA:13230037|366013|376011|1800799021|38070700015; x-microsoft-antispam-message-info: =?utf-8?B?MWo4OTRQM2RtVzJpMHFrT2VHd3kwc0w5cWpKRzFqOHVRYmsxalFnQWUzRlYw?= =?utf-8?B?cEJZZndTTVZ3WjhORTY5Kzd2V0RCTVk5UW1LTGFKYkdWMVAwRVlXdHgvMUEr?= =?utf-8?B?eE94ZnpPQ0FuOXBLZVUwc1REU3ZVRHpVVmpIMDl0WEJGNUFDTS8xNHk0OTFD?= =?utf-8?B?cEVQak1VVXVsdTlQVHdpM2pqQWVHcEIzTjN3Y1ZGdlRLTE1hd3RGTytCQWlC?= =?utf-8?B?ZktTN2NzelNma2tGTG1pWFI4aDlyTXlzaDQrZjkyUG16RnBXOU51MjI1dVNo?= =?utf-8?B?cjBiSFhOSzViWjhleHhOZ05WN3hWYzh5dFlMM0prTEhYYWVUMGhjRFh1TG53?= =?utf-8?B?NitVbnFpMUJxUERiNkw1elNKRkZLSzdYZ2xaQkZndDZ0ajBlayttc3ZvSE5J?= =?utf-8?B?OUdjeHBtZk1CSWk2THRleVRCRkRUU0tzdDhRd0ZCNlRIdnlONzBpVkdpcFF0?= =?utf-8?B?MjhpcmhkbjhjVDN4MDdzdjFIWFJONXRiVnJtbUd1TkNLeXQ0N2piUDBFSDIy?= =?utf-8?B?NmUzTk9HaGtnWEc1YU9SSGo5WCsrWUNGQWtmL0MyOHR3d2hhUzBUQmM2TUxQ?= =?utf-8?B?VzF0aEdKczJiS0JkRFZEZzlQZlFnTUp3R0hsMG9xVUNYak1WVnRUOXlCQ0xX?= =?utf-8?B?RnJwUUM0MGtVWHBtZzFNVHZlWDJ3RWdQOVdpalEvZ1g3Si9vMEpoTVl0SnZj?= =?utf-8?B?aFcxYVVNWHdsTm1UcFdoQ09NWlprVXRYb1NQS2RFbmMvRVVTeitMMWEvQm9I?= =?utf-8?B?L1hUQUc4ME5UbW9UcUZmWEhQNUo4KzF0anBGY3p2ejhPVVdyMEZlTm1sMjJY?= =?utf-8?B?YlVHTFVGVk9UQzNJU3prSlZYRmMwd0V5NlprYnlHU1d3a1NKTCtRNjUwUEZ1?= =?utf-8?B?aVJBQnBZQURUS1BaVDFYaHNPVXM3OWx5ZjZheWZaZTZhUzNoNi81Ti9NQ0da?= =?utf-8?B?bmFQQmJnS2gzU3RTTW44OTBZanE0ZmQxVElWWVVidXJ0UmVhVmFZNk1BY1gy?= =?utf-8?B?UGRLbXZSNThoOUNPdTljNWY1U1dtS3BsT09aU2JyTk1vbEJ1OTRISThZSlZh?= =?utf-8?B?QkZtcjIwTmMvUFVybTZKMldicmxCSVRkeXZVSzZFcUhtSkxIT21NWGZCVHQv?= =?utf-8?B?QTJ6cEdXUC9zQWxZQVBXL2ZmN2NVNVdVbXdQVWVzQk10MjJrUGVlMm0vcTNC?= =?utf-8?B?Z2lzbmh1Ujlpd1MxbC9kOG5DaHBCUHdMc1pXR0F0cmZKTERDTTZRS2xGV201?= =?utf-8?B?LzZsSkNOWlNYMjdJb2tiMUF3VlJEYU1mVWRVMFJBWkFJRTh2TXlQS2lGa21k?= =?utf-8?B?RkhjVVA3c2RnSXVtR1BjV2pTZ2NEaGRmZWhFK1NVVXNiTDhhRDNnbFZZQ2dj?= =?utf-8?B?cFg1dm52Y2M5WG9qUGh3eWxOdzdLdDVKM3VlUXQyc0dwYnY5OXhSR0JsVG5l?= =?utf-8?B?dnFjUWZnNUtwMFBKZEIwazNaS0pEVDczY2FjcHYzSUtucG15MHB5WEJ2VjVF?= =?utf-8?B?SmVqSjV6U240VHdEcDZNLzloM1RYdUwram5ZZkY5Tjh0UXlvL2hoV29WeFd4?= =?utf-8?B?UDZFcEkzeHQyTlJCWjZYRU40Q1p4SVUrZWZPazNraUZkRVZndGgyUWdBbWxk?= =?utf-8?B?czIwejdHWG9oaWkwbkVpWFcyemxpbSsvZnlMMWtPdGh4YkgxMHVodVBKcDFh?= =?utf-8?B?MmtCUkc3WGk0ek1vV1lzdjdtQUFlVHhxcUdkUk02Sk8yZ21FZzNKem9NOTVt?= =?utf-8?Q?or0pDYphUdc8JOV+8cIEQdxsmUOMizgjxq5s5d8?= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:PH0PR18MB5071.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230037)(366013)(376011)(1800799021)(38070700015); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?utf-8?B?TFE1RXo4SHVJSnBZQzRZd2pzMnA4YXB4Nlh4UXhaS2NPQURKZ3hqeDVvWkNT?= =?utf-8?B?cXRSTHdiMW5nMUlFY1V0Qm9YNFVmT1pLS3U0SUxhN1hPbE15WlBhdlFBcVNJ?= =?utf-8?B?MFJQcHdicnBQbjlLLzFpVm9rWlZrZUhFMDIrcG5vbWR0WG5qaVBmb21OZUJo?= =?utf-8?B?MHZMdFFQVUJQRVo4WHFteGNmbk5kVi9QM3prN3AxTHFCVWpiWlFwajVQYnE4?= =?utf-8?B?Qncxbkhqb24rVGU4dmFabGwxR3BWYXZ2N1RMb3p0S2VRb0hleVFHbG9BbWIr?= =?utf-8?B?WVdMZDVnaXFEVEYyVFc1eERCYVF6YlZ3Rk9xMlR4WllQVW1BYWZrYk9xYk5I?= =?utf-8?B?UDZtL2NndXNSLzg3VFdPdVZhTmtoNG1YRjdCSXlhNFdLb1BhbXliaVVoZlFO?= =?utf-8?B?eTZ2SFhOcGlUMkpvdHFhcllDSk5kQTNqVldjVG45QXJGQ3g0SWo5ZkhsM01y?= =?utf-8?B?ZHlLS1RUaVNrY2R2a2d1b0xmNDhkZHRVc05VQllmQ1pJcmJEbVJocFZESnk1?= =?utf-8?B?N3BxWVYyc3BkR3Rwc3NwNnZKNHdOaUdsc3k2M1dDUDlqcmF3OWhDQ0hvZ1Ri?= =?utf-8?B?QkdBY0ExTDZmdmFPVU5McmJIeGxPV1p3SzlNQUtyM1Rha3FxVVBMS3JjeG1s?= =?utf-8?B?NG05eDYvM1FCNjBqS25Xbjk3K1dsR0QzZmMrWm94QlJwNkpvbzAxMGpOcU4r?= =?utf-8?B?ek40NHNDYXhJbkxzbkFkMWo3YTg4UjkrUWhDZmFYaHlhcU5OSStZcXN6ZU9l?= =?utf-8?B?VnBNdmtHMGt1dExmRk0zZmNYUGVlNGkvdFlOTGpyTTdhamhvd2Z4bmxCSmhS?= =?utf-8?B?UGcyR2VSQUNOdEcvbWhrMHJWQTR5ZnlVY2w0emRjNVF3a0dXYjRlTDlrOFlD?= =?utf-8?B?cTdoK1BDaDZWSzQzaHpPOTNwY25aSG8wK1hHa0RUenArUUZuc2pCc3BXbks1?= =?utf-8?B?QWVkTjQ1REx5UHVxNEkxRjJod2hDT25GL3daTFN4QkNHa1crQ1dVeGNHY1RM?= =?utf-8?B?YXBYY3dja3VBUUJ0U0ZJUEViSlYyajdIcUhVS1haTGRudHZ1d0xWK0pUdGNB?= =?utf-8?B?YWRqTVVNaERONXJVSk92TURYMEdFdThsOXdMSXczVW4xQmppYUZqQjV5aGoz?= =?utf-8?B?RUhmc0RRTjU0eHg4U3lKOHBQQlFtVnY0VE5ZbjFlQ3pBVUJWZFdYMkN5QUEw?= =?utf-8?B?OVZ6S1pZck03c3g3a2FNNVF6NlFWOC84cWsxOFNyeWRKMkZjZFo0djFsSmtV?= =?utf-8?B?cDB5bDAzbHh5WjhMbGtEWDZjRVpma2VSSk00Y1dYUFJLYzdQcjdLaVQzbHov?= =?utf-8?B?ZmdSWW1NL21iUXoxdVZtam5Tb2NWbjNoTVJ0OG5iMW1NVExycEI4Y0dUR2Jh?= =?utf-8?B?SmllVHhSamsvUnRnZTF4dEs2eGhiSm1PL1VteGNhRStpaEVIM2h2dVJhbmcy?= =?utf-8?B?dHllaUNlT1JkREVJVHFsSjZiOGpzQ3kxYTdCeG5Wc25Db3Vldk5aOVAzU1Zw?= =?utf-8?B?dmwvQVJRT2pnYnJYcU85MzBEMmlTMk9GZVF3Qm5RTW43OW16NlFnWjg3djhk?= =?utf-8?B?aTUyVEkxQ2MreTltQU9uOERZdXV1UjV3dU5HanJQZUlHb3ZTd1czZ3lKWUhs?= =?utf-8?B?UGQyOWlQMDhJWThsS1h3ZEhSRUhXdzBuNWdKMllNUWN1WTFnV0xZVGw4YkN2?= =?utf-8?B?OVpzelE4SDhCaGExNFNlUGtGWm8zQURYb25lNFIwZE5hemFFYWkzTW9tUmV1?= =?utf-8?B?ZVQ2TWh1WGFISUNMT0dlUjdiMGZSTUFTTGZPbG1neVhaVzc3akVPbk84ckRU?= =?utf-8?B?aDdHYk9aMUN4QXhILzNBc2ZHTllJSkdiU2FOZHg5U0lVNTRoQmRLMFRFZkNw?= =?utf-8?B?UFhyVTlsYmtqOFdNTVY5ZXF6Y3hxSXp0MWpsTkhYR0dJdjA1VW5TaWdielND?= =?utf-8?B?dnBXdGwvMFNxNm1XanNESlhiL1Q2S1ZhSnV2WjdjWVFEc2YzR0dHZzZvTUxY?= =?utf-8?B?WmYveDluNHp5dXlLVUlhRjI3Y2h3alBGRGVzYmllT3dpSlkyeGJ0V2RpVEhL?= =?utf-8?B?Y3pFSnhsZEhRalV6WFFRVGp1TXVPK2tsV0JQRXR5QzRMdHVnTXRCNlZ5ZnBB?= =?utf-8?Q?591cq/8dIKwioHZ80jNaemcW9?= Content-Type: multipart/alternative; boundary="_000_PH0PR18MB5071831072BDDFB839186565ACCE2PH0PR18MB5071namp_" MIME-Version: 1.0 X-OriginatorOrg: marvell.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: PH0PR18MB5071.namprd18.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 9193bb69-b39a-4f8d-9b62-08dc8f7be9b0 X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jun 2024 09:49:14.3553 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: HGdEsx9kJLXmCOttnaIh54V7yBDxyLbcELZCHF+YCAdwpHSrm60ACWZ9WF7bu4+eprKN+rK/ugGpNKr37nx68g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR18MB4687 X-Proofpoint-ORIG-GUID: jKYvLnnngbw4Vgrcy_gIs4z_ds6LN2kx X-Proofpoint-GUID: jKYvLnnngbw4Vgrcy_gIs4z_ds6LN2kx X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16 definitions=2024-06-18_02,2024-06-17_01,2024-05-17_01 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 --_000_PH0PR18MB5071831072BDDFB839186565ACCE2PH0PR18MB5071namp_ Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable From: Robin Jarry Sent: Tuesday, June 18, 2024 2:53 PM To: dev@dpdk.org; Jerin Jacob ; Kiran Kumar Kokkilagadd= a ; Nithin Kumar Dabilpuram ; Zhirun Yan Subject: [EXTERNAL] [PATCH v3] graph: avoid id collisions The graph id is determined based on a global variable that is incremented e= very time a graph is created, and decremented every time a graph is destroy= ed. This only works if graphs are destroyed in the reverse order in which t= hey have been created.=E2=80=8A The graph id is determined based on a global variable that is incremented every time a graph is created, and decremented every time a graph is destroyed. This only works if graphs are destroyed in the reverse order in which they have been created. The following code produces duplicate graph IDs which can lead to use-after-free bugs and other undefined behaviours: a =3D rte_graph_create(...); // id=3D0 graph_id=3D1 b =3D rte_graph_create(...); // id=3D1 graph_id=3D2 rte_graph_destroy(a); // graph_id=3D1 c =3D rte_graph_create(...); // id=3D1 graph_id=3D2 (duplicate with b) rte_graph_destroy(c); // frees memory still used by b Remove the global counter. Make sure that the graph list is always ordered by increasing graph ids. When creating a new graph, pick a free id which is not allocated. Update unit tests to ensure it works as expected. Signed-off-by: Robin Jarry > Acked-by: Kiran Kumar Kokkilagadda kirankumark@marvell.com Notes: v3: * Fixed invalid logic in graph_insert_ordered. * Updated unit test to ensure head/tail/middle insertion work (the test fails before fixing graph_insert_ordered). app/test/test_graph.c | 72 ++++++++++++++++++++++++++++++++++++ lib/graph/graph.c | 86 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 141 insertions(+), 17 deletions(-) diff --git a/app/test/test_graph.c b/app/test/test_graph.c index b8409bc60497..2840a25b13b7 100644 --- a/app/test/test_graph.c +++ b/app/test/test_graph.c @@ -696,6 +696,77 @@ test_graph_clone(void) return ret; } +static int +test_graph_id_collisions(void) +{ + static const char *node_patterns[] =3D {"test_node_source1", "t= est_node00"}; + struct rte_graph_param gconf =3D { + .socket_id =3D SOCKET_ID_ANY, + .nb_node_patterns =3D 2, + .node_patterns =3D node_patterns, + }; + rte_graph_t g1, g2, g3, g4; + + g1 =3D rte_graph_create("worker1", &gconf); + if (g1 =3D=3D RTE_GRAPH_ID_INVALID) { + printf("Graph 1 creation failed with error =3D = %d\n", rte_errno); + return -1; + } + g2 =3D rte_graph_create("worker2", &gconf); + if (g2 =3D=3D RTE_GRAPH_ID_INVALID) { + printf("Graph 2 creation failed with error =3D = %d\n", rte_errno); + return -1; + } + g3 =3D rte_graph_create("worker3", &gconf); + if (g3 =3D=3D RTE_GRAPH_ID_INVALID) { + printf("Graph 3 creation failed with error =3D = %d\n", rte_errno); + return -1; + } + if (g1 =3D=3D g2 || g2 =3D=3D g3 || g1 =3D=3D g3) { + printf("Graph ids should be different\n"); + return -1; + } + if (rte_graph_destroy(g2) < 0) { + printf("Graph 2 suppression failed\n"); + return -1; + } + g4 =3D rte_graph_create("worker4", &gconf); + if (g4 =3D=3D RTE_GRAPH_ID_INVALID) { + printf("Graph 4 creation failed with error =3D = %d\n", rte_errno); + return -1; + } + if (g1 =3D=3D g3 || g1 =3D=3D g4 || g3 =3D=3D g4) { + printf("Graph ids should be different\n"); + return -1; + } + g2 =3D rte_graph_clone(g1, "worker2", &gconf); + if (g2 =3D=3D RTE_GRAPH_ID_INVALID) { + printf("Graph 4 creation failed with error =3D = %d\n", rte_errno); + return -1; + } + if (g1 =3D=3D g2 || g1 =3D=3D g3 || g1 =3D=3D g4 || g2 =3D=3D g= 3 || g2 =3D=3D g4 || g3 =3D=3D g4) { + printf("Graph ids should be different\n"); + return -1; + } + if (rte_graph_destroy(g1) < 0) { + printf("Graph 1 suppression failed\n"); + return -1; + } + if (rte_graph_destroy(g2) < 0) { + printf("Graph 2 suppression failed\n"); + return -1; + } + if (rte_graph_destroy(g3) < 0) { + printf("Graph 3 suppression failed\n"); + return -1; + } + if (rte_graph_destroy(g4) < 0) { + printf("Graph 4 suppression failed\n"); + return -1; + } + return 0; +} + static int test_graph_model_mcore_dispatch_node_lcore_affinity_set(void) { @@ -977,6 +1048,7 @@ static struct unit_test_suite graph_testsuite =3D { TEST_CASE(test_lookup_functions), TEST_CASE(test_create_graph), TEST_CASE(test_graph_clone), + TEST_CASE(test_graph_id_collisions), TEST_CASE(test_graph_model_mcore_dispatch_node_= lcore_affinity_set), TEST_CASE(test_graph_model_mcore_dispatch_core_= bind_unbind), TEST_CASE(test_graph_worker_model_set_get), diff --git a/lib/graph/graph.c b/lib/graph/graph.c index 147bc6c685c5..d5b8c9f918cf 100644 --- a/lib/graph/graph.c +++ b/lib/graph/graph.c @@ -19,11 +19,54 @@ static struct graph_head graph_list =3D STAILQ_HEAD_INITIALIZER(graph_list= ); static rte_spinlock_t graph_lock =3D RTE_SPINLOCK_INITIALIZER; -static rte_graph_t graph_id; - -#define GRAPH_ID_CHECK(id) ID_CHECK(id, graph_id) /* Private functions */ +static struct graph * +graph_from_id(rte_graph_t id) +{ + struct graph *graph; + STAILQ_FOREACH(graph, &graph_list, next) { + if (graph->id =3D=3D id) + return graph; + } + rte_errno =3D EINVAL; + return NULL; +} + +static rte_graph_t +graph_next_free_id(void) +{ + struct graph *graph; + rte_graph_t id =3D 0; + + STAILQ_FOREACH(graph, &graph_list, next) { + if (id < graph->id) + break; + id =3D graph->id + 1; + } + + return id; +} + +static void +graph_insert_ordered(struct graph *graph) +{ + struct graph *after, *g; + + after =3D NULL; + STAILQ_FOREACH(g, &graph_list, next) { + if (g->id < graph->id) + after =3D g; + else if (g->id > graph->id) + break; + } + if (after =3D=3D NULL) { + STAILQ_INSERT_HEAD(&graph_list, graph, next); + } else { + STAILQ_INSERT_AFTER(&graph_list, after, graph, = next); + } +} + struct graph_head * graph_list_head_get(void) { @@ -279,7 +322,8 @@ rte_graph_model_mcore_dispatch_core_bind(rte_graph_t id= , int lcore) { struct graph *graph; - GRAPH_ID_CHECK(id); + if (graph_from_id(id) =3D=3D NULL) + goto fail; if (!rte_lcore_is_enabled(lcore)) SET_ERR_JMP(ENOLINK, fail, "lcore %d not enabl= ed", lcore); @@ -309,7 +353,8 @@ rte_graph_model_mcore_dispatch_core_unbind(rte_graph_t = id) { struct graph *graph; - GRAPH_ID_CHECK(id); + if (graph_from_id(id) =3D=3D NULL) + goto fail; STAILQ_FOREACH(graph, &graph_list, next) if (graph->id =3D=3D id) break; @@ -406,7 +451,7 @@ rte_graph_create(const char *name, struct rte_graph_par= am *prm) graph->socket =3D prm->socket_id; graph->src_node_count =3D src_node_count; graph->node_count =3D graph_nodes_count(graph); - graph->id =3D graph_id; + graph->id =3D graph_next_free_id(); graph->parent_id =3D RTE_GRAPH_ID_INVALID; graph->lcore_id =3D RTE_MAX_LCORE; graph->num_pkt_to_capture =3D prm->num_pkt_to_capture; @@ -422,8 +467,7 @@ rte_graph_create(const char *name, struct rte_graph_par= am *prm) goto graph_mem_destroy; /* All good, Lets add the graph to the list */ - graph_id++; - STAILQ_INSERT_TAIL(&graph_list, graph, next); + graph_insert_ordered(graph); graph_spinlock_unlock(); return graph->id; @@ -467,7 +511,6 @@ rte_graph_destroy(rte_graph_t id) graph_cleanup(graph); STAILQ_REMOVE(&graph_list, grap= h, graph, next); free(graph); - graph_id--; goto done; } graph =3D tmp; @@ -520,7 +563,7 @@ graph_clone(struct graph *parent_graph, const char *nam= e, struct rte_graph_param graph->parent_id =3D parent_graph->id; graph->lcore_id =3D parent_graph->lcore_id; graph->socket =3D parent_graph->socket; - graph->id =3D graph_id; + graph->id =3D graph_next_free_id(); /* Allocate the Graph fast path memory and populate the data = */ if (graph_fp_mem_create(graph)) @@ -539,8 +582,7 @@ graph_clone(struct graph *parent_graph, const char *nam= e, struct rte_graph_param goto graph_mem_destroy; /* All good, Lets add the graph to the list */ - graph_id++; - STAILQ_INSERT_TAIL(&graph_list, graph, next); + graph_insert_ordered(graph); graph_spinlock_unlock(); return graph->id; @@ -561,7 +603,8 @@ rte_graph_clone(rte_graph_t id, const char *name, struc= t rte_graph_param *prm) { struct graph *graph; - GRAPH_ID_CHECK(id); + if (graph_from_id(id) =3D=3D NULL) + goto fail; STAILQ_FOREACH(graph, &graph_list, next) if (graph->id =3D=3D id) return graph_clone(graph, name,= prm); @@ -587,7 +630,8 @@ rte_graph_id_to_name(rte_graph_t id) { struct graph *graph; - GRAPH_ID_CHECK(id); + if (graph_from_id(id) =3D=3D NULL) + goto fail; STAILQ_FOREACH(graph, &graph_list, next) if (graph->id =3D=3D id) return graph->name; @@ -604,7 +648,8 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid) rte_graph_off_t off; rte_node_t count; - GRAPH_ID_CHECK(gid); + if (graph_from_id(gid) =3D=3D NULL) + goto fail; STAILQ_FOREACH(graph, &graph_list, next) if (graph->id =3D=3D gid) { rte_graph_foreach_node(count, o= ff, graph->graph, @@ -747,7 +792,8 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all) struct graph *graph; RTE_VERIFY(f); - GRAPH_ID_CHECK(id); + if (graph_from_id(id) =3D=3D NULL) + goto fail; STAILQ_FOREACH(graph, &graph_list, next) { if (all =3D=3D true) { @@ -776,7 +822,13 @@ rte_graph_list_dump(FILE *f) rte_graph_t rte_graph_max_count(void) { - return graph_id; + struct graph *graph; + rte_graph_t count =3D 0; + + STAILQ_FOREACH(graph, &graph_list, next) + count++; + + return count; } RTE_LOG_REGISTER_DEFAULT(rte_graph_logtype, INFO); -- 2.45.2 --_000_PH0PR18MB5071831072BDDFB839186565ACCE2PH0PR18MB5071namp_ Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

 

 

From:= Robin Jarry <rjarry@redhat.com>
Sent: Tuesday, June 18, 2024 2:53 PM
To: dev@dpdk.org; Jerin Jacob <jerinj@marvell.com>; Kiran Kuma= r Kokkilagadda <kirankumark@marvell.com>; Nithin Kumar Dabilpuram <= ;ndabilpuram@marvell.com>; Zhirun Yan <yanzhirun_163@163.com>
Subject: [EXTERNAL] [PATCH v3] graph: avoid id collisions=

 

The graph id is determined based on a global va= riable that is incremented every time a graph is created, and decremented e= very time a graph is destroyed. This only works if graphs are destroyed in the reverse order in which they have been= created.=E2=80=8A 

The graph id is determined based on a gl=
obal variable that is
incremented every time a graph is created, and decremented every time
a graph is destroyed. This only works if graphs are destroyed in the
reverse order in which they have been created.
 
The following code produces duplicate graph IDs which can lead to<=
/o:p>
use-after-free bugs and other undefined behaviours:

 


  a =3D rte_graph_create(...); // id=3D0 graph_id=3D1


  b =3D rte_graph_create(...); // id=3D1 graph_id=3D2


  rte_graph_destroy(a);      // graph_id=
=3D1


  c =3D rte_graph_create(...); // id=3D1 graph_id=3D2 (duplicate w=
ith b)


  rte_graph_destroy(c);      // frees mem=
ory still used by b


 


Remove the global counter. Make sure that the graph list is always=



ordered by increasing graph ids. When creating a new graph, pick a free=



id which is not allocated.


 


Update unit tests to ensure it works as expected.

 




Signed-off-by: Robin Jarry <rjarry@redhat.com><=
/pre>



 


Acked-by: Kiran Kumar Kokkilagadda kirankumark@marvell.com


 


Notes:


    v3:


    


    * Fixed invalid logic in graph_insert_ordered.<=
o:p>


    * Updated unit test to ensure head/tail/middle inser=
tion work (the test


      fails before fixing graph_insert_ordered=
).


 


 app/test/test_graph.c | 72 ++++++++++++++++++++++++++++++++++++


 lib/graph/graph.c     | 86 +++++++++++++++++++++++=
+++++++++++---------


 2 files changed, 141 insertions(+), 17 deletions(-)<=
/pre>

 


diff --git a/app/test/test_graph.c b/app/test/test_graph.c


index b8409bc60497..2840a25b13b7 100644


--- a/app/test/test_graph.c


+++ b/app/test/test_graph.c


@@ -696,6 +696,77 @@ test_graph_clone(void)


            &nbs=
p; return ret;


 }


 


+static int


+test_graph_id_collisions(void)


+{


+           static co=
nst char *node_patterns[] =3D {"test_node_source1", "test_no=
de00"};


+           struct rt=
e_graph_param gconf =3D {


+           &nbs=
p;            &=
nbsp;  .socket_id =3D SOCKET_ID_ANY,


+           &nbs=
p;            &=
nbsp;  .nb_node_patterns =3D 2,


+           &nbs=
p;            &=
nbsp;  .node_patterns =3D node_patterns,


+           };


+           rte_graph=
_t g1, g2, g3, g4;


+


+           g1 =3D rt=
e_graph_create("worker1", &gconf);


+           if (g1 =
=3D=3D RTE_GRAPH_ID_INVALID) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 1 creation failed with error =3D %d\n",=
 rte_errno);


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           g2 =3D rt=
e_graph_create("worker2", &gconf);


+           if (g2 =
=3D=3D RTE_GRAPH_ID_INVALID) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 2 creation failed with error =3D %d\n",=
 rte_errno);


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           g3 =3D rt=
e_graph_create("worker3", &gconf);


+           if (g3 =
=3D=3D RTE_GRAPH_ID_INVALID) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 3 creation failed with error =3D %d\n",=
 rte_errno);


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (g1 =
=3D=3D g2 || g2 =3D=3D g3 || g1 =3D=3D g3) {


+           &nbs=
p;            &=
nbsp;  printf("Graph ids should be different\n");=



+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (rte_g=
raph_destroy(g2) < 0) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 2 suppression failed\n");


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           g4 =3D rt=
e_graph_create("worker4", &gconf);


+           if (g4 =
=3D=3D RTE_GRAPH_ID_INVALID) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 4 creation failed with error =3D %d\n",=
 rte_errno);


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (g1 =
=3D=3D g3 || g1 =3D=3D g4 || g3 =3D=3D g4) {


+           &nbs=
p;            &=
nbsp;  printf("Graph ids should be different\n");=



+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           g2 =3D rt=
e_graph_clone(g1, "worker2", &gconf);


+           if (g2 =
=3D=3D RTE_GRAPH_ID_INVALID) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 4 creation failed with error =3D %d\n",=
 rte_errno);


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (g1 =
=3D=3D g2 || g1 =3D=3D g3 || g1 =3D=3D g4 || g2 =3D=3D g3 || g2 =3D=3D g4 |=
| g3 =3D=3D g4) {


+           &nbs=
p;            &=
nbsp;  printf("Graph ids should be different\n");=



+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (rte_g=
raph_destroy(g1) < 0) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 1 suppression failed\n");


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (rte_g=
raph_destroy(g2) < 0) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 2 suppression failed\n");


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (rte_g=
raph_destroy(g3) < 0) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 3 suppression failed\n");


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           if (rte_g=
raph_destroy(g4) < 0) {


+           &nbs=
p;            &=
nbsp;  printf("Graph 4 suppression failed\n");


+           &nbs=
p;            &=
nbsp;  return -1;


+           }


+           return 0;=



+}


+


 static int


 test_graph_model_mcore_dispatch_node_lcore_affinity_set(void)


 {


@@ -977,6 +1048,7 @@ static struct unit_test_suite graph_testsuite =3D =
{


            &nbs=
p;            &=
nbsp;   TEST_CASE(test_lookup_functions),


            &nbs=
p;            &=
nbsp;   TEST_CASE(test_create_graph),


            &nbs=
p;            &=
nbsp;   TEST_CASE(test_graph_clone),


+           &nbs=
p;            &=
nbsp;  TEST_CASE(test_graph_id_collisions),


            &nbs=
p;             =
  TEST_CASE(test_graph_model_mcore_dispatch_node_lcore_affinity_set),<=
o:p>


            &nbs=
p;             =
  TEST_CASE(test_graph_model_mcore_dispatch_core_bind_unbind),


            &nbs=
p;            &=
nbsp;   TEST_CASE(test_graph_worker_model_set_get),


diff --git a/lib/graph/graph.c b/lib/graph/graph.c

index 147bc6c685c5..d5b8c9f918cf 100644


--- a/lib/graph/graph.c


+++ b/lib/graph/graph.c


@@ -19,11 +19,54 @@


 


 static struct graph_head graph_list =3D STAILQ_HEAD_INITIALIZER(g=
raph_list);


 static rte_spinlock_t graph_lock =3D RTE_SPINLOCK_INITIALIZER;


-static rte_graph_t graph_id;


-


-#define GRAPH_ID_CHECK(id) ID_CHECK(id, graph_id)

 


 /* Private functions */


+static struct graph *


+graph_from_id(rte_graph_t id)


+{


+           struct gr=
aph *graph;


+           STAILQ_FO=
REACH(graph, &graph_list, next) {


+           &nbs=
p;            &=
nbsp;  if (graph->id =3D=3D id)


+           &nbs=
p;            &=
nbsp;           &nbs=
p;     return graph;


+           }


+           rte_errno=
 =3D EINVAL;


+           return NU=
LL;


+}


+


+static rte_graph_t


+graph_next_free_id(void)


+{


+           struct gr=
aph *graph;


+           rte_graph=
_t id =3D 0;


+


+           STAILQ_FO=
REACH(graph, &graph_list, next) {


+           &nbs=
p;            &=
nbsp;  if (id < graph->id)


+           &nbs=
p;            &=
nbsp;           &nbs=
p;     break;


+           &nbs=
p;            &=
nbsp;  id =3D graph->id + 1;


+           }


+


+           return id=
;


+}


+


+static void


+graph_insert_ordered(struct graph *graph)


+{


+           struct gr=
aph *after, *g;


+


+           after =3D=
 NULL;


+           STAILQ_FO=
REACH(g, &graph_list, next) {


+           &nbs=
p;            &=
nbsp;  if (g->id < graph->id)


+           &nbs=
p;            &=
nbsp;           &nbs=
p;     after =3D g;


+           &nbs=
p;            &=
nbsp;  else if (g->id > graph->id)


+           &nbs=
p;            &=
nbsp;           &nbs=
p;     break;


+           }


+           if (after=
 =3D=3D NULL) {


+           &nbs=
p;            &=
nbsp;  STAILQ_INSERT_HEAD(&graph_list, graph, next);


+           } else {<=
o:p>


+           &nbs=
p;            &=
nbsp;  STAILQ_INSERT_AFTER(&graph_list, after, graph, next);<=
/o:p>


+           }


+}


+


 struct graph_head *


 graph_list_head_get(void)


 {


@@ -279,7 +322,8 @@ rte_graph_model_mcore_dispatch_core_bind(rte_graph_=
t id, int lcore)


 {


            &nbs=
p; struct graph *graph;


 


-            GRA=
PH_ID_CHECK(id);


+           if (graph=
_from_id(id) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


            &nbs=
p; if (!rte_lcore_is_enabled(lcore))


            &nbs=
p;            &=
nbsp;   SET_ERR_JMP(ENOLINK, fail, "lcore %d not enabled&quo=
t;, lcore);


 


@@ -309,7 +353,8 @@ rte_graph_model_mcore_dispatch_core_unbind(rte_grap=
h_t id)


 {


            &nbs=
p; struct graph *graph;


 


-            GRA=
PH_ID_CHECK(id);


+           if (graph=
_from_id(id) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


            &nbs=
p; STAILQ_FOREACH(graph, &graph_list, next)


            &nbs=
p;            &=
nbsp;   if (graph->id =3D=3D id)


            &nbs=
p;            &=
nbsp;           &nbs=
p;      break;


@@ -406,7 +451,7 @@ rte_graph_create(const char *name, struct rte_graph=
_param *prm)


            &nbs=
p; graph->socket =3D prm->socket_id;


            &nbs=
p; graph->src_node_count =3D src_node_count;


            &nbs=
p; graph->node_count =3D graph_nodes_count(graph);

-            gra=
ph->id =3D graph_id;


+           graph->=
;id =3D graph_next_free_id();


            &nbs=
p; graph->parent_id =3D RTE_GRAPH_ID_INVALID;


            &nbs=
p; graph->lcore_id =3D RTE_MAX_LCORE;


            &nbs=
p; graph->num_pkt_to_capture =3D prm->num_pkt_to_capture;<=
/span>


@@ -422,8 +467,7 @@ rte_graph_create(const char *name, struct rte_graph=
_param *prm)


            &nbs=
p;            &=
nbsp;   goto graph_mem_destroy;


 


            =
;  /* All good, Lets add the graph to the list */

-            gra=
ph_id++;


-            STA=
ILQ_INSERT_TAIL(&graph_list, graph, next);


+           graph_ins=
ert_ordered(graph);


 


            =
;  graph_spinlock_unlock();


            &nbs=
p; return graph->id;


@@ -467,7 +511,6 @@ rte_graph_destroy(rte_graph_t id)=



            &nbs=
p;            &=
nbsp;           &nbs=
p;      graph_cleanup(graph);

            &nbs=
p;            &=
nbsp;           &nbs=
p;      STAILQ_REMOVE(&graph_list, graph, grap=
h, next);


            &nbs=
p;            &=
nbsp;           &nbs=
p;      free(graph);


-           &nbs=
p;            &=
nbsp;           &nbs=
p;      graph_id--;


            &nbs=
p;            &=
nbsp;           &nbs=
p;      goto done;


            &nbs=
p;            &=
nbsp;   }


            &nbs=
p;            &=
nbsp;   graph =3D tmp;


@@ -520,7 +563,7 @@ graph_clone(struct graph *parent_graph, const char =
*name, struct rte_graph_param


            &nbs=
p; graph->parent_id =3D parent_graph->id;


            &nbs=
p; graph->lcore_id =3D parent_graph->lcore_id;


            &nbs=
p; graph->socket =3D parent_graph->socket;


-            gra=
ph->id =3D graph_id;


+           graph->=
;id =3D graph_next_free_id();


 


            =
;  /* Allocate the Graph fast path memory and populate the data */


            &nbs=
p; if (graph_fp_mem_create(graph))


@@ -539,8 +582,7 @@ graph_clone(struct graph *parent_graph, const char =
*name, struct rte_graph_param


            &nbs=
p;            &=
nbsp;   goto graph_mem_destroy;


 


            =
;  /* All good, Lets add the graph to the list */

-            gra=
ph_id++;


-            STA=
ILQ_INSERT_TAIL(&graph_list, graph, next);


+           graph_ins=
ert_ordered(graph);


 


            =
;  graph_spinlock_unlock();


            &nbs=
p; return graph->id;


@@ -561,7 +603,8 @@ rte_graph_clone(rte_graph_t id, const char *name, s=
truct rte_graph_param *prm)


 {


            &nbs=
p; struct graph *graph;


 


-            GRA=
PH_ID_CHECK(id);


+           if (graph=
_from_id(id) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


            &nbs=
p; STAILQ_FOREACH(graph, &graph_list, next)


            &nbs=
p;            &=
nbsp;   if (graph->id =3D=3D id)


            &nbs=
p;            &=
nbsp;           &nbs=
p;      return graph_clone(graph, name, prm);=



@@ -587,7 +630,8 @@ rte_graph_id_to_name(rte_graph_t id)


 {


            &nbs=
p; struct graph *graph;


 


-            GRA=
PH_ID_CHECK(id);


+           if (graph=
_from_id(id) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


            &nbs=
p; STAILQ_FOREACH(graph, &graph_list, next)


            &nbs=
p;            &=
nbsp;   if (graph->id =3D=3D id)


            &nbs=
p;            &=
nbsp;           &nbs=
p;      return graph->name;

@@ -604,7 +648,8 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid)


            &nbs=
p; rte_graph_off_t off;


            &nbs=
p; rte_node_t count;


 


-            GRA=
PH_ID_CHECK(gid);


+           if (graph=
_from_id(gid) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


            &nbs=
p; STAILQ_FOREACH(graph, &graph_list, next)


            &nbs=
p;            &=
nbsp;   if (graph->id =3D=3D gid) {


            &nbs=
p;            &=
nbsp;           &nbs=
p;      rte_graph_foreach_node(count, off, graph-&=
gt;graph,


@@ -747,7 +792,8 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all)<=
o:p>


            &nbs=
p; struct graph *graph;


 


            =
;  RTE_VERIFY(f);


-            GRA=
PH_ID_CHECK(id);


+           if (graph=
_from_id(id) =3D=3D NULL)


+           &nbs=
p;            &=
nbsp;  goto fail;


 


            =
;  STAILQ_FOREACH(graph, &graph_list, next) {

            &nbs=
p;            &=
nbsp;   if (all =3D=3D true) {


@@ -776,7 +822,13 @@ rte_graph_list_dump(FILE *f)

 rte_graph_t


 rte_graph_max_count(void)


 {


-            ret=
urn graph_id;


+           struct gr=
aph *graph;


+           rte_graph=
_t count =3D 0;


+


+           STAILQ_FO=
REACH(graph, &graph_list, next)


+           &nbs=
p;            &=
nbsp;  count++;


+


+           return co=
unt;


 }


 


 RTE_LOG_REGISTER_DEFAULT(rte_graph_logtype, INFO);


-- 


2.45.2


 
--_000_PH0PR18MB5071831072BDDFB839186565ACCE2PH0PR18MB5071namp_--