On Thu, Jan 17, 2019 at 12:41:22PM -0800, Dennis Dalessandro wrote:
> From: Andrzej Witkowski <andrzej.witkowski@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Andrzej Witkowski <andrzej.witkowski@xxxxxxxxx>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> 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
