diff mbox series

[for-rc,2/7] IB/hfi1: Use new API to deallocate vnic rdma-netdev in hfi1 driver

Message ID 20190117204117.30826.2346.stgit@scvm10.sc.intel.com (mailing list archive)
State Changes Requested
Delegated to: Jason Gunthorpe
Headers show
Series hfi1 and qib patches for rc | expand

Commit Message

Dennis Dalessandro Jan. 17, 2019, 8:41 p.m. UTC 
From: Andrzej Witkowski <andrzej.witkowski@intel.com>

VNIC uses the old deprecated path for freeing netdev private device
information. This will lead to a memory leak (of netdev devices)
and possibly incorrect cleanup behavior.

Fix by using the new API in the VNIC code path.

Fixes: 9f49a5b5c21d ("RDMA/netdev: Use priv_destructor for netdev cleanup")
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Andrzej Witkowski <andrzej.witkowski@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/vnic_main.c            |    4 ++--
 drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c |    5 ++---
 include/rdma/ib_verbs.h                           |    7 -------
 3 files changed, 4 insertions(+), 12 deletions(-)

Comments

Jason Gunthorpe Jan. 17, 2019, 9:09 p.m. UTC | #1 
On Thu, Jan 17, 2019 at 12:41:22PM -0800, Dennis Dalessandro wrote:
> From: Andrzej Witkowski <andrzej.witkowski@intel.com>
> 
> VNIC uses the old deprecated path for freeing netdev private device
> information. This will lead to a memory leak (of netdev devices)
> and possibly incorrect cleanup behavior.

> Fix by using the new API in the VNIC code path.
> 
> Fixes: 9f49a5b5c21d ("RDMA/netdev: Use priv_destructor for netdev cleanup")

? vnic's flow didn't change at all in that patch, specifically to not
create new bugs in it. I think it was already broken mind you.. 

What did that patch break, why is it listed as Fixes ?

You need a better commit message for -rc patches, what is the exact
flow that leaks a netdev?

> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Signed-off-by: Andrzej Witkowski <andrzej.witkowski@intel.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
>  drivers/infiniband/hw/hfi1/vnic_main.c            |    4 ++--
>  drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c |    5 ++---
>  include/rdma/ib_verbs.h                           |    7 -------
>  3 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c
> index a922db5..1d66c43 100644
> +++ b/drivers/infiniband/hw/hfi1/vnic_main.c
> @@ -789,7 +789,6 @@ static void hfi1_vnic_free_rn(struct net_device *netdev)
>  
>  	hfi1_vnic_deinit(vinfo);
>  	mutex_destroy(&vinfo->lock);
> -	free_netdev(netdev);
>  }
>  
>  struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
> @@ -826,7 +825,6 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  	vinfo->num_tx_q = dd->num_sdma;
>  	vinfo->num_rx_q = dd->num_vnic_contexts;
>  	vinfo->netdev = netdev;
> -	rn->free_rdma_netdev = hfi1_vnic_free_rn;
>  	rn->set_id = hfi1_vnic_set_vesw_id;
>  
>  	netdev->features = NETIF_F_HIGHDMA | NETIF_F_SG;
> @@ -834,6 +832,8 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  	netdev->vlan_features = netdev->features;
>  	netdev->watchdog_timeo = msecs_to_jiffies(HFI_TX_TIMEOUT_MS);
>  	netdev->netdev_ops = &hfi1_netdev_ops;
> +	netdev->priv_destructor = hfi1_vnic_free_rn;
> +	netdev->needs_free_netdev = true;
>  	mutex_init(&vinfo->lock);
>  
>  	for (i = 0; i < vinfo->num_rx_q; i++) {
> diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
> index ae70cd1..e21dc73 100644
> +++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
> @@ -382,7 +382,8 @@ struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
>  	mutex_destroy(&adapter->mactbl_lock);
>  	kfree(adapter);
>  adapter_err:
> -	rn->free_rdma_netdev(netdev);
> +	netdev->priv_destructor(netdev);
> +	free_netdev(netdev);

register_netdev sometimes calls the priv_destructor, and sometimes
doesn't, so this is a double free on error paths. Look at how ipoib
works for guidance..

Jason
Dennis Dalessandro Jan. 18, 2019, 12:06 p.m. UTC | #2 
On 1/17/2019 4:09 PM, Jason Gunthorpe wrote:
> On Thu, Jan 17, 2019 at 12:41:22PM -0800, Dennis Dalessandro wrote:
>> From: Andrzej Witkowski <andrzej.witkowski@intel.com>
>>
>> VNIC uses the old deprecated path for freeing netdev private device
>> information. This will lead to a memory leak (of netdev devices)
>> and possibly incorrect cleanup behavior.
> 
>> Fix by using the new API in the VNIC code path.
>>
>> Fixes: 9f49a5b5c21d ("RDMA/netdev: Use priv_destructor for netdev cleanup")
> 
> ? vnic's flow didn't change at all in that patch, specifically to not
> create new bugs in it. I think it was already broken mind you..

Quite possible. Will look into it.

> register_netdev sometimes calls the priv_destructor, and sometimes
> doesn't, so this is a double free on error paths. Look at how ipoib
> works for guidance..

Will work with the patch author internally and revise.

-Denny
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c
index a922db5..1d66c43 100644
--- a/drivers/infiniband/hw/hfi1/vnic_main.c
+++ b/drivers/infiniband/hw/hfi1/vnic_main.c
@@ -789,7 +789,6 @@  static void hfi1_vnic_free_rn(struct net_device *netdev)
 
 	hfi1_vnic_deinit(vinfo);
 	mutex_destroy(&vinfo->lock);
-	free_netdev(netdev);
 }
 
 struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
@@ -826,7 +825,6 @@  struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 	vinfo->num_tx_q = dd->num_sdma;
 	vinfo->num_rx_q = dd->num_vnic_contexts;
 	vinfo->netdev = netdev;
-	rn->free_rdma_netdev = hfi1_vnic_free_rn;
 	rn->set_id = hfi1_vnic_set_vesw_id;
 
 	netdev->features = NETIF_F_HIGHDMA | NETIF_F_SG;
@@ -834,6 +832,8 @@  struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 	netdev->vlan_features = netdev->features;
 	netdev->watchdog_timeo = msecs_to_jiffies(HFI_TX_TIMEOUT_MS);
 	netdev->netdev_ops = &hfi1_netdev_ops;
+	netdev->priv_destructor = hfi1_vnic_free_rn;
+	netdev->needs_free_netdev = true;
 	mutex_init(&vinfo->lock);
 
 	for (i = 0; i < vinfo->num_rx_q; i++) {
diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
index ae70cd1..e21dc73 100644
--- a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
+++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
@@ -382,7 +382,8 @@  struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 	mutex_destroy(&adapter->mactbl_lock);
 	kfree(adapter);
 adapter_err:
-	rn->free_rdma_netdev(netdev);
+	netdev->priv_destructor(netdev);
+	free_netdev(netdev);
 
 	return ERR_PTR(rc);
 }
@@ -391,7 +392,6 @@  struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 void opa_vnic_rem_netdev(struct opa_vnic_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
-	struct rdma_netdev *rn = netdev_priv(netdev);
 
 	v_info("removing\n");
 	unregister_netdev(netdev);
@@ -399,5 +399,4 @@  void opa_vnic_rem_netdev(struct opa_vnic_adapter *adapter)
 	mutex_destroy(&adapter->lock);
 	mutex_destroy(&adapter->mactbl_lock);
 	kfree(adapter);
-	rn->free_rdma_netdev(netdev);
 }
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index a3ceed3..7539030 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2213,13 +2213,6 @@  struct rdma_netdev {
 	struct ib_device  *hca;
 	u8                 port_num;
 
-	/*
-	 * cleanup function must be specified.
-	 * FIXME: This is only used for OPA_VNIC and that usage should be
-	 * removed too.
-	 */
-	void (*free_rdma_netdev)(struct net_device *netdev);
-
 	/* control functions */
 	void (*set_id)(struct net_device *netdev, int id);
 	/* send packet */