* [PATCH 1/3] ethdev: fix null pointer checking
@ 2019-04-03 16:07 Mohammad Abdul Awal
2019-04-03 16:27 ` Thomas Monjalon
2019-04-03 18:50 ` Stephen Hemminger
0 siblings, 2 replies; 10+ messages in thread
From: Mohammad Abdul Awal @ 2019-04-03 16:07 UTC (permalink / raw)
To: dev; +Cc: thomas, ferruh.yigit, arybchenko, Mohammad Abdul Awal, stable
Null value for parameter name will cause segfault for the
strnlen and strcmp functions.
Fixes: 0b33b68d12 ("ethdev: export allocate function")
Fixes: 942661004c ("ethdev: export secondary attach function")
Cc: stable@dpdk.org
Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
---
lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 10bdfb37e..26898ed08 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
struct rte_eth_dev *eth_dev = NULL;
size_t name_len;
+ if (name == NULL) {
+ RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
+ return NULL;
+ }
+
name_len = strnlen(name, RTE_ETH_NAME_MAX_LEN);
if (name_len == 0) {
RTE_ETHDEV_LOG(ERR, "Zero length Ethernet device name\n");
@@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char *name)
uint16_t i;
struct rte_eth_dev *eth_dev = NULL;
+ if (name == NULL) {
+ RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
+ return NULL;
+ }
+
rte_eth_dev_shared_data_prepare();
/* Synchronize port attachment to primary port creation and release. */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:07 [PATCH 1/3] ethdev: fix null pointer checking Mohammad Abdul Awal
@ 2019-04-03 16:27 ` Thomas Monjalon
2019-04-03 16:35 ` Ferruh Yigit
2019-04-03 18:50 ` Stephen Hemminger
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2019-04-03 16:27 UTC (permalink / raw)
To: Mohammad Abdul Awal; +Cc: dev, ferruh.yigit, arybchenko, stable
03/04/2019 18:07, Mohammad Abdul Awal:
> Null value for parameter name will cause segfault for the
> strnlen and strcmp functions.
I'm not sure we want such obvious checks for all APIs.
Here I would say yes.
> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> Fixes: 942661004c ("ethdev: export secondary attach function")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> ---
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
> struct rte_eth_dev *eth_dev = NULL;
> size_t name_len;
>
> + if (name == NULL) {
> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
This is a very generic error message.
It might be "Fail to allocate port with empty name"
> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char *name)
> uint16_t i;
> struct rte_eth_dev *eth_dev = NULL;
>
> + if (name == NULL) {
> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
"Fail to attach port with empty name"
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:27 ` Thomas Monjalon
@ 2019-04-03 16:35 ` Ferruh Yigit
2019-04-03 16:41 ` Bruce Richardson
2019-04-03 17:30 ` Awal, Mohammad Abdul
0 siblings, 2 replies; 10+ messages in thread
From: Ferruh Yigit @ 2019-04-03 16:35 UTC (permalink / raw)
To: Thomas Monjalon, Mohammad Abdul Awal; +Cc: dev, arybchenko, stable
On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> 03/04/2019 18:07, Mohammad Abdul Awal:
>> Null value for parameter name will cause segfault for the
>> strnlen and strcmp functions.
>
> I'm not sure we want such obvious checks for all APIs.
> Here I would say yes.
These are internal functions, not APIs.
I am for verifying input for (all) APIs but not for internal functions, drivers
should call them and they are in our control, if they are passing NULL we can
fix them :)
>
>> Fixes: 0b33b68d12 ("ethdev: export allocate function")
>> Fixes: 942661004c ("ethdev: export secondary attach function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
>> ---
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
>> struct rte_eth_dev *eth_dev = NULL;
>> size_t name_len;
>>
>> + if (name == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>
> This is a very generic error message.
> It might be "Fail to allocate port with empty name"
>
>> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char *name)
>> uint16_t i;
>> struct rte_eth_dev *eth_dev = NULL;
>>
>> + if (name == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>
> "Fail to attach port with empty name"
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:35 ` Ferruh Yigit
@ 2019-04-03 16:41 ` Bruce Richardson
2019-04-03 16:52 ` Ferruh Yigit
2019-04-03 17:30 ` Awal, Mohammad Abdul
1 sibling, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2019-04-03 16:41 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Thomas Monjalon, Mohammad Abdul Awal, dev, arybchenko, stable
On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > 03/04/2019 18:07, Mohammad Abdul Awal:
> >> Null value for parameter name will cause segfault for the strnlen and
> >> strcmp functions.
> >
> > I'm not sure we want such obvious checks for all APIs. Here I would
> > say yes.
>
> These are internal functions, not APIs. I am for verifying input for
> (all) APIs but not for internal functions, drivers should call them and
> they are in our control, if they are passing NULL we can fix them :)
>
True, but if these are control path or init time code paths rather than
data path APIs, I don't see the harm in putting in the checks.
/Bruce
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:41 ` Bruce Richardson
@ 2019-04-03 16:52 ` Ferruh Yigit
2019-04-03 17:32 ` David Marchand
0 siblings, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2019-04-03 16:52 UTC (permalink / raw)
To: Bruce Richardson
Cc: Thomas Monjalon, Mohammad Abdul Awal, dev, arybchenko, stable
On 4/3/2019 5:41 PM, Bruce Richardson wrote:
> On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
>> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
>>> 03/04/2019 18:07, Mohammad Abdul Awal:
>>>> Null value for parameter name will cause segfault for the strnlen and
>>>> strcmp functions.
>>>
>>> I'm not sure we want such obvious checks for all APIs. Here I would
>>> say yes.
>>
>> These are internal functions, not APIs. I am for verifying input for
>> (all) APIs but not for internal functions, drivers should call them and
>> they are in our control, if they are passing NULL we can fix them :)
>>
> True, but if these are control path or init time code paths rather than
> data path APIs, I don't see the harm in putting in the checks.
No harm from performance point of view, agree, but also looks unnecessary to me.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:35 ` Ferruh Yigit
2019-04-03 16:41 ` Bruce Richardson
@ 2019-04-03 17:30 ` Awal, Mohammad Abdul
2019-04-03 18:42 ` Thomas Monjalon
1 sibling, 1 reply; 10+ messages in thread
From: Awal, Mohammad Abdul @ 2019-04-03 17:30 UTC (permalink / raw)
To: Yigit, Ferruh, Thomas Monjalon; +Cc: dev, arybchenko, stable
The null checks are for the input param "char *name" of APIs rte_eth_dev_allocate and rte_eth_dev_attach_secondary.
I will change the err msg to suggested one.
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, April 3, 2019 5:35 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Awal, Mohammad Abdul
> <mohammad.abdul.awal@intel.com>
> Cc: dev@dpdk.org; arybchenko@solarflare.com; stable@dpdk.org
> Subject: Re: [PATCH 1/3] ethdev: fix null pointer checking
>
> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > 03/04/2019 18:07, Mohammad Abdul Awal:
> >> Null value for parameter name will cause segfault for the
> >> strnlen and strcmp functions.
> >
> > I'm not sure we want such obvious checks for all APIs.
> > Here I would say yes.
>
> These are internal functions, not APIs.
> I am for verifying input for (all) APIs but not for internal functions, drivers
> should call them and they are in our control, if they are passing NULL we can
> fix them :)
>
> >
> >> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> >> Fixes: 942661004c ("ethdev: export secondary attach function")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Mohammad Abdul Awal
> <mohammad.abdul.awal@intel.com>
> >> ---
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
> >> struct rte_eth_dev *eth_dev = NULL;
> >> size_t name_len;
> >>
> >> + if (name == NULL) {
> >> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> >
> > This is a very generic error message.
> > It might be "Fail to allocate port with empty name"
> >
> >> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char
> *name)
> >> uint16_t i;
> >> struct rte_eth_dev *eth_dev = NULL;
> >>
> >> + if (name == NULL) {
> >> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> >
> > "Fail to attach port with empty name"
> >
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:52 ` Ferruh Yigit
@ 2019-04-03 17:32 ` David Marchand
2019-04-04 8:33 ` Mohammad Abdul Awal
0 siblings, 1 reply; 10+ messages in thread
From: David Marchand @ 2019-04-03 17:32 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Bruce Richardson, Thomas Monjalon, Mohammad Abdul Awal, dev,
Andrew Rybchenko, dpdk stable
On Wed, Apr 3, 2019 at 6:53 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 4/3/2019 5:41 PM, Bruce Richardson wrote:
> > On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
> >> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> >>> 03/04/2019 18:07, Mohammad Abdul Awal:
> >>>> Null value for parameter name will cause segfault for the strnlen and
> >>>> strcmp functions.
> >>>
> >>> I'm not sure we want such obvious checks for all APIs. Here I would
> >>> say yes.
> >>
> >> These are internal functions, not APIs. I am for verifying input for
> >> (all) APIs but not for internal functions, drivers should call them and
> >> they are in our control, if they are passing NULL we can fix them :)
> >>
> > True, but if these are control path or init time code paths rather than
> > data path APIs, I don't see the harm in putting in the checks.
>
> No harm from performance point of view, agree, but also looks unnecessary
> to me.
>
+1
All the more when you see the following patches that adds input checks in
the faulty/too naive drivers.
--
David Marchand
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 17:30 ` Awal, Mohammad Abdul
@ 2019-04-03 18:42 ` Thomas Monjalon
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2019-04-03 18:42 UTC (permalink / raw)
To: Awal, Mohammad Abdul; +Cc: Yigit, Ferruh, dev, arybchenko, stable
03/04/2019 19:30, Awal, Mohammad Abdul:
> From: Yigit, Ferruh
> > On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > > 03/04/2019 18:07, Mohammad Abdul Awal:
> > >> Null value for parameter name will cause segfault for the
> > >> strnlen and strcmp functions.
> > >
> > > I'm not sure we want such obvious checks for all APIs.
> > > Here I would say yes.
> >
> > These are internal functions, not APIs.
> > I am for verifying input for (all) APIs but not for internal functions, drivers
> > should call them and they are in our control, if they are passing NULL we can
> > fix them :)
>
> The null checks are for the input param "char *name" of APIs rte_eth_dev_allocate and rte_eth_dev_attach_secondary.
> I will change the err msg to suggested one.
As Ferruh said, these are not really API in the sense that they
are not called by the applications but only by drivers.
PS: please write replies below original text.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 16:07 [PATCH 1/3] ethdev: fix null pointer checking Mohammad Abdul Awal
2019-04-03 16:27 ` Thomas Monjalon
@ 2019-04-03 18:50 ` Stephen Hemminger
1 sibling, 0 replies; 10+ messages in thread
From: Stephen Hemminger @ 2019-04-03 18:50 UTC (permalink / raw)
To: Mohammad Abdul Awal; +Cc: dev, thomas, ferruh.yigit, arybchenko, stable
On Wed, 3 Apr 2019 17:07:26 +0100
Mohammad Abdul Awal <mohammad.abdul.awal@intel.com> wrote:
> Null value for parameter name will cause segfault for the
> strnlen and strcmp functions.
>
> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> Fixes: 942661004c ("ethdev: export secondary attach function")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 10bdfb37e..26898ed08 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
> struct rte_eth_dev *eth_dev = NULL;
> size_t name_len;
>
> + if (name == NULL) {
> + RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> + return NULL;
> + }
> +
Ok, but I doubt that a driver that is so buggy that it passes NULL
that it will do any proper error handling on return of NULL.
Would rather just see the application crash hard immediately.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ethdev: fix null pointer checking
2019-04-03 17:32 ` David Marchand
@ 2019-04-04 8:33 ` Mohammad Abdul Awal
0 siblings, 0 replies; 10+ messages in thread
From: Mohammad Abdul Awal @ 2019-04-04 8:33 UTC (permalink / raw)
To: David Marchand, Ferruh Yigit
Cc: Bruce Richardson, Thomas Monjalon, dev, Andrew Rybchenko, dpdk stable
On 03/04/2019 18:32, David Marchand wrote:
> On Wed, Apr 3, 2019 at 6:53 PM Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
>
> On 4/3/2019 5:41 PM, Bruce Richardson wrote:
> > On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
> >> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> >>> 03/04/2019 18:07, Mohammad Abdul Awal:
> >>>> Null value for parameter name will cause segfault for the
> strnlen and
> >>>> strcmp functions.
> >>>
> >>> I'm not sure we want such obvious checks for all APIs. Here I
> would
> >>> say yes.
> >>
> >> These are internal functions, not APIs. I am for verifying
> input for
> >> (all) APIs but not for internal functions, drivers should call
> them and
> >> they are in our control, if they are passing NULL we can fix
> them :)
> >>
> > True, but if these are control path or init time code paths
> rather than
> > data path APIs, I don't see the harm in putting in the checks.
>
> No harm from performance point of view, agree, but also looks
> unnecessary to me.
>
>
> +1
> All the more when you see the following patches that adds input checks
> in the faulty/too naive drivers.
>
>
> --
> David Marchand
Self-NACK to the patch considering the discussion above.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-04 8:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-03 16:07 [PATCH 1/3] ethdev: fix null pointer checking Mohammad Abdul Awal
2019-04-03 16:27 ` Thomas Monjalon
2019-04-03 16:35 ` Ferruh Yigit
2019-04-03 16:41 ` Bruce Richardson
2019-04-03 16:52 ` Ferruh Yigit
2019-04-03 17:32 ` David Marchand
2019-04-04 8:33 ` Mohammad Abdul Awal
2019-04-03 17:30 ` Awal, Mohammad Abdul
2019-04-03 18:42 ` Thomas Monjalon
2019-04-03 18:50 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.