[PATCH net 0/3] mlx4 fix for shutdown flow

[PATCH net 0/3] mlx4 fix for shutdown flow

[Tariq Toukan]

This patchset fixes an invalid reference to mdev in mlx4 shutdown flow.

In patch 1, we make sure netif_device_detach() is called from shutdown flow only,
since we want to keep it present during a simple configuration change.

In patches 2 and 3, we add checks that were missing in:
* dev_get_phys_port_id
* dev_get_phys_port_name
We check the presence of the network device before calling the driver's
callbacks. This already exists for all other ndo's.

Series generated against net commit:
e5f6f564fd19 bnxt: add a missing rcu synchronization

Thanks,
Tariq.

Eugenia Emantayev (3):
  net/mlx4_en: Remove netif_device_detach from stop port flow
  net: Check netdevice presence on dev_get_phys_port_id
  net: Check netdevice presence on dev_get_phys_port_name

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  8 ++++----
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  | 22 ++++++++--------------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  2 +-
 net/core/dev.c                                  |  4 ++++
 net/core/rtnetlink.c                            |  4 ++--
 5 files changed, 19 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH net 1/3] net/mlx4_en: Remove netif_device_detach from stop port flow

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

netif_device_detach() should be called from shutdown flow only,
in any other scenario netif_device_detach is not needed and may
result in -ENODEV error in certain cases.
In order to prevent TX timeout issue during heavy CPU load
netif_carrier_off will be called.

Fixes: 3484aac16149 ("net/mlx4_en: Fix transmit timeout when driver restarts port")
Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  8 ++++----
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  | 22 ++++++++--------------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  2 +-
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index bdda17d2ea0f..3bb30f66aa07 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -932,7 +932,7 @@ static __be32 speed_set_ptys_admin(struct mlx4_en_priv *priv, u32 speed,
 	mutex_lock(&priv->mdev->state_lock);
 	if (priv->port_up) {
 		en_warn(priv, "Port link mode changed, restarting port...\n");
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 		if (mlx4_en_start_port(dev))
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
@@ -1077,7 +1077,7 @@ static int mlx4_en_set_ringparam(struct net_device *dev,
 
 	if (priv->port_up) {
 		port_up = 1;
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 	}
 
 	mlx4_en_safe_replace_resources(priv, tmp);
@@ -1205,7 +1205,7 @@ static int mlx4_en_set_rxfh(struct net_device *dev, const u32 *ring_index,
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		port_up = 1;
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 	}
 
 	if (ring_index)
@@ -1751,7 +1751,7 @@ static int mlx4_en_set_channels(struct net_device *dev,
 
 	if (priv->port_up) {
 		port_up = 1;
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 	}
 
 	mlx4_en_safe_replace_resources(priv, tmp);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 3a47e83d3e07..01a680b66177 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1741,8 +1741,7 @@ int mlx4_en_start_port(struct net_device *dev)
 		napi_schedule(&priv->rx_cq[i]->napi);
 
 	netif_tx_start_all_queues(dev);
-	netif_device_attach(dev);
-
+	netif_carrier_on(dev);
 	return 0;
 
 tx_err:
@@ -1767,7 +1766,7 @@ int mlx4_en_start_port(struct net_device *dev)
 }
 
 
-void mlx4_en_stop_port(struct net_device *dev, int detach)
+void mlx4_en_stop_port(struct net_device *dev)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_en_dev *mdev = priv->mdev;
@@ -1785,12 +1784,7 @@ void mlx4_en_stop_port(struct net_device *dev, int detach)
 	mlx4_CLOSE_PORT(mdev->dev, priv->port);
 
 	/* Synchronize with tx routine */
-	netif_tx_lock_bh(dev);
-	if (detach)
-		netif_device_detach(dev);
-	netif_tx_stop_all_queues(dev);
-	netif_tx_unlock_bh(dev);
-
+	netif_carrier_off(dev);
 	netif_tx_disable(dev);
 
 	/* Set port as not active */
@@ -1903,7 +1897,7 @@ static void mlx4_en_restart(struct work_struct *work)
 	rtnl_lock();
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 		if (mlx4_en_start_port(dev))
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
@@ -1987,7 +1981,7 @@ static int mlx4_en_close(struct net_device *dev)
 
 	mutex_lock(&mdev->state_lock);
 
-	mlx4_en_stop_port(dev, 0);
+	mlx4_en_stop_port(dev);
 	netif_carrier_off(dev);
 
 	mutex_unlock(&mdev->state_lock);
@@ -2231,7 +2225,7 @@ static int mlx4_en_change_mtu(struct net_device *dev, int new_mtu)
 			 * the port */
 			en_dbg(DRV, priv, "Change MTU called with card down!?\n");
 		} else {
-			mlx4_en_stop_port(dev, 1);
+			mlx4_en_stop_port(dev);
 			err = mlx4_en_start_port(dev);
 			if (err) {
 				en_err(priv, "Failed restarting port:%d\n",
@@ -2687,7 +2681,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		port_up = 1;
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 	}
 
 	priv->xdp_ring_num = xdp_ring_num;
@@ -3406,7 +3400,7 @@ int mlx4_en_reset_config(struct net_device *dev,
 
 	if (priv->port_up) {
 		port_up = 1;
-		mlx4_en_stop_port(dev, 1);
+		mlx4_en_stop_port(dev);
 	}
 
 	en_warn(priv, "Changing device configuration rx filter(%x) rx vlan(%x)\n",
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index a3528dd1e72e..fb17acdfe528 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -659,7 +659,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 			struct mlx4_en_port_profile *prof);
 
 int mlx4_en_start_port(struct net_device *dev);
-void mlx4_en_stop_port(struct net_device *dev, int detach);
+void mlx4_en_stop_port(struct net_device *dev);
 
 void mlx4_en_set_stats_bitmap(struct mlx4_dev *dev,
 			      struct mlx4_en_stats_bitmap *stats_bitmap,
-- 
1.8.3.1

[PATCH net 2/3] net: Check netdevice presence on dev_get_phys_port_id

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

Check presence of network device before calling device driver
ndo_get_phys_port_id callback. Otherwise callback may access
non-existing data structures and cause kernel panic.

Fixes: 66b52b0dc82c ("net: add ndo to get id of physical port of the device")
Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Reported-by: Steve Wise <swise@opengridcomputing.com>
Cc: Jiri Pirko <jiri@mellanox.com>
---
 net/core/dev.c       | 2 ++
 net/core/rtnetlink.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6666b28b6815..7de0d000a9f8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6602,6 +6602,8 @@ int dev_get_phys_port_id(struct net_device *dev,
 
 	if (!ops->ndo_get_phys_port_id)
 		return -EOPNOTSUPP;
+	if (!netif_device_present(dev))
+		return -ENODEV;
 	return ops->ndo_get_phys_port_id(dev, ppid);
 }
 EXPORT_SYMBOL(dev_get_phys_port_id);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a6529c55ffb7..b3dd81f82e70 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1036,7 +1036,7 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
 
 	err = dev_get_phys_port_id(dev, &ppid);
 	if (err) {
-		if (err == -EOPNOTSUPP)
+		if (err == -EOPNOTSUPP || err == -ENODEV)
 			return 0;
 		return err;
 	}
-- 
1.8.3.1

[PATCH net 3/3] net: Check netdevice presence on dev_get_phys_port_name

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

Check presence of network device before calling device driver
ndo_get_phys_port_name callback. Otherwise callback may access
non-existing data structures and cause kernel panic.

Fixes: db24a9044ee1 ('net: add support for phys_port_name')
Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Reported-by: Steve Wise <swise@opengridcomputing.com>
Cc: Jiri Pirko <jiri@mellanox.com>
---
 net/core/dev.c       | 2 ++
 net/core/rtnetlink.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7de0d000a9f8..b21d77405863 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6623,6 +6623,8 @@ int dev_get_phys_port_name(struct net_device *dev,
 
 	if (!ops->ndo_get_phys_port_name)
 		return -EOPNOTSUPP;
+	if (!netif_device_present(dev))
+		return -ENODEV;
 	return ops->ndo_get_phys_port_name(dev, name, len);
 }
 EXPORT_SYMBOL(dev_get_phys_port_name);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b3dd81f82e70..40cd41376566 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1054,7 +1054,7 @@ static int rtnl_phys_port_name_fill(struct sk_buff *skb, struct net_device *dev)
 
 	err = dev_get_phys_port_name(dev, name, sizeof(name));
 	if (err) {
-		if (err == -EOPNOTSUPP)
+		if (err == -EOPNOTSUPP || err == -ENODEV)
 			return 0;
 		return err;
 	}
-- 
1.8.3.1

Re: [PATCH net 0/3] mlx4 fix for shutdown flow

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Thu, 17 Nov 2016 17:40:48 +0200

> This patchset fixes an invalid reference to mdev in mlx4 shutdown flow.
> 
> In patch 1, we make sure netif_device_detach() is called from shutdown flow only,
> since we want to keep it present during a simple configuration change.
> 
> In patches 2 and 3, we add checks that were missing in:
> * dev_get_phys_port_id
> * dev_get_phys_port_name
> We check the presence of the network device before calling the driver's
> callbacks. This already exists for all other ndo's.
> 
> Series generated against net commit:
> e5f6f564fd19 bnxt: add a missing rcu synchronization

I don't like where this is going nor the precedence it is setting.

If you are taking the device into a state where it cannot be safely
accessed by ndo operations, then you _MUST_ do whatever is necessary
to make sure the device is unregistered and cannot be found in the
various global lists and tables of network devices.

This is mandatory.

And this is how we must fix these kinds of problems instead of
peppering device presence test all over the place.  That will be
error prone and in the long term a huge maintainence burdon.

I'm not applying this series, sorry.  You have to fix this properly.