Fixes for IPoIB flows

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 7 ++++++-
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 5 ++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

index a115c0b7a310..0ddd9709e1df 100644
@@ -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);
 	}
 

index 36dc4fcaa3cd..1ee46194bbf5 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

Limit calls to free_rdma_netdev() for capable devices only.

---
 drivers/infiniband/hw/mlx5/main.c         | 6 ++++--
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

index 0c79983c8b1a..9ecc089d4529 100644
@@ -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;

index 0ddd9709e1df..91fae34bdd4f 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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);

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

Thanks

Yes, it is technicaly OK

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 +++
 1 file changed, 3 insertions(+)

index 91fae34bdd4f..1015a63de6ae 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

---
 drivers/infiniband/ulp/ipoib/ipoib_ib.c | 1 -
 1 file changed, 1 deletion(-)

index 0060b2f9f659..efe7402f4885 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 1ee46194bbf5..081b33deff1b 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Doug Ledford <[email protected]>
    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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Thanks