[dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[Mohammad Abdul Awal]

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

Re: [dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[Thomas Monjalon]

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"

Re: [dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[Ferruh Yigit]

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"
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] ethdev: fix null pointer checking

[Bruce Richardson]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] ethdev: fix null pointer checking

[Ferruh Yigit]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] ethdev: fix null pointer checking

[David Marchand]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] ethdev: fix null pointer checking

[Mohammad Abdul Awal]

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.

Re: [dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[Awal, Mohammad Abdul]

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"
> >
> >
> >

Re: [dpdk-stable] [PATCH 1/3] ethdev: fix null pointer checking

[Thomas Monjalon]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] ethdev: fix null pointer checking

[Stephen Hemminger]

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.