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 4B78D4296F; Mon, 17 Apr 2023 18:37:56 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1E00140DFB; Mon, 17 Apr 2023 18:37:56 +0200 (CEST) Received: from NAM12-DM6-obe.outbound.protection.outlook.com (mail-dm6nam12on2076.outbound.protection.outlook.com [40.107.243.76]) by mails.dpdk.org (Postfix) with ESMTP id 1036D40698 for ; Mon, 17 Apr 2023 18:37:54 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PepBN+3ZJnNrG3EfUpsJcHR3VuZsnireHSR8TsyxfU3yuposO56kKyXAFgfxtfGIEbt0TLAkDA1yZ1jJDzEriWACUWgbJoY7lV6POMoZikQFglajboreOznUxtayqnP2TGIIbXg7x5wdC8e4xC6/miyB1u1xzffsPyVQoKIAFnr5sboP3EYzLt36skDDbyztxqeTlCvikYMkmJWXF55a09yMSgAnDQ5M0Zo+cVLZf/gxw0f1VYQDrpZV1JA01oJaU139fUcm7XhLazXriYyMgcLrz7hNUF/W8j6pzmh1AGPf20tmAnX72btyVWy3WbX4KmH1uo1BFlCbd2ZP4LQ3Ew== 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=kkUym/0d9kL65GnkigAhgKpX8FdumtFAX9SoIPhEKok=; b=fxF94xYpUlpPtxfp5Xal8GG63B++nrZl7LuYah0VH/JivIyE+L9Wm9PGvG8zG7smbkYKfsEtix1Eh9qKTSBxt+KdAErKwyGbjXPtZ0Ejt5ErWVumUVBThF8snm0R/thGHAjiDgiwdakOCfryFt7YmTQAr+gT6Ys37iTaRJi1uxXiBV4x2YO7Eo4dS7t9IRz7OFw/rTIrCtZwlLffbJvJx6VZnYoeBKjSlOEaKTmjSmhY2NsXFW17bFci0DbXEmVb2zhUir7Zrvd7+Oren5fW3q7k7ojgK85mPI/ZolGu7K1ys+YKx6NqBh8Lp3fP7cbPd6m5wFpcqHFrPQpRGE/0Sw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=kkUym/0d9kL65GnkigAhgKpX8FdumtFAX9SoIPhEKok=; b=mDJyCiCzsF4MlQ66clbBb30fhbYW3zuJwGv9ntGhOlFd+OYuvON6vaXnCm79LNuwEiEDEYQhY2W835jt9mOCnLh1IKx/cOAnYfQeE7WS8iuETGgLCPR+0tbfHCTPyAnAg2clX0dYM9nHKojRdWaF17bcViGH2654r6si7kdM6yU= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; Received: from CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) by DM6PR12MB4974.namprd12.prod.outlook.com (2603:10b6:5:1bb::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6298.45; Mon, 17 Apr 2023 16:37:52 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::5e2c:c0ed:88a6:a4c7]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::5e2c:c0ed:88a6:a4c7%7]) with mapi id 15.20.6298.045; Mon, 17 Apr 2023 16:37:52 +0000 Message-ID: <1c49ce5b-4109-8621-ee9e-4a9f575280ee@amd.com> Date: Mon, 17 Apr 2023 17:37:46 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Content-Language: en-US To: fengchengwen , thomas@monjalon.net Cc: dev@dpdk.org References: <20230314124813.39521-1-fengchengwen@huawei.com> <20230320092110.37295-1-fengchengwen@huawei.com> <957bcf61-090d-a0ed-afb1-94897993bcb2@huawei.com> From: Ferruh Yigit Subject: Re: [PATCH v2 00/44] fix segment fault when parse args In-Reply-To: <957bcf61-090d-a0ed-afb1-94897993bcb2@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO2P265CA0073.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:8::13) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|DM6PR12MB4974:EE_ X-MS-Office365-Filtering-Correlation-Id: f5cf36c0-8a92-4f1b-863a-08db3f62165c X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 15JofZaMjr9mPjDZ4d8yftlbe6C6Su5nrVDgtPBtgma2T17EiI3nnd1G/6XrS3SFdkiTV3ZdRJq+2jjSWbDWdqf4UjUgm73koJoBTq74rUenHa0lYd+8Tg46Uv25MyfqwPItexBC6fLQQrG8Y13VlMPmZOldqta/rXLGoZ5U7nTCIXQE11vKoCd777yZmeEHKf+Gi8eS4IOzxWTYfG7HN1dzdu47qnJFKwbhLaR6CddqnUe+K54NI96Op4eZjvbfD4uCRXDjJbtTlDNo1e0yRWYDH0uHWXL9eBfhVNli/iHVSEV1FyvwPES/thou/6lorel80qd9oAqnCjZUN6bFQMCqBL/Okq79cTOl50/M6sPh/U8q6BDZNbdcKOe2SrOqQ1sdQeF6U/x+q0XsZSNxfZuNb+fpoWED1wDqMn0WvkqjjFpyhnw6j5+gNS10pNcLI68gQi71EJfhq4DZKE0Jv8hf4ixJEOB0ykv4pE1w5UaMR8bUi0j3vaLUsZ3eRIF/L3ZuIqywaspc2YtiK8bT1w0TrrtnGL2b91iftH4scfBioHp2T03xIseZMsBkYWmxF1PWLVH5qSrL4UvarjtHXa3CmArvN4nXpWUiwtVwPPZpEjp2dSQPcJoorG+n4ZVBWNshwffPjWlc+P0ZU8sR6A== X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH2PR12MB4294.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230028)(4636009)(396003)(376002)(366004)(346002)(39860400002)(136003)(451199021)(6666004)(6486002)(478600001)(86362001)(31696002)(2616005)(36756003)(26005)(83380400001)(6506007)(186003)(6512007)(53546011)(38100700002)(66946007)(66476007)(66556008)(316002)(2906002)(4326008)(8936002)(5660300002)(8676002)(44832011)(31686004)(41300700001)(66899021)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bitBVE1WZE1TQmp2V094UGRHQlZXejQzNFRQamVGNVNjbzJyQ3VtdkZscE1x?= =?utf-8?B?ODgxSjdVNkJOVGY3d1Q0TGg1N0YzeWI5eWdIYW9BTG43eFJjYTBWTFFTenR4?= =?utf-8?B?WkR3a0dzOFNFSkEyUXFUbGd0dE8xTC9LQUVUYnlIbGx3YSswQlJvMTZTQnRp?= =?utf-8?B?S25lT2plK05lWDEyclY0ck1kdmFTa0hOdnZDQlpMZk9UUjhIbXVyYUhBS3g3?= =?utf-8?B?dDJIampYak5YYUxyajVFQndTRnFRMDUrWWNucUovazh3ak84ZlpjZ2pqVXlB?= =?utf-8?B?NXNON0doVzhqdXFPbnV5dmdjWU44OUtLMWYydHNVL3BJM3dWelpBMVlLRSty?= =?utf-8?B?S0s5M1QwaUNCUFN6OTBobGMxdlZqMVVES3Fvc3J1VlpLQXJHRTRWSWNLZDh2?= =?utf-8?B?allQSURoSmNTZWMzaUpSeFFTNm9kdjNMUlBwSUpJa0htWHJrTHFGWFVFdVBq?= =?utf-8?B?WUNadmtHbGxWbFFEUXV6ZjlGV3JmSUVJTWkvdm85QjJLeVNNdXBUcm9JU05B?= =?utf-8?B?VnAvQUZjeWQ1dCtPMng3VkxCeEJJR2tyNGpzYW5UbXhqaHNLWEhXS2QyTzZk?= =?utf-8?B?Q2YzcHBIWFRmeDU1RGJ4Qm5GZHkvMC9zT2grRWM4YUplcXYvbVVTSVFUamhw?= =?utf-8?B?dEljQlRLdzRlZDNsZThxaXU3ajhSalRKVkJrVjNYa2VlMGhvVkRyMVZUYTJX?= =?utf-8?B?bFljNHZlV2JVY096dlJpUE8zTFlxdUx5eFJ5Q1BZKzI3R3VPcCsxWjZrTXZ4?= =?utf-8?B?VUhxMTZJTzhkQUU3SGVxZUhUdjNrbUhTYXQxMVlSV25wM0RGRG1UWVp0alFk?= =?utf-8?B?c1QxdytnYnlSQWZ6VnQ3ZHBjWEVHVThza2drQ1ltb1lXbERsZnBNUjJud0gy?= =?utf-8?B?a0VNOXByWE1xRmE2Qk5PMzRwQ2ZiSTZZeDFlS0g4cnBudmhVaHN2c1kvYzRR?= =?utf-8?B?d21QeHdmN1V5VGk5cENyMFQrN0FJV2hWVkgzaG41c3dUWjgwTEFWQTZkTzRm?= =?utf-8?B?bWgxclE4ckVnc2lLYk9Bd05DWW9PMjFPOUVTNEE2a1ppaHZBMVBjTU9VNFhi?= =?utf-8?B?SVZmZVNBMG1jbDczUndsZkFBazVHbUpsT29XczZjcjhQNFVmNHVWRUVzUHlr?= =?utf-8?B?bi9Jd1QvaUxKQm8zaDIvRDNMakhTekR3U09JYWVLTG00ZGFIUURJVUMraTI1?= =?utf-8?B?R2d4V0R6aHpVZTNpdTRySWUrYmtiNFlLb20xeXVtem5iUU9lSnRlNzZSbGtO?= =?utf-8?B?Y1cvNUo4dklvRWdZM0VSSEtGQ21JYmhNMmVTVHFxNWgwSGtNYTd0Sk9vZ2tI?= =?utf-8?B?SHdieEVmNllpMk56UElhemdJTkg5NnBSQ0cvVFRDQVVPbWZLK1JpYUpDY2pI?= =?utf-8?B?MkxXNlZpdE1oaHVBSEcvRWRmaWlub0pMZCt4blpWT0Z4Z1V2cE5MZVphY1Uy?= =?utf-8?B?VSs5dXUwVkc2OHZJb2Q4RUNIcGpyVjQ3NVU3bTVLbW0xb1I2cTY5VGdiWUhY?= =?utf-8?B?TVJmZWhyTWE3WUVSUi9kaGJlemR2WGNueXVLQUhmT2RjcFpvL28wUlR6RFQy?= =?utf-8?B?V3FIYTlXTFJpUytoWnQzejdkUVBPWldqR2xVQTdXUnFteVhISmRYU05MUW91?= =?utf-8?B?d1BZYVo1UHJjSnNQZmhIK0p1L3RaZVExbTdyRkYzeVBkTFZLNXB4V1VIdG5Z?= =?utf-8?B?OU5EVWdyODA0Y1BXZ21DWFYxWVIvVFFjdlR5bkdoN0l0M2ZOMkkyTXFVMDAv?= =?utf-8?B?bFdicFRDWGlCMitlNjFCT0VLTDZvbTBRQzdpRzZ1ZW83cHgxMkRxVDRDaDBQ?= =?utf-8?B?MlB4WnZjcVE3TnZ6WnhGc3NrSFdnblkyZXV0VTNaWm9LYzEvWG1rek5FeWV0?= =?utf-8?B?S3ZBWEdYcUV4YWl6czZKUkg5U1BvMGFHN0NoV3ByaXQreHFBend3RWFwUUhS?= =?utf-8?B?Q21Gd0pLb1U2SC9IZEUvYlFuRXI3RVJEVDEyVXVFMVdGRnpDSjY1cFE3VWdF?= =?utf-8?B?dElvVGorUnRoNDRKNzhpaEN3NVJmR0dZeFpYTEdXN0xPbzNFODd2S1NIN0Rz?= =?utf-8?B?SVhwdCt2SERSV0pnUDRrOUptYWtuOE1xTmJPa25qQjJsMnhDR3BSZDlnREg3?= =?utf-8?Q?7zgESkGKPmOU8sE8s5HCyJMtN?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: f5cf36c0-8a92-4f1b-863a-08db3f62165c X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2023 16:37:52.0653 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: nFZbIRsUWv4inV7o/SDr87GbmGp43hsGUidC61vIMOwzvcEy9mvrp3eSxngf9q9u X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4974 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 On 4/15/2023 2:38 AM, fengchengwen wrote: > Hi Thomas, Ferruh, > > This patch-set get almost 30% ack by PMD's maintainer. > Could it be applied? and squeeze the patch-set is okay. > Hi Chengwen, The patch is trivial and safe on its own, so for me having enough ack or not is not what blocks the set. As we discussed before, instead of adding NULL checks to the callbacks, it is better to handle this in the kvargs API level, that discussion is holding this patchset back. According result of discussion we may prefer to not merge this patch at all. > Another thread talk about a change in kvargs API, we could discuss here. > My opinion: > 1) the below PMD has the bug, but there are also many PMD take care of only key situation. > I think it mainly because the API definition is not detailed, but this hole was already > fill by commit: 52ab17efdec. Ack > 2) the rte_kvargs_get API, I think we could refine it definition, because it can't identify > invalid keys and only-keys (both of them return NULL), and I suggest add one extra parameter: > const char *rte_kvargs_get(const struct rte_kvargs *kvlist, const char *key, bool *key_exist); > I am not clear why to update `rte_kvargs_get()`, and how to benefit from this update. My suggestion was to update kvargs API to let user of API define if 'key' should have a 'value' or not, similar to done in `getopt()` API. One option can be to update `rte_kvargs_parse()` API to recognize a special char in "const char *const valid_keys[]" to know if value expected or not, and API can do internal checks based on this information. To reduce the impact of change, no special char in the valid_keys string may mean key expects a value (I think this is the most common case), and in this case if "value == NULL" API can return an error. But if there is a special char at the end, lets say '-', key doesn't need a value and having "value == NULL" is accepted. Again this is similar to ':' char in the `getopt()` API, but meaning is reverse. Above is just an option, there can be another solution by to updating `rte_kvargs_process()` API, only this may be more disruptive. Any other solution is welcomed. > Thanks. > > On 2023/3/20 17:20, Chengwen Feng wrote: >> The rte_kvargs_process() was used to parse KV pairs, it also supports >> to parse 'only keys' (e.g. socket_id) type. And the callback function >> parameter 'value' is NULL when parsed 'only keys'. >> >> It may leads to segment fault when parse args with 'only key', this >> patchset fixes rest of them. >> >> Chengwen Feng (44): >> app/pdump: fix segment fault when parse args >> ethdev: fix segment fault when parse args >> net/memif: fix segment fault when parse devargs >> net/pcap: fix segment fault when parse devargs >> net/ring: fix segment fault when parse devargs >> net/sfc: fix segment fault when parse devargs >> net/af_xdp: fix segment fault when parse devargs >> net/ark: fix segment fault when parse devargs >> net/cnxk: fix segment fault when parse devargs >> net/cxgbe: fix segment fault when parse devargs >> net/dpaa2: fix segment fault when parse devargs >> net/ena: fix segment fault when parse devargs >> net/enic: fix segment fault when parse devargs >> net/fm10k: fix segment fault when parse devargs >> net/i40e: fix segment fault when parse devargs >> net/iavf: fix segment fault when parse devargs >> net/ice: fix segment fault when parse devargs >> net/idpf: fix segment fault when parse devargs >> net/ionic: fix segment fault when parse devargs >> net/mana: fix segment fault when parse devargs >> net/mlx4: fix segment fault when parse devargs >> net/mvneta: fix segment fault when parse devargs >> net/mvpp2: fix segment fault when parse devargs >> net/netvsc: fix segment fault when parse devargs >> net/octeontx: fix segment fault when parse devargs >> net/pfe: fix segment fault when parse devargs >> net/qede: fix segment fault when parse devargs >> baseband/la12xx: fix segment fault when parse devargs >> bus/pci: fix segment fault when parse args >> common/mlx5: fix segment fault when parse devargs >> crypto/cnxk: fix segment fault when parse devargs >> crypto/dpaa_sec: fix segment fault when parse devargs >> crypto/dpaa2_sec: fix segment fault when parse devargs >> crypto/mvsam: fix segment fault when parse devargs >> crypto/scheduler: fix segment fault when parse devargs >> dma/dpaa2: fix segment fault when parse devargs >> event/cnxk: fix segment fault when parse devargs >> event/dlb2: fix segment fault when parse devargs >> event/dpaa: fix segment fault when parse devargs >> event/octeontx: fix segment fault when parse devargs >> event/opdl: fix segment fault when parse devargs >> event/sw: fix segment fault when parse devargs >> mempool/cnxk: fix segment fault when parse devargs >> raw/cnxk_gpio: fix segment fault when parse devargs >> >> --- >> v2: according Ferruh's comments: >> fix all 'rte_kvargs_process()' bug instances. >> only judge value validation. >> >> app/pdump/main.c | 12 ++++++ >> drivers/baseband/la12xx/bbdev_la12xx.c | 3 ++ >> drivers/bus/pci/pci_params.c | 2 + >> drivers/common/mlx5/mlx5_common.c | 5 +++ >> drivers/crypto/cnxk/cnxk_cryptodev_devargs.c | 3 ++ >> drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 2 + >> drivers/crypto/dpaa_sec/dpaa_sec.c | 3 ++ >> drivers/crypto/mvsam/rte_mrvl_pmd.c | 6 +++ >> drivers/crypto/scheduler/scheduler_pmd.c | 21 +++++++++++ >> drivers/dma/dpaa2/dpaa2_qdma.c | 3 ++ >> drivers/event/cnxk/cnxk_eventdev.c | 6 +++ >> drivers/event/cnxk/cnxk_eventdev.h | 6 +++ >> drivers/event/cnxk/cnxk_tim_evdev.c | 6 +++ >> drivers/event/dlb2/dlb2.c | 5 ++- >> drivers/event/dpaa/dpaa_eventdev.c | 3 ++ >> drivers/event/octeontx/ssovf_evdev.c | 2 + >> drivers/event/opdl/opdl_evdev.c | 9 +++++ >> drivers/event/sw/sw_evdev.c | 12 ++++++ >> drivers/mempool/cnxk/cnxk_mempool.c | 3 ++ >> drivers/net/af_xdp/rte_eth_af_xdp.c | 12 ++++++ >> drivers/net/ark/ark_ethdev.c | 3 ++ >> drivers/net/cnxk/cnxk_ethdev_devargs.c | 39 ++++++++++++++++++++ >> drivers/net/cnxk/cnxk_ethdev_sec.c | 12 ++++++ >> drivers/net/cxgbe/cxgbe_main.c | 3 ++ >> drivers/net/dpaa2/dpaa2_ethdev.c | 3 ++ >> drivers/net/ena/ena_ethdev.c | 6 +++ >> drivers/net/enic/enic_ethdev.c | 6 +++ >> drivers/net/fm10k/fm10k_ethdev.c | 3 ++ >> drivers/net/i40e/i40e_ethdev.c | 15 ++++++++ >> drivers/net/iavf/iavf_ethdev.c | 6 +++ >> drivers/net/ice/ice_dcf_ethdev.c | 6 +++ >> drivers/net/ice/ice_ethdev.c | 6 +++ >> drivers/net/idpf/idpf_ethdev.c | 6 +++ >> drivers/net/ionic/ionic_dev_pci.c | 3 ++ >> drivers/net/mana/mana.c | 3 ++ >> drivers/net/memif/rte_eth_memif.c | 30 +++++++++++++++ >> drivers/net/mlx4/mlx4.c | 3 ++ >> drivers/net/mvneta/mvneta_ethdev.c | 3 ++ >> drivers/net/mvpp2/mrvl_ethdev.c | 3 ++ >> drivers/net/mvpp2/mrvl_qos.c | 6 ++- >> drivers/net/netvsc/hn_ethdev.c | 3 ++ >> drivers/net/octeontx/octeontx_ethdev.c | 3 ++ >> drivers/net/pcap/pcap_ethdev.c | 18 ++++++++- >> drivers/net/pfe/pfe_ethdev.c | 3 ++ >> drivers/net/qede/qede_ethdev.c | 3 ++ >> drivers/net/ring/rte_eth_ring.c | 6 +++ >> drivers/net/sfc/sfc.c | 3 ++ >> drivers/net/sfc/sfc_ev.c | 3 ++ >> drivers/net/sfc/sfc_kvargs.c | 6 +++ >> drivers/raw/cnxk_gpio/cnxk_gpio.c | 6 +++ >> lib/ethdev/rte_class_eth.c | 6 +++ >> 51 files changed, 345 insertions(+), 4 deletions(-) >>