| Message ID | 20230314124813.39521-1-fengchengwen@huawei.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4EB9A41E28; Tue, 14 Mar 2023 13:54:54 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 287E740F18; Tue, 14 Mar 2023 13:54:54 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id F2AFA40A7E for <dev@dpdk.org>; Tue, 14 Mar 2023 13:54:51 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.56]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4PbYLb6KVRzSl0C; Tue, 14 Mar 2023 20:51:35 +0800 (CST) Received: from localhost.localdomain (10.50.163.32) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Tue, 14 Mar 2023 20:54:49 +0800 From: Chengwen Feng <fengchengwen@huawei.com> To: <thomas@monjalon.net>, <ferruh.yigit@amd.com> CC: <dev@dpdk.org> Subject: [PATCH 0/5] fix segment fault when parse args Date: Tue, 14 Mar 2023 12:48:08 +0000 Message-ID: <20230314124813.39521-1-fengchengwen@huawei.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.50.163.32] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
| Series |
fix segment fault when parse args
|
|
Message
fengchengwen
March 14, 2023, 12:48 p.m. UTC
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 (5): app/pdump: 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 app/pdump/main.c | 12 ++++++++++++ drivers/net/memif/rte_eth_memif.c | 30 ++++++++++++++++++++++++++++++ drivers/net/pcap/pcap_ethdev.c | 13 +++++++++++-- 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 ++++++ 7 files changed, 71 insertions(+), 2 deletions(-)
Comments
On 3/14/2023 12:48 PM, 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 (5): > app/pdump: 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 Hi Chengwen, Did you scan all `rte_kvargs_process()` instances? And if there would be a way to tell kvargs that a value is expected (or not) this checks could be done in kvargs layer, I think this also can be to look at.
On 2023/3/17 2:18, Ferruh Yigit wrote: > On 3/14/2023 12:48 PM, 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 (5): >> app/pdump: 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 > > Hi Chengwen, > > Did you scan all `rte_kvargs_process()` instances? No, I was just looking at the modules I was concerned about. I looked at it briefly, and some modules had the same problem. > > > And if there would be a way to tell kvargs that a value is expected (or > not) this checks could be done in kvargs layer, I think this also can be > to look at. Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. But it also break the API's behavior. Or continue fix the exist code (about 10+ place more), for new invoking, because the 'arg_handler_t' already well documented (52ab17efdecf935792ee1d0cb749c0dbd536c083), they'll take the initiative to prevent this. Hope for more advise for the next. > . >
On 3/17/2023 2:43 AM, fengchengwen wrote: > On 2023/3/17 2:18, Ferruh Yigit wrote: >> On 3/14/2023 12:48 PM, 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 (5): >>> app/pdump: 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 >> >> Hi Chengwen, >> >> Did you scan all `rte_kvargs_process()` instances? > > No, I was just looking at the modules I was concerned about. > I looked at it briefly, and some modules had the same problem. > >> >> >> And if there would be a way to tell kvargs that a value is expected (or >> not) this checks could be done in kvargs layer, I think this also can be >> to look at. > > Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. > I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. > But it also break the API's behavior. > What about having a new API, like `rte_kvargs_process_extended()`, That gets an additional flag as parameter, which may have values like following to indicate if key expects a value or not: ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' ARG_WITH_VALUE --> "key=value" ARG_NO_VALUE --> 'key' Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as `rte_kvargs_process()`. This way instead of adding checks, relevant usage can be replaced by `rte_kvargs_process_extended()`, this requires similar amount of change but code will be more clean I think. Do you think does this work? > > Or continue fix the exist code (about 10+ place more), > for new invoking, because the 'arg_handler_t' already well documented (52ab17efdecf935792ee1d0cb749c0dbd536c083), > they'll take the initiative to prevent this. > > > Hope for more advise for the next. > >> . >>
On 2023/3/21 21:50, Ferruh Yigit wrote: > On 3/17/2023 2:43 AM, fengchengwen wrote: >> On 2023/3/17 2:18, Ferruh Yigit wrote: >>> On 3/14/2023 12:48 PM, 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 (5): >>>> app/pdump: 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 >>> >>> Hi Chengwen, >>> >>> Did you scan all `rte_kvargs_process()` instances? >> >> No, I was just looking at the modules I was concerned about. >> I looked at it briefly, and some modules had the same problem. >> >>> >>> >>> And if there would be a way to tell kvargs that a value is expected (or >>> not) this checks could be done in kvargs layer, I think this also can be >>> to look at. >> >> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. >> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. >> But it also break the API's behavior. >> > > What about having a new API, like `rte_kvargs_process_extended()`, > > That gets an additional flag as parameter, which may have values like > following to indicate if key expects a value or not: > ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' > ARG_WITH_VALUE --> "key=value" > ARG_NO_VALUE --> 'key' > > Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as > `rte_kvargs_process()`. > > This way instead of adding checks, relevant usage can be replaced by > `rte_kvargs_process_extended()`, this requires similar amount of change > but code will be more clean I think. > > Do you think does this work? Yes, it can work. But I think the introduction of new API adds some complexity. And a good API definition could more simpler. > > >> >> Or continue fix the exist code (about 10+ place more), >> for new invoking, because the 'arg_handler_t' already well documented (52ab17efdecf935792ee1d0cb749c0dbd536c083), >> they'll take the initiative to prevent this. >> >> >> Hope for more advise for the next. >> >>> . >>> > > . >
On 3/22/2023 1:15 AM, fengchengwen wrote: > On 2023/3/21 21:50, Ferruh Yigit wrote: >> On 3/17/2023 2:43 AM, fengchengwen wrote: >>> On 2023/3/17 2:18, Ferruh Yigit wrote: >>>> On 3/14/2023 12:48 PM, 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 (5): >>>>> app/pdump: 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 >>>> >>>> Hi Chengwen, >>>> >>>> Did you scan all `rte_kvargs_process()` instances? >>> >>> No, I was just looking at the modules I was concerned about. >>> I looked at it briefly, and some modules had the same problem. >>> >>>> >>>> >>>> And if there would be a way to tell kvargs that a value is expected (or >>>> not) this checks could be done in kvargs layer, I think this also can be >>>> to look at. >>> >>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. >>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. >>> But it also break the API's behavior. >>> >> >> What about having a new API, like `rte_kvargs_process_extended()`, >> >> That gets an additional flag as parameter, which may have values like >> following to indicate if key expects a value or not: >> ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' >> ARG_WITH_VALUE --> "key=value" >> ARG_NO_VALUE --> 'key' >> >> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as >> `rte_kvargs_process()`. >> >> This way instead of adding checks, relevant usage can be replaced by >> `rte_kvargs_process_extended()`, this requires similar amount of change >> but code will be more clean I think. >> >> Do you think does this work? > > Yes, it can work. > > But I think the introduction of new API adds some complexity. > And a good API definition could more simpler. > Other option is changing existing API, but that may be widely used and changing it impacts applications, I don't think it worth. Of course we can live with as it is and add checks to the callback functions, although I still believe a new 'process()' API is better idea. >> >> >>> >>> Or continue fix the exist code (about 10+ place more), >>> for new invoking, because the 'arg_handler_t' already well documented (52ab17efdecf935792ee1d0cb749c0dbd536c083), >>> they'll take the initiative to prevent this. >>> >>> >>> Hope for more advise for the next. >>> >>>> . >>>> >> >> . >>
22/03/2023 09:53, Ferruh Yigit: > On 3/22/2023 1:15 AM, fengchengwen wrote: > > On 2023/3/21 21:50, Ferruh Yigit wrote: > >> On 3/17/2023 2:43 AM, fengchengwen wrote: > >>> On 2023/3/17 2:18, Ferruh Yigit wrote: > >>>> On 3/14/2023 12:48 PM, 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 (5): > >>>>> app/pdump: 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 > >>>> > >>>> Hi Chengwen, > >>>> > >>>> Did you scan all `rte_kvargs_process()` instances? > >>> > >>> No, I was just looking at the modules I was concerned about. > >>> I looked at it briefly, and some modules had the same problem. > >>> > >>>> > >>>> > >>>> And if there would be a way to tell kvargs that a value is expected (or > >>>> not) this checks could be done in kvargs layer, I think this also can be > >>>> to look at. > >>> > >>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. > >>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. > >>> But it also break the API's behavior. > >>> > >> > >> What about having a new API, like `rte_kvargs_process_extended()`, > >> > >> That gets an additional flag as parameter, which may have values like > >> following to indicate if key expects a value or not: > >> ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' > >> ARG_WITH_VALUE --> "key=value" > >> ARG_NO_VALUE --> 'key' > >> > >> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as > >> `rte_kvargs_process()`. > >> > >> This way instead of adding checks, relevant usage can be replaced by > >> `rte_kvargs_process_extended()`, this requires similar amount of change > >> but code will be more clean I think. > >> > >> Do you think does this work? > > > > Yes, it can work. > > > > But I think the introduction of new API adds some complexity. > > And a good API definition could more simpler. > > > > Other option is changing existing API, but that may be widely used and > changing it impacts applications, I don't think it worth. I've planned a change in kvargs API 5 years ago and never did it: From doc/guides/rel_notes/deprecation.rst: " * kvargs: The function ``rte_kvargs_process`` will get a new parameter for returning key match count. It will ease handling of no-match case. " > Of course we can live with as it is and add checks to the callback > functions, although I still believe a new 'process()' API is better idea.
On 2023/3/22 21:49, Thomas Monjalon wrote: > 22/03/2023 09:53, Ferruh Yigit: >> On 3/22/2023 1:15 AM, fengchengwen wrote: >>> On 2023/3/21 21:50, Ferruh Yigit wrote: >>>> On 3/17/2023 2:43 AM, fengchengwen wrote: >>>>> On 2023/3/17 2:18, Ferruh Yigit wrote: >>>>>> On 3/14/2023 12:48 PM, 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 (5): >>>>>>> app/pdump: 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 >>>>>> >>>>>> Hi Chengwen, >>>>>> >>>>>> Did you scan all `rte_kvargs_process()` instances? >>>>> >>>>> No, I was just looking at the modules I was concerned about. >>>>> I looked at it briefly, and some modules had the same problem. >>>>> >>>>>> >>>>>> >>>>>> And if there would be a way to tell kvargs that a value is expected (or >>>>>> not) this checks could be done in kvargs layer, I think this also can be >>>>>> to look at. >>>>> >>>>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. >>>>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. >>>>> But it also break the API's behavior. >>>>> >>>> >>>> What about having a new API, like `rte_kvargs_process_extended()`, >>>> >>>> That gets an additional flag as parameter, which may have values like >>>> following to indicate if key expects a value or not: >>>> ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' >>>> ARG_WITH_VALUE --> "key=value" >>>> ARG_NO_VALUE --> 'key' >>>> >>>> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as >>>> `rte_kvargs_process()`. >>>> >>>> This way instead of adding checks, relevant usage can be replaced by >>>> `rte_kvargs_process_extended()`, this requires similar amount of change >>>> but code will be more clean I think. >>>> >>>> Do you think does this work? >>> >>> Yes, it can work. >>> >>> But I think the introduction of new API adds some complexity. >>> And a good API definition could more simpler. >>> >> >> Other option is changing existing API, but that may be widely used and >> changing it impacts applications, I don't think it worth. > > I've planned a change in kvargs API 5 years ago and never did it: >>From doc/guides/rel_notes/deprecation.rst: > " > * kvargs: The function ``rte_kvargs_process`` will get a new parameter > for returning key match count. It will ease handling of no-match case. > " I think it's okay to add extra parameter for rte_kvargs_process. But it will break ABI. Also I notice patchset was deferred in patchwork. Does it mean that the new version can't accept until the 23.11 release cycle ? > >> Of course we can live with as it is and add checks to the callback >> functions, although I still believe a new 'process()' API is better idea. > > > > . >
23/03/2023 12:58, fengchengwen: > On 2023/3/22 21:49, Thomas Monjalon wrote: > > 22/03/2023 09:53, Ferruh Yigit: > >> On 3/22/2023 1:15 AM, fengchengwen wrote: > >>> On 2023/3/21 21:50, Ferruh Yigit wrote: > >>>> On 3/17/2023 2:43 AM, fengchengwen wrote: > >>>>> On 2023/3/17 2:18, Ferruh Yigit wrote: > >>>>>> On 3/14/2023 12:48 PM, 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 (5): > >>>>>>> app/pdump: 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 > >>>>>> > >>>>>> Hi Chengwen, > >>>>>> > >>>>>> Did you scan all `rte_kvargs_process()` instances? > >>>>> > >>>>> No, I was just looking at the modules I was concerned about. > >>>>> I looked at it briefly, and some modules had the same problem. > >>>>> > >>>>>> > >>>>>> > >>>>>> And if there would be a way to tell kvargs that a value is expected (or > >>>>>> not) this checks could be done in kvargs layer, I think this also can be > >>>>>> to look at. > >>>>> > >>>>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. > >>>>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. > >>>>> But it also break the API's behavior. > >>>>> > >>>> > >>>> What about having a new API, like `rte_kvargs_process_extended()`, > >>>> > >>>> That gets an additional flag as parameter, which may have values like > >>>> following to indicate if key expects a value or not: > >>>> ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' > >>>> ARG_WITH_VALUE --> "key=value" > >>>> ARG_NO_VALUE --> 'key' > >>>> > >>>> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as > >>>> `rte_kvargs_process()`. > >>>> > >>>> This way instead of adding checks, relevant usage can be replaced by > >>>> `rte_kvargs_process_extended()`, this requires similar amount of change > >>>> but code will be more clean I think. > >>>> > >>>> Do you think does this work? > >>> > >>> Yes, it can work. > >>> > >>> But I think the introduction of new API adds some complexity. > >>> And a good API definition could more simpler. > >>> > >> > >> Other option is changing existing API, but that may be widely used and > >> changing it impacts applications, I don't think it worth. > > > > I've planned a change in kvargs API 5 years ago and never did it: > >>From doc/guides/rel_notes/deprecation.rst: > > " > > * kvargs: The function ``rte_kvargs_process`` will get a new parameter > > for returning key match count. It will ease handling of no-match case. > > " > > I think it's okay to add extra parameter for rte_kvargs_process. But it will > break ABI. > Also I notice patchset was deferred in patchwork. > > Does it mean that the new version can't accept until the 23.11 release cycle ? It is a bit too late to take a decision in 23.03 cycle. Let's continue this discussion. We can either have some fixes in 23.07 or have an ABI breaking change in 23.11.