[PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[Hannes Frederic Sowa]

This patchset removes the dependency of network drivers on vxlan or
geneve, so those don't get autoloaded when the nic driver is loaded.

Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
are not called without rtnl lock.

Hannes Frederic Sowa (7):
  benet: be_resume needs to protect be_open with rtnl_lock
  fm10k: protect fm10k_open in fm10k_io_resume with rtnl_lock
  mlx4: protect mlx4_en_start_port in mlx4_en_restart with rtnl_lock
  ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock
  qlcnic: protect qlicnic_attach_func with rtnl_lock
  vxlan: break dependency with netdev drivers
  geneve: break dependency with netdev drivers

 drivers/net/ethernet/emulex/benet/be_main.c      | 10 +++++---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     |  2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  2 ++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |  2 ++
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 10 ++++++--
 drivers/net/geneve.c                             | 31 +++++++++++++++++++++---
 drivers/net/vxlan.c                              | 14 +++++++----
 include/linux/netdevice.h                        |  2 ++
 include/net/geneve.h                             |  6 ++---
 include/net/vxlan.h                              |  6 ++---
 10 files changed, 63 insertions(+), 22 deletions(-)

-- 
2.5.5

[PATCH net-next 1/7] benet: be_resume needs to protect be_open with rtnl_lock

[Hannes Frederic Sowa]

be_open calls down to functions which expects rtnl lock to be held.

Cc: Sathya Perla <sathya.perla@broadcom.com>
Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>
Cc: Padmanabh Ratnakar <padmanabh.ratnakar@broadcom.com>
Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Cc: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/emulex/benet/be_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 536686476369bf..ed98ef1ecac38d 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4890,11 +4890,13 @@ static int be_resume(struct be_adapter *adapter)
 	if (status)
 		return status;
 
-	if (netif_running(netdev)) {
+	rtnl_lock();
+	if (netif_running(netdev))
 		status = be_open(netdev);
-		if (status)
-			return status;
-	}
+	rtnl_unlock();
+
+	if (status)
+		return status;
 
 	netif_device_attach(netdev);
 
-- 
2.5.5

[PATCH net-next 2/7] fm10k: protect fm10k_open in fm10k_io_resume with rtnl_lock

[Hannes Frederic Sowa]

fm10k_open requires rtnl_lock to be held.

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Shannon Nelson <shannon.nelson@intel.com>
Cc: Carolyn Wyborny <carolyn.wyborny@intel.com>
Cc: Don Skidmore <donald.c.skidmore@intel.com>
Cc: Bruce Allan <bruce.w.allan@intel.com>
Cc: John Ronciak <john.ronciak@intel.com>
Cc: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index f0992950e228eb..04304d9a633935 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2362,8 +2362,10 @@ static void fm10k_io_resume(struct pci_dev *pdev)
 	/* reset clock */
 	fm10k_ts_reset(interface);
 
+	rtnl_lock();
 	if (netif_running(netdev))
 		err = fm10k_open(netdev);
+	rtnl_unlock();
 
 	/* final check of hardware state before registering the interface */
 	err = err ? : fm10k_hw_ready(interface);
-- 
2.5.5

[PATCH net-next 3/7] mlx4: protect mlx4_en_start_port in mlx4_en_restart with rtnl_lock

[Hannes Frederic Sowa]

mlx4_en_start_port requires rtnl_lock to be held.

Cc: Eugenia Emantayev <eugenia@mellanox.com>
Cc: Yishai Hadas <yishaih@mellanox.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index b4b258c8ca47d4..8bd143dda95d11 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1856,6 +1856,7 @@ static void mlx4_en_restart(struct work_struct *work)
 
 	en_dbg(DRV, priv, "Watchdog task called for port %d\n", priv->port);
 
+	rtnl_lock();
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		mlx4_en_stop_port(dev, 1);
@@ -1863,6 +1864,7 @@ static void mlx4_en_restart(struct work_struct *work)
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
 	mutex_unlock(&mdev->state_lock);
+	rtnl_unlock();
 }
 
 static void mlx4_en_clear_stats(struct net_device *dev)
-- 
2.5.5

[PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock

[Hannes Frederic Sowa]

vxlan_get_rx_port requires rtnl_lock to be held.

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Shannon Nelson <shannon.nelson@intel.com>
Cc: Carolyn Wyborny <carolyn.wyborny@intel.com>
Cc: Don Skidmore <donald.c.skidmore@intel.com>
Cc: Bruce Allan <bruce.w.allan@intel.com>
Cc: John Ronciak <john.ronciak@intel.com>
Cc: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 2976df77bf14f5..b2f2cf40f06a87 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work)
 		return;
 	}
 #ifdef CONFIG_IXGBE_VXLAN
+	rtnl_lock();
 	if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
 		adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
 		vxlan_get_rx_port(adapter->netdev);
 	}
+	rtnl_unlock();
 #endif /* CONFIG_IXGBE_VXLAN */
 	ixgbe_reset_subtask(adapter);
 	ixgbe_phy_interrupt_subtask(adapter);
-- 
2.5.5

[PATCH net-next 5/7] qlcnic: protect qlicnic_attach_func with rtnl_lock

[Hannes Frederic Sowa]

qlcnic_attach_func requires rtnl_lock to be held.

Cc: Dept-GELinuxNICDev@qlogic.com
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1205f6f9c94173..1c29105b6c364f 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -3952,8 +3952,14 @@ static pci_ers_result_t qlcnic_82xx_io_error_detected(struct pci_dev *pdev,
 
 static pci_ers_result_t qlcnic_82xx_io_slot_reset(struct pci_dev *pdev)
 {
-	return qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
-				PCI_ERS_RESULT_RECOVERED;
+	pci_ers_result_t res;
+
+	rtnl_lock();
+	res = qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
+					 PCI_ERS_RESULT_RECOVERED;
+	rtnl_unlock();
+
+	return res;
 }
 
 static void qlcnic_82xx_io_resume(struct pci_dev *pdev)
-- 
2.5.5

[PATCH net-next 6/7] vxlan: break dependency with netdev drivers

[Hannes Frederic Sowa]

Currently all drivers depend and autoload the vxlan module because how
vxlan_get_rx_port is linked into them. Remove this dependency:

By using a new event type in the netdevice notifier call chain we proxy
the request from the drivers to flush and resetup the vxlan ports not
directly via function call but by the already existing netdevice
notifier call chain.

I added a separate new event type, NETDEV_OFFLOAD_PUSH_VXLAN, to do so.
We don't need to save those ids, as the event type field is an unsigned
long and using specialized event types for this purpose seemed to be a
more elegant way. This also comes in beneficial if in future we want to
add offloading knobs for vxlan.

Cc: Jesse Gross <jesse@kernel.org>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/vxlan.c       | 14 +++++++++-----
 include/linux/netdevice.h |  1 +
 include/net/vxlan.h       |  6 ++----
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index c2e22c2532a1a8..6fb93b57a72489 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2527,7 +2527,7 @@ static struct device_type vxlan_type = {
  * supply the listening VXLAN udp ports. Callers are expected
  * to implement the ndo_add_vxlan_port.
  */
-void vxlan_get_rx_port(struct net_device *dev)
+static void vxlan_push_rx_ports(struct net_device *dev)
 {
 	struct vxlan_sock *vs;
 	struct net *net = dev_net(dev);
@@ -2536,6 +2536,9 @@ void vxlan_get_rx_port(struct net_device *dev)
 	__be16 port;
 	unsigned int i;
 
+	if (!dev->netdev_ops->ndo_add_vxlan_port)
+		return;
+
 	spin_lock(&vn->sock_lock);
 	for (i = 0; i < PORT_HASH_SIZE; ++i) {
 		hlist_for_each_entry_rcu(vs, &vn->sock_list[i], hlist) {
@@ -2547,7 +2550,6 @@ void vxlan_get_rx_port(struct net_device *dev)
 	}
 	spin_unlock(&vn->sock_lock);
 }
-EXPORT_SYMBOL_GPL(vxlan_get_rx_port);
 
 /* Initialize the device structure. */
 static void vxlan_setup(struct net_device *dev)
@@ -3283,20 +3285,22 @@ static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
 	unregister_netdevice_many(&list_kill);
 }
 
-static int vxlan_lowerdev_event(struct notifier_block *unused,
-				unsigned long event, void *ptr)
+static int vxlan_netdevice_event(struct notifier_block *unused,
+				 unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
 
 	if (event == NETDEV_UNREGISTER)
 		vxlan_handle_lowerdev_unregister(vn, dev);
+	else if (event == NETDEV_OFFLOAD_PUSH_VXLAN)
+		vxlan_push_rx_ports(dev);
 
 	return NOTIFY_DONE;
 }
 
 static struct notifier_block vxlan_notifier_block __read_mostly = {
-	.notifier_call = vxlan_lowerdev_event,
+	.notifier_call = vxlan_netdevice_event,
 };
 
 static __net_init int vxlan_init_net(struct net *net)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a3bb534576a383..d4c8cd424f8dbc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2244,6 +2244,7 @@ struct netdev_lag_lower_state_info {
 #define NETDEV_BONDING_INFO	0x0019
 #define NETDEV_PRECHANGEUPPER	0x001A
 #define NETDEV_CHANGELOWERSTATE	0x001B
+#define NETDEV_OFFLOAD_PUSH_VXLAN	0x001C
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index d442eb3129cde4..673e9f9e6da768 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -390,13 +390,11 @@ static inline __be32 vxlan_compute_rco(unsigned int start, unsigned int offset)
 	return vni_field;
 }
 
-#if IS_ENABLED(CONFIG_VXLAN)
-void vxlan_get_rx_port(struct net_device *netdev);
-#else
 static inline void vxlan_get_rx_port(struct net_device *netdev)
 {
+	ASSERT_RTNL();
+	call_netdevice_notifiers(NETDEV_OFFLOAD_PUSH_VXLAN, netdev);
 }
-#endif
 
 static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
 {
-- 
2.5.5

[PATCH net-next 7/7] geneve: break dependency with netdev drivers

[Hannes Frederic Sowa]

Equivalent to "vxlan: break dependency with netdev drivers", don't
autoload geneve module in case the driver is loaded. Instead make the
coupling weaker by using netdevice notifiers as proxy.

Cc: Jesse Gross <jesse@kernel.org>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/geneve.c      | 31 ++++++++++++++++++++++++++++---
 include/linux/netdevice.h |  1 +
 include/net/geneve.h      |  6 ++----
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index efbc7ceedc3a13..59314cf47584e4 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1172,7 +1172,7 @@ static struct device_type geneve_type = {
  * supply the listening GENEVE udp ports. Callers are expected
  * to implement the ndo_add_geneve_port.
  */
-void geneve_get_rx_port(struct net_device *dev)
+static void geneve_push_rx_ports(struct net_device *dev)
 {
 	struct net *net = dev_net(dev);
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1181,6 +1181,9 @@ void geneve_get_rx_port(struct net_device *dev)
 	struct sock *sk;
 	__be16 port;
 
+	if (!dev->netdev_ops->ndo_add_geneve_port)
+		return;
+
 	rcu_read_lock();
 	list_for_each_entry_rcu(gs, &gn->sock_list, list) {
 		sk = gs->sock->sk;
@@ -1190,7 +1193,6 @@ void geneve_get_rx_port(struct net_device *dev)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL_GPL(geneve_get_rx_port);
 
 /* Initialize the device structure. */
 static void geneve_setup(struct net_device *dev)
@@ -1538,6 +1540,21 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 }
 EXPORT_SYMBOL_GPL(geneve_dev_create_fb);
 
+static int geneve_netdevice_event(struct notifier_block *unused,
+				  unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+	if (event == NETDEV_OFFLOAD_PUSH_GENEVE)
+		geneve_push_rx_ports(dev);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block geneve_notifier_block __read_mostly = {
+	.notifier_call = geneve_netdevice_event,
+};
+
 static __net_init int geneve_init_net(struct net *net)
 {
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1590,11 +1607,18 @@ static int __init geneve_init_module(void)
 	if (rc)
 		goto out1;
 
-	rc = rtnl_link_register(&geneve_link_ops);
+	rc = register_netdevice_notifier(&geneve_notifier_block);
 	if (rc)
 		goto out2;
 
+	rc = rtnl_link_register(&geneve_link_ops);
+	if (rc)
+		goto out3;
+
 	return 0;
+
+out3:
+	unregister_netdevice_notifier(&geneve_notifier_block);
 out2:
 	unregister_pernet_subsys(&geneve_net_ops);
 out1:
@@ -1605,6 +1629,7 @@ late_initcall(geneve_init_module);
 static void __exit geneve_cleanup_module(void)
 {
 	rtnl_link_unregister(&geneve_link_ops);
+	unregister_netdevice_notifier(&geneve_notifier_block);
 	unregister_pernet_subsys(&geneve_net_ops);
 }
 module_exit(geneve_cleanup_module);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d4c8cd424f8dbc..1f6d5db471a24f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2245,6 +2245,7 @@ struct netdev_lag_lower_state_info {
 #define NETDEV_PRECHANGEUPPER	0x001A
 #define NETDEV_CHANGELOWERSTATE	0x001B
 #define NETDEV_OFFLOAD_PUSH_VXLAN	0x001C
+#define NETDEV_OFFLOAD_PUSH_GENEVE	0x001D
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/include/net/geneve.h b/include/net/geneve.h
index e6c23dc765f7ec..cb544a530146c2 100644
--- a/include/net/geneve.h
+++ b/include/net/geneve.h
@@ -62,13 +62,11 @@ struct genevehdr {
 	struct geneve_opt options[];
 };
 
-#if IS_ENABLED(CONFIG_GENEVE)
-void geneve_get_rx_port(struct net_device *netdev);
-#else
 static inline void geneve_get_rx_port(struct net_device *netdev)
 {
+	ASSERT_RTNL();
+	call_netdevice_notifiers(NETDEV_OFFLOAD_PUSH_GENEVE, netdev);
 }
-#endif
 
 #ifdef CONFIG_INET
 struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
-- 
2.5.5

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Mon, 18 Apr 2016 21:19:41 +0200

> This patchset removes the dependency of network drivers on vxlan or
> geneve, so those don't get autoloaded when the nic driver is loaded.
> 
> Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
> are not called without rtnl lock.

In net-next, the 'qed' driver has tunneling offload support too.
Don't you need to update this series to handle that driver as
well?

Thanks.

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[Hannes Frederic Sowa]

On Wed, Apr 20, 2016, at 02:27, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Mon, 18 Apr 2016 21:19:41 +0200
> 
> > This patchset removes the dependency of network drivers on vxlan or
> > geneve, so those don't get autoloaded when the nic driver is loaded.
> > 
> > Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
> > are not called without rtnl lock.
> 
> In net-next, the 'qed' driver has tunneling offload support too.
> Don't you need to update this series to handle that driver as
> well?

I audited qede as well:

qede calls {vxlan,geneve}_get_rx_port only from ndo_open which isn't
reused anywhere else in the driver, only from ndo_open, which holds
rtnl_lock also. Thus the driver is safe and doesn't need a change.

Bye,
Hannes

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Wed, 20 Apr 2016 03:06:13 +0200

> On Wed, Apr 20, 2016, at 02:27, David Miller wrote:
>> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Date: Mon, 18 Apr 2016 21:19:41 +0200
>> 
>> > This patchset removes the dependency of network drivers on vxlan or
>> > geneve, so those don't get autoloaded when the nic driver is loaded.
>> > 
>> > Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
>> > are not called without rtnl lock.
>> 
>> In net-next, the 'qed' driver has tunneling offload support too.
>> Don't you need to update this series to handle that driver as
>> well?
> 
> I audited qede as well:
> 
> qede calls {vxlan,geneve}_get_rx_port only from ndo_open which isn't
> reused anywhere else in the driver, only from ndo_open, which holds
> rtnl_lock also. Thus the driver is safe and doesn't need a change.

I'm talking about your final patches which elide the dependencies.

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[Hannes Frederic Sowa]

On 20.04.2016 03:11, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Wed, 20 Apr 2016 03:06:13 +0200
> 
>> On Wed, Apr 20, 2016, at 02:27, David Miller wrote:
>>> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>> Date: Mon, 18 Apr 2016 21:19:41 +0200
>>>
>>>> This patchset removes the dependency of network drivers on vxlan or
>>>> geneve, so those don't get autoloaded when the nic driver is loaded.
>>>>
>>>> Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
>>>> are not called without rtnl lock.
>>>
>>> In net-next, the 'qed' driver has tunneling offload support too.
>>> Don't you need to update this series to handle that driver as
>>> well?
>>
>> I audited qede as well:
>>
>> qede calls {vxlan,geneve}_get_rx_port only from ndo_open which isn't
>> reused anywhere else in the driver, only from ndo_open, which holds
>> rtnl_lock also. Thus the driver is safe and doesn't need a change.
> 
> I'm talking about your final patches which elide the dependencies.

The dependency will be removed with only the last two patches for all
drivers. The static inline functions {vxlan,geneve}_get_rx_port now
simply expand to a call to call_netdevice_notifiers, which is provided
by the core kernel and not by the vxlan or geneve module anymore.

During testing I saw that very often some drivers actually reused their
*_attach or *_open function from other callbacks, e.g. pci ones. While
they often didn't care about rtnl_lock, we now require that for the new
get_rx_port functions, thus I added them.

I hadn't had to modify all drivers with vxlan offloading support, e.g.
also mlx5 and the broadcom drivers, bnx2x and bnxt, were already fine.

Bye,
Hannes

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[Hannes Frederic Sowa]

Hi David,

On Wed, Apr 20, 2016, at 03:11, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Wed, 20 Apr 2016 03:06:13 +0200
> 
> > On Wed, Apr 20, 2016, at 02:27, David Miller wrote:
> >> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >> Date: Mon, 18 Apr 2016 21:19:41 +0200
> >> 
> >> > This patchset removes the dependency of network drivers on vxlan or
> >> > geneve, so those don't get autoloaded when the nic driver is loaded.
> >> > 
> >> > Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
> >> > are not called without rtnl lock.
> >> 
> >> In net-next, the 'qed' driver has tunneling offload support too.
> >> Don't you need to update this series to handle that driver as
> >> well?
> > 
> > I audited qede as well:
> > 
> > qede calls {vxlan,geneve}_get_rx_port only from ndo_open which isn't
> > reused anywhere else in the driver, only from ndo_open, which holds
> > rtnl_lock also. Thus the driver is safe and doesn't need a change.
> 
> I'm talking about your final patches which elide the dependencies.

could you look at this again? If you have further feedback I am happy to
incooperate those.

Thank you,
Hannes

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 21 Apr 2016 09:43:11 +0200

> could you look at this again? If you have further feedback I am happy to
> incooperate those.

I will.

Re: [PATCH net-next 0/7] net: network drivers should not depend on geneve/vxlan

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 21 Apr 2016 09:43:11 +0200

> Hi David,
> 
> On Wed, Apr 20, 2016, at 03:11, David Miller wrote:
>> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Date: Wed, 20 Apr 2016 03:06:13 +0200
>> 
>> > On Wed, Apr 20, 2016, at 02:27, David Miller wrote:
>> >> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> >> Date: Mon, 18 Apr 2016 21:19:41 +0200
>> >> 
>> >> > This patchset removes the dependency of network drivers on vxlan or
>> >> > geneve, so those don't get autoloaded when the nic driver is loaded.
>> >> > 
>> >> > Also audited the code such that vxlan_get_rx_port and geneve_get_rx_port
>> >> > are not called without rtnl lock.
>> >> 
>> >> In net-next, the 'qed' driver has tunneling offload support too.
>> >> Don't you need to update this series to handle that driver as
>> >> well?
>> > 
>> > I audited qede as well:
>> > 
>> > qede calls {vxlan,geneve}_get_rx_port only from ndo_open which isn't
>> > reused anywhere else in the driver, only from ndo_open, which holds
>> > rtnl_lock also. Thus the driver is safe and doesn't need a change.
>> 
>> I'm talking about your final patches which elide the dependencies.
> 
> could you look at this again? If you have further feedback I am happy to
> incooperate those.

Series applied, thanks Hannes.

Re: [PATCH net-next 3/7] mlx4: protect mlx4_en_start_port in mlx4_en_restart with rtnl_lock

[Eric Dumazet]

On Mon, 2016-04-18 at 21:19 +0200, Hannes Frederic Sowa wrote:
> mlx4_en_start_port requires rtnl_lock to be held.
> 
> Cc: Eugenia Emantayev <eugenia@mellanox.com>
> Cc: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index b4b258c8ca47d4..8bd143dda95d11 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -1856,6 +1856,7 @@ static void mlx4_en_restart(struct work_struct *work)
>  
>  	en_dbg(DRV, priv, "Watchdog task called for port %d\n", priv->port);
>  
> +	rtnl_lock();
>  	mutex_lock(&mdev->state_lock);
>  	if (priv->port_up) {
>  		mlx4_en_stop_port(dev, 1);
> @@ -1863,6 +1864,7 @@ static void mlx4_en_restart(struct work_struct *work)
>  			en_err(priv, "Failed restarting port %d\n", priv->port);
>  	}
>  	mutex_unlock(&mdev->state_lock);
> +	rtnl_unlock();
>  }
>  
>  static void mlx4_en_clear_stats(struct net_device *dev)

It looks that this work queue is not canceled at device dismantle.

I suspect a use after free is possible.

Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock

[Eric Dumazet]

On Mon, 2016-04-18 at 21:19 +0200, Hannes Frederic Sowa wrote:
> vxlan_get_rx_port requires rtnl_lock to be held.
> 
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Shannon Nelson <shannon.nelson@intel.com>
> Cc: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Cc: Don Skidmore <donald.c.skidmore@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> Cc: John Ronciak <john.ronciak@intel.com>
> Cc: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 2976df77bf14f5..b2f2cf40f06a87 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work)
>  		return;
>  	}
>  #ifdef CONFIG_IXGBE_VXLAN
> +	rtnl_lock();
>  	if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
>  		adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
>  		vxlan_get_rx_port(adapter->netdev);
>  	}
> +	rtnl_unlock();
>  #endif /* CONFIG_IXGBE_VXLAN */
>  	ixgbe_reset_subtask(adapter);
>  	ixgbe_phy_interrupt_subtask(adapter);

Although there might be a deadlock with a concurrent
cancel_work_sync(&adapter->service_task);  from ixgbe_remove()

Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock

[Alexander Duyck]

On Fri, Jun 10, 2016 at 2:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2016-04-18 at 21:19 +0200, Hannes Frederic Sowa wrote:
>> vxlan_get_rx_port requires rtnl_lock to be held.
>>
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Cc: Shannon Nelson <shannon.nelson@intel.com>
>> Cc: Carolyn Wyborny <carolyn.wyborny@intel.com>
>> Cc: Don Skidmore <donald.c.skidmore@intel.com>
>> Cc: Bruce Allan <bruce.w.allan@intel.com>
>> Cc: John Ronciak <john.ronciak@intel.com>
>> Cc: Mitch Williams <mitch.a.williams@intel.com>
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 2976df77bf14f5..b2f2cf40f06a87 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work)
>>               return;
>>       }
>>  #ifdef CONFIG_IXGBE_VXLAN
>> +     rtnl_lock();
>>       if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
>>               adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
>>               vxlan_get_rx_port(adapter->netdev);
>>       }
>> +     rtnl_unlock();
>>  #endif /* CONFIG_IXGBE_VXLAN */
>>       ixgbe_reset_subtask(adapter);
>>       ixgbe_phy_interrupt_subtask(adapter);
>
> Although there might be a deadlock with a concurrent
> cancel_work_sync(&adapter->service_task);  from ixgbe_remove()

I don't think we are holding the rtnl_lock when that is called.  The
ixgbe_remove function is a PCI function.  The ixgbe_close routine is
the one called with the rtnl_lock held.

- Alex

Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock

[Alexander Duyck]

On Mon, Apr 18, 2016 at 12:19 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> vxlan_get_rx_port requires rtnl_lock to be held.
>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Shannon Nelson <shannon.nelson@intel.com>
> Cc: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Cc: Don Skidmore <donald.c.skidmore@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> Cc: John Ronciak <john.ronciak@intel.com>
> Cc: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 2976df77bf14f5..b2f2cf40f06a87 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work)
>                 return;
>         }
>  #ifdef CONFIG_IXGBE_VXLAN
> +       rtnl_lock();
>         if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
>                 adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
>                 vxlan_get_rx_port(adapter->netdev);
>         }
> +       rtnl_unlock();
>  #endif /* CONFIG_IXGBE_VXLAN */
>         ixgbe_reset_subtask(adapter);
>         ixgbe_phy_interrupt_subtask(adapter);
> --
> 2.5.5
>

Would it be possible to only take the lock inside of the conditional
statement?  There isn't really any need to be holding the rtnl_lock to
check the adapter flag.

- Alex