From: Ferruh Yigit <ferruh.yigit@amd.com>
To: fengchengwen <fengchengwen@huawei.com>, thomas@monjalon.net
Cc: dev@dpdk.org
Subject: Re: [PATCH v2 00/44] fix segment fault when parse args
Date: Mon, 17 Apr 2023 17:37:46 +0100 [thread overview]
Message-ID: <1c49ce5b-4109-8621-ee9e-4a9f575280ee@amd.com> (raw)
In-Reply-To: <957bcf61-090d-a0ed-afb1-94897993bcb2@huawei.com>
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(-)
>>
next prev parent reply other threads:[~2023-04-17 16:37 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-14 12:48 [PATCH 0/5] " Chengwen Feng
2023-03-14 12:48 ` [PATCH 1/5] app/pdump: " Chengwen Feng
2023-03-16 17:34 ` Ferruh Yigit
2023-03-14 12:48 ` [PATCH 2/5] net/memif: fix segment fault when parse devargs Chengwen Feng
2023-03-16 18:20 ` Ferruh Yigit
2023-03-14 12:48 ` [PATCH 3/5] net/pcap: " Chengwen Feng
2023-03-16 18:20 ` Ferruh Yigit
2023-03-14 12:48 ` [PATCH 4/5] net/ring: " Chengwen Feng
2023-03-14 12:48 ` [PATCH 5/5] net/sfc: " Chengwen Feng
2023-03-16 18:20 ` Ferruh Yigit
2023-03-16 18:18 ` [PATCH 0/5] fix segment fault when parse args Ferruh Yigit
2023-03-17 2:43 ` fengchengwen
2023-03-21 13:50 ` Ferruh Yigit
2023-03-22 1:15 ` fengchengwen
2023-03-22 8:53 ` Ferruh Yigit
2023-03-22 13:49 ` Thomas Monjalon
2023-03-23 11:58 ` fengchengwen
2023-03-23 12:51 ` Thomas Monjalon
2023-03-20 9:20 ` [PATCH v2 00/44] " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 01/44] app/pdump: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 02/44] ethdev: " Chengwen Feng
2023-04-09 8:10 ` Andrew Rybchenko
2023-03-20 9:20 ` [PATCH v2 03/44] net/memif: fix segment fault when parse devargs Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 04/44] net/pcap: " Chengwen Feng
2023-07-04 3:04 ` Stephen Hemminger
2023-03-20 9:20 ` [PATCH v2 05/44] net/ring: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 06/44] net/sfc: " Chengwen Feng
2023-04-09 8:09 ` Andrew Rybchenko
2023-03-20 9:20 ` [PATCH v2 07/44] net/af_xdp: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 08/44] net/ark: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 09/44] net/cnxk: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 10/44] net/cxgbe: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 11/44] net/dpaa2: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 12/44] net/ena: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 13/44] net/enic: " Chengwen Feng
2023-03-20 21:37 ` John Daley (johndale)
2023-03-20 9:20 ` [PATCH v2 14/44] net/fm10k: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 15/44] net/i40e: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 16/44] net/iavf: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 17/44] net/ice: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 18/44] net/idpf: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 19/44] net/ionic: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 20/44] net/mana: " Chengwen Feng
2023-03-21 22:19 ` Long Li
2023-03-20 9:20 ` [PATCH v2 21/44] net/mlx4: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 22/44] net/mvneta: " Chengwen Feng
2023-03-20 9:33 ` [EXT] " Liron Himi
2023-03-20 9:20 ` [PATCH v2 23/44] net/mvpp2: " Chengwen Feng
2023-03-20 9:33 ` [EXT] " Liron Himi
2023-03-20 9:20 ` [PATCH v2 24/44] net/netvsc: " Chengwen Feng
2023-03-21 22:20 ` Long Li
2023-07-04 3:02 ` Stephen Hemminger
2023-03-20 9:20 ` [PATCH v2 25/44] net/octeontx: " Chengwen Feng
2023-04-12 7:54 ` [EXT] " Harman Kalra
2023-03-20 9:20 ` [PATCH v2 26/44] net/pfe: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 27/44] net/qede: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 28/44] baseband/la12xx: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 29/44] bus/pci: fix segment fault when parse args Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 30/44] common/mlx5: fix segment fault when parse devargs Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 31/44] crypto/cnxk: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 32/44] crypto/dpaa_sec: " Chengwen Feng
2023-03-20 9:20 ` [PATCH v2 33/44] crypto/dpaa2_sec: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 34/44] crypto/mvsam: " Chengwen Feng
2023-03-20 9:33 ` [EXT] " Liron Himi
2023-03-20 9:21 ` [PATCH v2 35/44] crypto/scheduler: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 36/44] dma/dpaa2: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 37/44] event/cnxk: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 38/44] event/dlb2: " Chengwen Feng
2023-03-24 16:26 ` Sevincer, Abdullah
2023-03-20 9:21 ` [PATCH v2 39/44] event/dpaa: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 40/44] event/octeontx: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 41/44] event/opdl: " Chengwen Feng
2023-03-23 13:48 ` Liang Ma
2023-03-20 9:21 ` [PATCH v2 42/44] event/sw: " Chengwen Feng
2023-03-20 11:58 ` Van Haaren, Harry
2023-03-20 9:21 ` [PATCH v2 43/44] mempool/cnxk: " Chengwen Feng
2023-03-20 9:21 ` [PATCH v2 44/44] raw/cnxk_gpio: " Chengwen Feng
2023-04-15 1:38 ` [PATCH v2 00/44] fix segment fault when parse args fengchengwen
2023-04-17 16:37 ` Ferruh Yigit [this message]
2023-10-31 20:46 ` Stephen Hemminger
2023-10-31 20:58 ` [RFC] kvargs: don't pass parse handler a NULL pointer Stephen Hemminger
2023-11-01 1:16 ` fengchengwen
2023-11-03 7:38 ` [PATCH v3 0/5] fix segment fault when parse args Chengwen Feng
2023-11-03 7:38 ` [PATCH v3 1/5] kvargs: add one new process API Chengwen Feng
2023-11-03 13:09 ` Ferruh Yigit
2023-11-05 5:55 ` fengchengwen
2023-11-03 7:38 ` [PATCH v3 2/5] net/af_packet: use new API to parse kvargs Chengwen Feng
2023-11-03 13:11 ` Ferruh Yigit
2023-11-05 5:56 ` fengchengwen
2023-11-03 7:38 ` [PATCH v3 3/5] net/sfc: " Chengwen Feng
2023-11-03 13:23 ` Ferruh Yigit
2023-11-03 7:38 ` [PATCH v3 4/5] net/tap: " Chengwen Feng
2023-11-03 13:34 ` Ferruh Yigit
2023-11-05 5:57 ` fengchengwen
2023-11-03 7:38 ` [PATCH v3 5/5] net/mvneta: fix possible out-of-bounds write Chengwen Feng
2023-11-03 13:39 ` Ferruh Yigit
2023-11-03 13:41 ` [PATCH v3 0/5] fix segment fault when parse args Ferruh Yigit
2023-11-05 5:50 ` fengchengwen
2023-11-05 5:45 ` [PATCH v4 " Chengwen Feng
2023-11-05 5:45 ` [PATCH v4 1/5] kvargs: add one new process API Chengwen Feng
2023-11-06 3:18 ` Stephen Hemminger
2023-11-06 7:13 ` fengchengwen
2023-11-06 16:19 ` Stephen Hemminger
2023-11-07 3:21 ` fengchengwen
2023-11-05 5:45 ` [PATCH v4 2/5] net/sfc: use new API to parse kvargs Chengwen Feng
2023-11-05 5:45 ` [PATCH v4 3/5] net/tap: " Chengwen Feng
2023-11-05 5:45 ` [PATCH v4 4/5] common/nfp: " Chengwen Feng
2023-11-06 3:19 ` Stephen Hemminger
2023-11-06 7:22 ` fengchengwen
2023-11-05 5:45 ` [PATCH v4 5/5] net/mvneta: fix possible out-of-bounds write Chengwen Feng
2023-11-06 7:31 ` [PATCH v5 0/5] fix segment fault when parse args Chengwen Feng
2023-11-06 7:31 ` [PATCH v5 1/5] kvargs: add one new process API Chengwen Feng
2023-11-06 7:31 ` [PATCH v5 2/5] net/sfc: use new API to parse kvargs Chengwen Feng
2023-11-06 10:28 ` Andrew Rybchenko
2023-11-06 12:29 ` fengchengwen
2023-11-07 7:18 ` Andrew Rybchenko
2023-11-06 7:31 ` [PATCH v5 3/5] net/tap: " Chengwen Feng
2023-11-06 7:31 ` [PATCH v5 4/5] common/nfp: " Chengwen Feng
2023-11-06 7:31 ` [PATCH v5 5/5] net/mvneta: fix possible out-of-bounds write Chengwen Feng
2023-11-21 2:11 ` lihuisong (C)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c49ce5b-4109-8621-ee9e-4a9f575280ee@amd.com \
--to=ferruh.yigit@amd.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).