[PATCH rdma-rc 0/5] Fixes for IPoIB flows

[PATCH rdma-rc 0/5] Fixes for IPoIB flows

[Leon Romanovsky]

Hi Doug,

There are 5 small patches from Alex and Feras for IPoIB flows.

Thanks

Alex Vesker (4):
  IB/ipoib: Fix memory leaks for child interfaces priv
  IB/ipoib: Limit call to free rdma_netdev for capable devices
  IB/ipoib: Delete napi in device uninit default
  IB/ipoib: Fix access to un-initialized napi struct

Feras Daoud (1):
  IB/ipoib: Fix memory leak in create child syscall

 drivers/infiniband/hw/mlx5/main.c         |  6 ++++--
 drivers/infiniband/ulp/ipoib/ipoib_ib.c   |  1 -
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 15 +++++++++++++--
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 11 +++++++----
 4 files changed, 24 insertions(+), 9 deletions(-)

--
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 1/5] IB/ipoib: Fix memory leaks for child interfaces priv

[Leon Romanovsky]

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

There is a need to free priv explicitly and not just to release
the device, child priv is freed explicitly on remove flow and this
patch also includes priv free on error flow in P_key creation
and also in add_port.

Fixes: cd565b4b51e5 ('IB/IPoIB: Support acceleration options callbacks')
Signed-off-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 7 ++++++-
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 5 ++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index a115c0b7a310..0ddd9709e1df 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2237,6 +2237,7 @@ static struct net_device *ipoib_add_port(const char *format,
 
 device_init_failed:
 	free_netdev(priv->dev);
+	kfree(priv);
 
 alloc_mem_failed:
 	return ERR_PTR(result);
@@ -2277,7 +2278,7 @@ static void ipoib_add_one(struct ib_device *device)
 
 static void ipoib_remove_one(struct ib_device *device, void *client_data)
 {
-	struct ipoib_dev_priv *priv, *tmp;
+	struct ipoib_dev_priv *priv, *tmp, *cpriv, *tcpriv;
 	struct list_head *dev_list = client_data;
 
 	if (!dev_list)
@@ -2301,6 +2302,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 
 		unregister_netdev(priv->dev);
 		free_netdev(priv->dev);
+
+		list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list)
+			kfree(cpriv);
+
 		kfree(priv);
 	}
 
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 36dc4fcaa3cd..1ee46194bbf5 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -167,8 +167,10 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 
 	rtnl_unlock();
 
-	if (result)
+	if (result) {
 		free_netdev(priv->dev);
+		kfree(priv);
+	}
 
 	return result;
 }
@@ -209,6 +211,7 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 
 	if (dev) {
 		free_netdev(dev);
+		kfree(priv);
 		return 0;
 	}
 
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Leon Romanovsky]

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Limit calls to free_rdma_netdev() for capable devices only.

Fixes: cd565b4b51e5 ('IB/IPoIB: Support acceleration options callbacks')
Signed-off-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/main.c         | 6 ++++--
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 0c79983c8b1a..9ecc089d4529 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3692,8 +3692,10 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
 	dev->ib_dev.check_mr_status	= mlx5_ib_check_mr_status;
 	dev->ib_dev.get_port_immutable  = mlx5_port_immutable;
 	dev->ib_dev.get_dev_fw_str      = get_dev_fw_str;
-	dev->ib_dev.alloc_rdma_netdev	= mlx5_ib_alloc_rdma_netdev;
-	dev->ib_dev.free_rdma_netdev	= mlx5_ib_free_rdma_netdev;
+	if (MLX5_CAP_GEN(mdev, ipoib_enhanced_offloads)) {
+		dev->ib_dev.alloc_rdma_netdev	= mlx5_ib_alloc_rdma_netdev;
+		dev->ib_dev.free_rdma_netdev	= mlx5_ib_free_rdma_netdev;
+	}
 	if (mlx5_core_is_pf(mdev)) {
 		dev->ib_dev.get_vf_config	= mlx5_ib_get_vf_config;
 		dev->ib_dev.set_vf_link_state	= mlx5_ib_set_vf_link_state;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 0ddd9709e1df..91fae34bdd4f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2301,7 +2301,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 		flush_workqueue(priv->wq);
 
 		unregister_netdev(priv->dev);
-		free_netdev(priv->dev);
+		if (device->free_rdma_netdev)
+			device->free_rdma_netdev(priv->dev);
+		else
+			free_netdev(priv->dev);
 
 		list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list)
 			kfree(cpriv);
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 3/5] IB/ipoib: Delete napi in device uninit default

[Leon Romanovsky]

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch mekas init_default and uninit_default symmetric
with a call to delete napi. Additionally, the uninit_default
gained delete napi call in case of init_default fails.

Fixes: 515ed4f3aab4 ('IB/IPoIB: Separate control and data related initializations')
Signed-off-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 91fae34bdd4f..1015a63de6ae 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1596,6 +1596,8 @@ static void ipoib_dev_uninit_default(struct net_device *dev)
 
 	ipoib_transport_dev_cleanup(dev);
 
+	netif_napi_del(&priv->napi);
+
 	ipoib_cm_dev_cleanup(dev);
 
 	kfree(priv->rx_ring);
@@ -1649,6 +1651,7 @@ static int ipoib_dev_init_default(struct net_device *dev)
 	kfree(priv->rx_ring);
 
 out:
+	netif_napi_del(&priv->napi);
 	return -ENOMEM;
 }
 
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 4/5] IB/ipoib: Fix access to un-initialized napi struct

[Leon Romanovsky]

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

There is no need to re-enable napi since we set the initialized
flag before calling ipoib_ib_dev_stop which will disable napi,
disabling napi twice is harmless in case it was already disabled.

One more reason for this fix is that when using IPoIB new device
driver napi is not added to priv, this can lead to kernel panic
when rn_ops ndo_open fails.

[ 289.755840] invalid opcode: 0000 [#1] SMP
[ 289.757111] task: ffff880036964440 ti: ffff880178ee8000 task.ti: ffff880178ee8000
[ 289.757111] RIP: 0010:[<ffffffffa05368d6>] [<ffffffffa05368d6>] napi_enable.part.24+0x4/0x6 [ib_ipoib]
[ 289.757111] RSP: 0018:ffff880178eeb6d8 EFLAGS: 00010246
[ 289.757111] RAX: 0000000000000000 RBX: ffff880177a80010 RCX: 000000007fffffff
[ 289.757111] RDX: ffffffff81d5f118 RSI: 0000000000000000 RDI: ffff880177a80010
[ 289.757111] RBP: ffff880178eeb6d8 R08: 0000000000000082 R09: 0000000000000283
[ 289.757111] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880175a00000
[ 289.757111] R13: ffff880177a80080 R14: 0000000000000000 R15: 0000000000000001
[ 289.757111] FS: 00007fe2ee346880(0000) GS:ffff88017fc00000(0000) knlGS:0000000000000000
[ 289.757111] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 289.757111] CR2: 00007fffca979020 CR3: 00000001792e4000 CR4: 00000000000006f0
[ 289.757111] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 289.757111] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 289.757111] Stack:
[ 289.796027] ffff880178eeb6f0 ffffffffa05251f5 ffff880177a80000 ffff880178eeb718
[ 289.796027] ffffffffa0528505 ffff880175a00000 ffff880177a80000 0000000000000000
[ 289.796027] ffff880178eeb748 ffffffffa051f0ab ffff880175a00000 ffffffffa0537d60
[ 289.796027] Call Trace:
[ 289.796027] [<ffffffffa05251f5>] napi_enable+0x25/0x30 [ib_ipoib]
[ 289.796027] [<ffffffffa0528505>] ipoib_ib_dev_open+0x175/0x190 [ib_ipoib]
[ 289.796027] [<ffffffffa051f0ab>] ipoib_open+0x4b/0x160 [ib_ipoib]
[ 289.796027] [<ffffffff814fe33f>] _dev_open+0xbf/0x130
[ 289.796027] [<ffffffff814fe62d>] __dev_change_flags+0x9d/0x170
[ 289.796027] [<ffffffff814fe729>] dev_change_flags+0x29/0x60
[ 289.796027] [<ffffffff8150caf7>] do_setlink+0x397/0xa40

Fixes: cd565b4b51e5 ('IB/IPoIB: Support acceleration options callbacks')
Signed-off-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_ib.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
index 0060b2f9f659..efe7402f4885 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -863,7 +863,6 @@ int ipoib_ib_dev_open(struct net_device *dev)
 	set_bit(IPOIB_STOP_REAPER, &priv->flags);
 	cancel_delayed_work(&priv->ah_reap_task);
 	set_bit(IPOIB_FLAG_INITIALIZED, &priv->flags);
-	napi_enable(&priv->napi);
 	ipoib_ib_dev_stop(dev);
 	return -1;
 }
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 5/5] IB/ipoib: Fix memory leak in create child syscall

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The flow of creating a new child goes through ipoib_vlan_add
which allocates a new interface and checks the rtnl_lock.

If the lock is taken, restart_syscall will be called to restart
the system call again. In this case we are not releasing the
already allocated interface, causing a leak.

Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 1ee46194bbf5..081b33deff1b 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -133,13 +133,13 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 	snprintf(intf_name, sizeof intf_name, "%s.%04x",
 		 ppriv->dev->name, pkey);
 
+	if (!rtnl_trylock())
+		return restart_syscall();
+
 	priv = ipoib_intf_alloc(ppriv->ca, ppriv->port, intf_name);
 	if (!priv)
 		return -ENOMEM;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
 	down_write(&ppriv->vlan_rwsem);
 
 	/*
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/5] Fixes for IPoIB flows

[Doug Ledford]

On Wed, 2017-06-14 at 09:59 +0300, Leon Romanovsky wrote:
> Hi Doug,
> 
> There are 5 small patches from Alex and Feras for IPoIB flows.
> 
> Thanks
> 
> Alex Vesker (4):
>   IB/ipoib: Fix memory leaks for child interfaces priv
>   IB/ipoib: Limit call to free rdma_netdev for capable devices
>   IB/ipoib: Delete napi in device uninit default
>   IB/ipoib: Fix access to un-initialized napi struct
> 
> Feras Daoud (1):
>   IB/ipoib: Fix memory leak in create child syscall
> 
>  drivers/infiniband/hw/mlx5/main.c         |  6 ++++--
>  drivers/infiniband/ulp/ipoib/ipoib_ib.c   |  1 -
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 15 +++++++++++++--
>  drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 11 +++++++----
>  4 files changed, 24 insertions(+), 9 deletions(-)

Thanks, series applied.  We are close to done with -rc fixes I hope,
yes?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/5] Fixes for IPoIB flows

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Wed, Jun 14, 2017 at 03:20:07PM -0400, Doug Ledford wrote:
> On Wed, 2017-06-14 at 09:59 +0300, Leon Romanovsky wrote:
> > Hi Doug,
> >
> > There are 5 small patches from Alex and Feras for IPoIB flows.
> >
> > Thanks
> >
> > Alex Vesker (4):
> >   IB/ipoib: Fix memory leaks for child interfaces priv
> >   IB/ipoib: Limit call to free rdma_netdev for capable devices
> >   IB/ipoib: Delete napi in device uninit default
> >   IB/ipoib: Fix access to un-initialized napi struct
> >
> > Feras Daoud (1):
> >   IB/ipoib: Fix memory leak in create child syscall
> >
> >  drivers/infiniband/hw/mlx5/main.c         |  6 ++++--
> >  drivers/infiniband/ulp/ipoib/ipoib_ib.c   |  1 -
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 15 +++++++++++++--
> >  drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 11 +++++++----
> >  4 files changed, 24 insertions(+), 9 deletions(-)
>
> Thanks, series applied.  We are close to done with -rc fixes I hope,
> yes?

Yes, we don't have anything what deserves such late rc.
Everything else will go to ->next.

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Vishwanathapura, Niranjana]

On Wed, Jun 14, 2017 at 09:59:06AM +0300, Leon Romanovsky wrote:
>From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
>Limit calls to free_rdma_netdev() for capable devices only.
>
>Fixes: cd565b4b51e5 ('IB/IPoIB: Support acceleration options callbacks')
>Signed-off-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>---
> drivers/infiniband/hw/mlx5/main.c         | 6 ++++--
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
>index 0c79983c8b1a..9ecc089d4529 100644
>--- a/drivers/infiniband/hw/mlx5/main.c
>+++ b/drivers/infiniband/hw/mlx5/main.c
>@@ -3692,8 +3692,10 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
> 	dev->ib_dev.check_mr_status	= mlx5_ib_check_mr_status;
> 	dev->ib_dev.get_port_immutable  = mlx5_port_immutable;
> 	dev->ib_dev.get_dev_fw_str      = get_dev_fw_str;
>-	dev->ib_dev.alloc_rdma_netdev	= mlx5_ib_alloc_rdma_netdev;
>-	dev->ib_dev.free_rdma_netdev	= mlx5_ib_free_rdma_netdev;
>+	if (MLX5_CAP_GEN(mdev, ipoib_enhanced_offloads)) {
>+		dev->ib_dev.alloc_rdma_netdev	= mlx5_ib_alloc_rdma_netdev;
>+		dev->ib_dev.free_rdma_netdev	= mlx5_ib_free_rdma_netdev;
>+	}
> 	if (mlx5_core_is_pf(mdev)) {
> 		dev->ib_dev.get_vf_config	= mlx5_ib_get_vf_config;
> 		dev->ib_dev.set_vf_link_state	= mlx5_ib_set_vf_link_state;
>diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>index 0ddd9709e1df..91fae34bdd4f 100644
>--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
>+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>@@ -2301,7 +2301,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
> 		flush_workqueue(priv->wq);
>
> 		unregister_netdev(priv->dev);
>-		free_netdev(priv->dev);
>+		if (device->free_rdma_netdev)
>+			device->free_rdma_netdev(priv->dev);
>+		else
>+			free_netdev(priv->dev);

This is causing an crash in hfi1 driver.
If the call to alloc_rdma_netdev() has returned -EOPNOTSUPP, we shouldn't be 
calling free_rdma_netdev() here (as device driver doesn't own that netdev).
I will send out a patch to hfi1 driver to guard check for the type in its 
free_rdma_netdev() call to handle such issues.

But we need to properly free the netdev here, probably by something like this.

if ((priv->dev->netdev_ops != ipoib_netdev_default_pf) &&
     device->free_rdma_netdev)
	device->free_rdma_netdev(priv->dev);
else
	free_netdev(priv->dev);

Niranjana

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Jason Gunthorpe]

On Tue, Jun 27, 2017 at 10:34:07AM -0700, Vishwanathapura, Niranjana wrote:
> >diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> >index 0ddd9709e1df..91fae34bdd4f 100644
> >+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> >@@ -2301,7 +2301,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
> >		flush_workqueue(priv->wq);
> >
> >		unregister_netdev(priv->dev);
> >-		free_netdev(priv->dev);
> >+		if (device->free_rdma_netdev)
> >+			device->free_rdma_netdev(priv->dev);
> >+		else
> >+			free_netdev(priv->dev);
> 
> This is causing an crash in hfi1 driver.
> If the call to alloc_rdma_netdev() has returned -EOPNOTSUPP, we shouldn't be
> calling free_rdma_netdev() here (as device driver doesn't own that netdev).
> I will send out a patch to hfi1 driver to guard check for the type in its
> free_rdma_netdev() call to handle such issues.

I think instead you should move free_rdma_netdev from struct ib_device
to struct rdma_netdev to prevent this mistake in general.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Vishwanathapura, Niranjana]

On Tue, Jun 27, 2017 at 11:41:56AM -0600, Jason Gunthorpe wrote:
>On Tue, Jun 27, 2017 at 10:34:07AM -0700, Vishwanathapura, Niranjana wrote:
>> >diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>> >index 0ddd9709e1df..91fae34bdd4f 100644
>> >+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>> >@@ -2301,7 +2301,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
>> >		flush_workqueue(priv->wq);
>> >
>> >		unregister_netdev(priv->dev);
>> >-		free_netdev(priv->dev);
>> >+		if (device->free_rdma_netdev)
>> >+			device->free_rdma_netdev(priv->dev);
>> >+		else
>> >+			free_netdev(priv->dev);
>>
>> This is causing an crash in hfi1 driver.
>> If the call to alloc_rdma_netdev() has returned -EOPNOTSUPP, we shouldn't be
>> calling free_rdma_netdev() here (as device driver doesn't own that netdev).
>> I will send out a patch to hfi1 driver to guard check for the type in its
>> free_rdma_netdev() call to handle such issues.
>
>I think instead you should move free_rdma_netdev from struct ib_device
>to struct rdma_netdev to prevent this mistake in general.
>

Thanks, that is another option. With that, we will be freeing the rdma_netdev 
from a function pointer call that belongs to that rdma_netdev. Is that ok?  
(Though I think it should work fine).

Also, I think this needs updating ib_verbs.h, hfi1, ipoib and mlx5 (under 
drivers/net) together. I can make the change, if this is the direction we are 
taking.

Another option is to add a new type RDMA_NETDEV_IPOIB_DEF and use only within 
ipoib driver to differentiate default case.

Thanks,
Niranjana

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

On Tue, Jun 27, 2017 at 12:08:38PM -0700, Vishwanathapura, Niranjana wrote:
> On Tue, Jun 27, 2017 at 11:41:56AM -0600, Jason Gunthorpe wrote:
> > On Tue, Jun 27, 2017 at 10:34:07AM -0700, Vishwanathapura, Niranjana wrote:
> > > >diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > >index 0ddd9709e1df..91fae34bdd4f 100644
> > > >+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > >@@ -2301,7 +2301,10 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
> > > >		flush_workqueue(priv->wq);
> > > >
> > > >		unregister_netdev(priv->dev);
> > > >-		free_netdev(priv->dev);
> > > >+		if (device->free_rdma_netdev)
> > > >+			device->free_rdma_netdev(priv->dev);
> > > >+		else
> > > >+			free_netdev(priv->dev);
> > >
> > > This is causing an crash in hfi1 driver.
> > > If the call to alloc_rdma_netdev() has returned -EOPNOTSUPP, we shouldn't be
> > > calling free_rdma_netdev() here (as device driver doesn't own that netdev).
> > > I will send out a patch to hfi1 driver to guard check for the type in its
> > > free_rdma_netdev() call to handle such issues.
> >
> > I think instead you should move free_rdma_netdev from struct ib_device
> > to struct rdma_netdev to prevent this mistake in general.
> >
>
> Thanks, that is another option. With that, we will be freeing the
> rdma_netdev from a function pointer call that belongs to that rdma_netdev.
> Is that ok?  (Though I think it should work fine).

The downside of this that the code will be non-symmetrical, for
alloc_rdma_netdev vs. free_rdma_netdev.

2198         /**
2199          * rdma netdev operations
2200          *
2201          * Driver implementing alloc_rdma_netdev must return -EOPNOTSUPP if it
2202          * doesn't support the specified rdma netdev type.
2203          */
2204         struct net_device *(*alloc_rdma_netdev)(
2205                                         struct ib_device *device,
2206                                         u8 port_num,
2207                                         enum rdma_netdev_t type,
2208                                         const char *name,
2209                                         unsigned char name_assign_type,
2210                                         void (*setup)(struct net_device *));
2211         void (*free_rdma_netdev)(struct net_device *netdev);

>
> Also, I think this needs updating ib_verbs.h, hfi1, ipoib and mlx5 (under
> drivers/net) together. I can make the change, if this is the direction we
> are taking.
>
> Another option is to add a new type RDMA_NETDEV_IPOIB_DEF and use only
> within ipoib driver to differentiate default case.

It is less optimal option, I would prefer to see IPoIB the same for all flows.

Thanks

>
> Thanks,
> Niranjana
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 2/5] IB/ipoib: Limit call to free rdma_netdev for capable devices

[Jason Gunthorpe]

On Wed, Jun 28, 2017 at 10:13:15AM +0300, Leon Romanovsky wrote:

> > Thanks, that is another option. With that, we will be freeing the
> > rdma_netdev from a function pointer call that belongs to that rdma_netdev.
> > Is that ok?  (Though I think it should work fine).

Yes, it is technicaly OK

> The downside of this that the code will be non-symmetrical, for
> alloc_rdma_netdev vs. free_rdma_netdev.

Not sure this really matters.. Obviously the way it is now is hard to
use since this mistake was made.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html