[dpdk-stable] [PATCH] net/mlx5: fix verification of device context

[dpdk-stable] [PATCH] net/mlx5: fix verification of device context

[Shachar Beiser]

Get interface name function lacks verification of device context.
It might lead to segmentation fault when trying to query the name
after the device is closed.fixing it by adding the missing verification

Fixes: cd89f22a1e9770 ("net/mlx5: remove unused interface name query")
Cc: stable@dpdk.org

Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
---
 drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index b70b7b9..6e67461 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -173,6 +173,10 @@ struct priv *
 	char match[IF_NAMESIZE] = "";
 
 	{
+		if (priv->ctx == NULL) {
+			DEBUG("The device is closed, cannot query interface name ");
+			return -1;
+		}
 		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
 
 		dir = opendir(path);
-- 
1.8.3.1

Re: [dpdk-stable] [PATCH] net/mlx5: fix verification of device context

[Adrien Mazarguil]

Hi Shachar,

On Wed, Jul 26, 2017 at 05:43:24AM +0000, Shachar Beiser wrote:
> Get interface name function lacks verification of device context.
> It might lead to segmentation fault when trying to query the name
> after the device is closed.fixing it by adding the missing verification
> 

Thanks, however if by "close" you mean it may occur when applications use
ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs
have to protect themselves against bad application behavior, otherwise there
is no end to such fixes.

The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any
other ethdev callback (see documentation), but a bus probe operation.

Perhaps I've missed something, so in case a crash occurs *while* calling
rte_eth_dev_close() I guess this patch is fine, but then please describe
the reason.

> Fixes: cd89f22a1e9770 ("net/mlx5: remove unused interface name query")

This commit doesn't look like the root cause of that issue?

> Cc: stable@dpdk.org
> 
> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index b70b7b9..6e67461 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -173,6 +173,10 @@ struct priv *
>  	char match[IF_NAMESIZE] = "";
>  
>  	{
> +		if (priv->ctx == NULL) {
> +			DEBUG("The device is closed, cannot query interface name ");
> +			return -1;
> +		}

MKSTR() is at the beginning of this block because it defines a new variable
(path). For coding style consistency you should not put any code before
variable declarations, or at least insert an empty line between
them. Otherwise you could move this check to the parent block.

>  		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
>  
>  		dir = opendir(path);
> -- 
> 1.8.3.1
> 

I think this patch is not necessary unless proved otherwise, have you
actually observed a crash addressed by it?

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-stable] [PATCH] net/mlx5: fix verification of device context

[Shachar Beiser]

Hi ,

       When I say close I mean : " mlx5_dev_close" . This function set the priv->ctx to NULL.
       We think this patch is required because we have an open bug of seg fault while accessing priv->ctx == NULL.

                         -Shachar Beiser. 
         

-----Original Message-----
From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] 
Sent: Wednesday, July 26, 2017 12:06 PM
To: Shachar Beiser <shacharbe@mellanox.com>
Cc: dev@dpdk.org; Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; stable@dpdk.org
Subject: Re: [PATCH] net/mlx5: fix verification of device context

Hi Shachar,

On Wed, Jul 26, 2017 at 05:43:24AM +0000, Shachar Beiser wrote:
> Get interface name function lacks verification of device context.
> It might lead to segmentation fault when trying to query the name 
> after the device is closed.fixing it by adding the missing 
> verification
> 

Thanks, however if by "close" you mean it may occur when applications use ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs have to protect themselves against bad application behavior, otherwise there is no end to such fixes.

The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any other ethdev callback (see documentation), but a bus probe operation.

Perhaps I've missed something, so in case a crash occurs *while* calling
rte_eth_dev_close() I guess this patch is fine, but then please describe the reason.

> Fixes: cd89f22a1e9770 ("net/mlx5: remove unused interface name query")

This commit doesn't look like the root cause of that issue?

> Cc: stable@dpdk.org
> 
> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_ethdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c 
> b/drivers/net/mlx5/mlx5_ethdev.c index b70b7b9..6e67461 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -173,6 +173,10 @@ struct priv *
>  	char match[IF_NAMESIZE] = "";
>  
>  	{
> +		if (priv->ctx == NULL) {
> +			DEBUG("The device is closed, cannot query interface name ");
> +			return -1;
> +		}

MKSTR() is at the beginning of this block because it defines a new variable (path). For coding style consistency you should not put any code before variable declarations, or at least insert an empty line between them. Otherwise you could move this check to the parent block.

>  		MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path);
>  
>  		dir = opendir(path);
> --
> 1.8.3.1
> 

I think this patch is not necessary unless proved otherwise, have you
actually observed a crash addressed by it?

-- 
Adrien Mazarguil
6WIND

[dpdk-stable] [PATCH 1/6] ethdev: fix device state on detach

[Gaetan Rivet]

The device state should be handled by the ether layer when possible.
Applications should not have to do it.

Not setting the state to UNUSED will make the port_id of the device
valid for all ether API functions, usually resulting in segfault.

Fixes: 284c908cc588 ("app/testpmd: request device removal interrupt")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 app/test-pmd/testpmd.c        | 1 -
 lib/librte_ether/rte_ethdev.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e754d12..9142218 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1836,7 +1836,6 @@ rmv_event_callback(void *arg)
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
 	rte_eal_dev_detach(dev->device);
-	dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 /* This function is used by the interrupt thread */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index d4ebb1b..8c365ed 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -453,6 +453,7 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (ret < 0)
 		goto err;
 
+	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
 	return 0;
 
 err:
-- 
2.1.4

[dpdk-stable] [PATCH v2 1/6] ethdev: fix device state on detach

[Gaetan Rivet]

The device state should be handled by the ether layer when possible.
Applications should not have to do it.

Not setting the state to UNUSED will make the port_id of the device
valid for all ether API functions, usually resulting in segfault.

Fixes: 284c908cc588 ("app/testpmd: request device removal interrupt")
Cc: stable@dpdk.org

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 app/test-pmd/testpmd.c        | 1 -
 lib/librte_ether/rte_ethdev.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e754d12..9142218 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1836,7 +1836,6 @@ rmv_event_callback(void *arg)
 	close_port(port_id);
 	printf("removing device %s\n", dev->device->name);
 	rte_eal_dev_detach(dev->device);
-	dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 /* This function is used by the interrupt thread */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index d4ebb1b..8c365ed 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -453,6 +453,7 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
 	if (ret < 0)
 		goto err;
 
+	rte_eth_devices[port_id].state = RTE_ETH_DEV_UNUSED;
 	return 0;
 
 err:
-- 
2.1.4