[dpdk-dev] [PATCH] eal devargs: don't call rte_log when not initialized

[dpdk-dev] [PATCH] eal devargs: don't call rte_log when not initialized

[Stephen Hemminger]

This problem was discovered when passing invalid PCI id to the
blacklist API in devargs.

Any failures in rte_devargs_add would cause a core dump because
it would call rte_log() before the the EAL log environment was
initailized.  Rather than try and log just remove the messages
and leave it up to the caller to check the return value.

Most of the other failure possibilities are when malloc() fails, and if
that happens any logging that used malloc() would also fail.

This failure was not caught by the standalone tests to devargs
because the tests are run after calling rte_eal_init (which is not
how devargs is intended to be used).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 615945e..ec56165 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -31,11 +31,15 @@
  */
 
 /* This file manages the list of devices and their arguments, as given
- * by the user at startup */
+ * by the user at startup
+ *
+ * Code here should not call rte_log since the EAL environment
+ * may not be initialized.
+ */
 
+#include <stdio.h>
 #include <string.h>
 
-#include <rte_log.h>
 #include <rte_pci.h>
 #include <rte_devargs.h>
 #include "eal_private.h"
@@ -54,11 +58,8 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 		return -1;
 
 	*drvname = strdup(devargs_str);
-	if (drvname == NULL) {
-		RTE_LOG(ERR, EAL,
-			"cannot allocate temp memory for driver name\n");
+	if (drvname == NULL)
 		return -1;
-	}
 
 	/* set the first ',' to '\0' to split name and arguments */
 	sep = strchr(*drvname, ',');
@@ -70,8 +71,6 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 	}
 
 	if (*drvargs == NULL) {
-		RTE_LOG(ERR, EAL,
-			"cannot allocate temp memory for driver arguments\n");
 		free(*drvname);
 		return -1;
 	}
@@ -88,10 +87,9 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 
 	/* use malloc instead of rte_malloc as it's called early at init */
 	devargs = malloc(sizeof(*devargs));
-	if (devargs == NULL) {
-		RTE_LOG(ERR, EAL, "cannot allocate devargs\n");
+	if (devargs == NULL)
 		goto fail;
-	}
+
 	memset(devargs, 0, sizeof(*devargs));
 	devargs->type = devtype;
 
@@ -103,19 +101,17 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	case RTE_DEVTYPE_BLACKLISTED_PCI:
 		/* try to parse pci identifier */
 		if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 &&
-		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
-			RTE_LOG(ERR, EAL, "invalid PCI identifier <%s>\n", buf);
+		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0)
 			goto fail;
-		}
+
 		break;
 	case RTE_DEVTYPE_VIRTUAL:
 		/* save driver name */
 		ret = snprintf(devargs->virtual.drv_name,
 			       sizeof(devargs->virtual.drv_name), "%s", buf);
-		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name)) {
-			RTE_LOG(ERR, EAL, "driver name too large: <%s>\n", buf);
+		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name))
 			goto fail;
-		}
+
 		break;
 	}
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH] eal devargs: don't call rte_log when not initialized

[Olivier MATZ]

Hi Stephen,

On 05/15/2015 06:37 PM, Stephen Hemminger wrote:
> This problem was discovered when passing invalid PCI id to the
> blacklist API in devargs.
> 
> Any failures in rte_devargs_add would cause a core dump because
> it would call rte_log() before the the EAL log environment was
> initailized.  Rather than try and log just remove the messages
> and leave it up to the caller to check the return value.
> 
> Most of the other failure possibilities are when malloc() fails, and if
> that happens any logging that used malloc() would also fail.
> 
> This failure was not caught by the standalone tests to devargs
> because the tests are run after calling rte_eal_init (which is not
> how devargs is intended to be used).
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks,
Olivier



> ---
>  lib/librte_eal/common/eal_common_devargs.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 615945e..ec56165 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -31,11 +31,15 @@
>   */
>  
>  /* This file manages the list of devices and their arguments, as given
> - * by the user at startup */
> + * by the user at startup
> + *
> + * Code here should not call rte_log since the EAL environment
> + * may not be initialized.
> + */
>  
> +#include <stdio.h>
>  #include <string.h>
>  
> -#include <rte_log.h>
>  #include <rte_pci.h>
>  #include <rte_devargs.h>
>  #include "eal_private.h"
> @@ -54,11 +58,8 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>  		return -1;
>  
>  	*drvname = strdup(devargs_str);
> -	if (drvname == NULL) {
> -		RTE_LOG(ERR, EAL,
> -			"cannot allocate temp memory for driver name\n");
> +	if (drvname == NULL)
>  		return -1;
> -	}
>  
>  	/* set the first ',' to '\0' to split name and arguments */
>  	sep = strchr(*drvname, ',');
> @@ -70,8 +71,6 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>  	}
>  
>  	if (*drvargs == NULL) {
> -		RTE_LOG(ERR, EAL,
> -			"cannot allocate temp memory for driver arguments\n");
>  		free(*drvname);
>  		return -1;
>  	}
> @@ -88,10 +87,9 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  
>  	/* use malloc instead of rte_malloc as it's called early at init */
>  	devargs = malloc(sizeof(*devargs));
> -	if (devargs == NULL) {
> -		RTE_LOG(ERR, EAL, "cannot allocate devargs\n");
> +	if (devargs == NULL)
>  		goto fail;
> -	}
> +
>  	memset(devargs, 0, sizeof(*devargs));
>  	devargs->type = devtype;
>  
> @@ -103,19 +101,17 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  	case RTE_DEVTYPE_BLACKLISTED_PCI:
>  		/* try to parse pci identifier */
>  		if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 &&
> -		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
> -			RTE_LOG(ERR, EAL, "invalid PCI identifier <%s>\n", buf);
> +		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0)
>  			goto fail;
> -		}
> +
>  		break;
>  	case RTE_DEVTYPE_VIRTUAL:
>  		/* save driver name */
>  		ret = snprintf(devargs->virtual.drv_name,
>  			       sizeof(devargs->virtual.drv_name), "%s", buf);
> -		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name)) {
> -			RTE_LOG(ERR, EAL, "driver name too large: <%s>\n", buf);
> +		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name))
>  			goto fail;
> -		}
> +
>  		break;
>  	}
>  
> 

Re: [dpdk-dev] [PATCH] eal devargs: don't call rte_log when not initialized

[Thomas Monjalon]

> > This problem was discovered when passing invalid PCI id to the
> > blacklist API in devargs.
> > 
> > Any failures in rte_devargs_add would cause a core dump because
> > it would call rte_log() before the the EAL log environment was
> > initailized.  Rather than try and log just remove the messages
> > and leave it up to the caller to check the return value.
> > 
> > Most of the other failure possibilities are when malloc() fails, and if
> > that happens any logging that used malloc() would also fail.
> > 
> > This failure was not caught by the standalone tests to devargs
> > because the tests are run after calling rte_eal_init (which is not
> > how devargs is intended to be used).
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks