LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow
@ 2018-06-07 19:19 Gustavo A. R. Silva
  2018-06-08 23:08 ` Jason Gunthorpe
  0 siblings, 1 reply; 5+ messages in thread
From: Gustavo A. R. Silva @ 2018-06-07 19:19 UTC (permalink / raw)
  To: Raed Salem, Leon Romanovsky, Doug Ledford, Jason Gunthorpe
  Cc: linux-rdma, linux-kernel, Gustavo A. R. Silva

In case memory resources for *ucmd* were allocated, release them
before return.

Addresses-Coverity-ID: 1469857 ("Resource leak")
Fixes: 3b3233fbf02e ("IB/mlx5: Add flow counters binding support")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/infiniband/hw/mlx5/main.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index e52dd21..0472e6c 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3546,29 +3546,35 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
 			return ERR_PTR(-ENOMEM);
 
 		err = ib_copy_from_udata(ucmd, udata, required_ucmd_sz);
-		if (err) {
-			kfree(ucmd);
-			return ERR_PTR(err);
-		}
+		if (err)
+			goto free_ucmd;
 	}
 
-	if (flow_attr->priority > MLX5_IB_FLOW_LAST_PRIO)
-		return ERR_PTR(-ENOMEM);
+	if (flow_attr->priority > MLX5_IB_FLOW_LAST_PRIO) {
+		err = -ENOMEM;
+		goto free_ucmd;
+	}
 
 	if (domain != IB_FLOW_DOMAIN_USER ||
 	    flow_attr->port > dev->num_ports ||
 	    (flow_attr->flags & ~(IB_FLOW_ATTR_FLAGS_DONT_TRAP |
-				  IB_FLOW_ATTR_FLAGS_EGRESS)))
-		return ERR_PTR(-EINVAL);
+				  IB_FLOW_ATTR_FLAGS_EGRESS))) {
+		err = -EINVAL;
+		goto free_ucmd;
+	}
 
 	if (is_egress &&
 	    (flow_attr->type == IB_FLOW_ATTR_ALL_DEFAULT ||
-	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT))
-		return ERR_PTR(-EINVAL);
+	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT)) {
+		err = -EINVAL;
+		goto free_ucmd;
+	}
 
 	dst = kzalloc(sizeof(*dst), GFP_KERNEL);
-	if (!dst)
-		return ERR_PTR(-ENOMEM);
+	if (!dst) {
+		err = -ENOMEM;
+		goto free_ucmd;
+	}
 
 	mutex_lock(&dev->flow_db->lock);
 
@@ -3640,6 +3646,9 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
 	kfree(ucmd);
 	kfree(handler);
 	return ERR_PTR(err);
+free_ucmd:
+	kfree(ucmd);
+	return ERR_PTR(err);
 }
 
 static u32 mlx5_ib_flow_action_flags_to_accel_xfrm_flags(u32 mlx5_flags)
-- 
2.7.4

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow
  2018-06-07 19:19 [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow Gustavo A. R. Silva
@ 2018-06-08 23:08 ` Jason Gunthorpe
  2018-06-08 23:11   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2018-06-08 23:08 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Raed Salem, Leon Romanovsky, Doug Ledford, linux-rdma, linux-kernel

On Thu, Jun 07, 2018 at 02:19:15PM -0500, Gustavo A. R. Silva wrote:
> In case memory resources for *ucmd* were allocated, release them
> before return.
> 
> Addresses-Coverity-ID: 1469857 ("Resource leak")
> Fixes: 3b3233fbf02e ("IB/mlx5: Add flow counters binding support")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>  drivers/infiniband/hw/mlx5/main.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index e52dd21..0472e6c 100644
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -3546,29 +3546,35 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
>  			return ERR_PTR(-ENOMEM);
>  
>  		err = ib_copy_from_udata(ucmd, udata, required_ucmd_sz);
> -		if (err) {
> -			kfree(ucmd);
> -			return ERR_PTR(err);
> -		}
> +		if (err)
> +			goto free_ucmd;
>  	}
>  
> -	if (flow_attr->priority > MLX5_IB_FLOW_LAST_PRIO)
> -		return ERR_PTR(-ENOMEM);
> +	if (flow_attr->priority > MLX5_IB_FLOW_LAST_PRIO) {
> +		err = -ENOMEM;
> +		goto free_ucmd;
> +	}
>  
>  	if (domain != IB_FLOW_DOMAIN_USER ||
>  	    flow_attr->port > dev->num_ports ||
>  	    (flow_attr->flags & ~(IB_FLOW_ATTR_FLAGS_DONT_TRAP |
> -				  IB_FLOW_ATTR_FLAGS_EGRESS)))
> -		return ERR_PTR(-EINVAL);
> +				  IB_FLOW_ATTR_FLAGS_EGRESS))) {
> +		err = -EINVAL;
> +		goto free_ucmd;
> +	}
>  
>  	if (is_egress &&
>  	    (flow_attr->type == IB_FLOW_ATTR_ALL_DEFAULT ||
> -	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT))
> -		return ERR_PTR(-EINVAL);
> +	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT)) {
> +		err = -EINVAL;
> +		goto free_ucmd;
> +	}
>  
>  	dst = kzalloc(sizeof(*dst), GFP_KERNEL);
> -	if (!dst)
> -		return ERR_PTR(-ENOMEM);
> +	if (!dst) {
> +		err = -ENOMEM;
> +		goto free_ucmd;
> +	}
>  
>  	mutex_lock(&dev->flow_db->lock);
>  
> @@ -3640,6 +3646,9 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
>  	kfree(ucmd);
>  	kfree(handler);
>  	return ERR_PTR(err);
> +free_ucmd:
> +	kfree(ucmd);
> +	return ERR_PTR(err);
>  }

This hunk is a bit wonky, can we do this instead? handle never needs
to be freed.

destroy_ft:
	put_flow_table(dev, ft_prio, false);
	if (ft_prio_tx)
		put_flow_table(dev, ft_prio_tx, false);
unlock:
	mutex_unlock(&dev->flow_db->lock);
	kfree(dst);
free_ucmd:
	kfree(ucmd);
	return ERR_PTR(err);
}


Jason

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow
  2018-06-08 23:08 ` Jason Gunthorpe
@ 2018-06-08 23:11   ` Gustavo A. R. Silva
  2018-06-08 23:21     ` Jason Gunthorpe
  0 siblings, 1 reply; 5+ messages in thread
From: Gustavo A. R. Silva @ 2018-06-08 23:11 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Raed Salem, Leon Romanovsky, Doug Ledford, linux-rdma, linux-kernel


>>   
>>   	mutex_lock(&dev->flow_db->lock);
>>   
>> @@ -3640,6 +3646,9 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
>>   	kfree(ucmd);
>>   	kfree(handler);
>>   	return ERR_PTR(err);
>> +free_ucmd:
>> +	kfree(ucmd);
>> +	return ERR_PTR(err);
>>   }
> 
> This hunk is a bit wonky, can we do this instead? handle never needs
> to be freed.
> 
> destroy_ft:
> 	put_flow_table(dev, ft_prio, false);
> 	if (ft_prio_tx)
> 		put_flow_table(dev, ft_prio_tx, false);
> unlock:
> 	mutex_unlock(&dev->flow_db->lock);
> 	kfree(dst);
> free_ucmd:
> 	kfree(ucmd);
> 	return ERR_PTR(err);
> }
> 
> 

Sure thing. I'll send v2 shortly.

Thanks for the feedback.
--
Gustavo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow
  2018-06-08 23:11   ` Gustavo A. R. Silva
@ 2018-06-08 23:21     ` Jason Gunthorpe
  2018-06-08 23:24       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2018-06-08 23:21 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Raed Salem, Leon Romanovsky, Doug Ledford, linux-rdma, linux-kernel

On Fri, Jun 08, 2018 at 06:11:49PM -0500, Gustavo A. R. Silva wrote:
> 
> >>  	mutex_lock(&dev->flow_db->lock);
> >>@@ -3640,6 +3646,9 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
> >>  	kfree(ucmd);
> >>  	kfree(handler);
> >>  	return ERR_PTR(err);
> >>+free_ucmd:
> >>+	kfree(ucmd);
> >>+	return ERR_PTR(err);
> >>  }
> >
> >This hunk is a bit wonky, can we do this instead? handle never needs
> >to be freed.
> >
> >destroy_ft:
> >	put_flow_table(dev, ft_prio, false);
> >	if (ft_prio_tx)
> >		put_flow_table(dev, ft_prio_tx, false);
> >unlock:
> >	mutex_unlock(&dev->flow_db->lock);
> >	kfree(dst);
> >free_ucmd:
> >	kfree(ucmd);
> >	return ERR_PTR(err);
> >}
> >
> >
> 
> Sure thing. I'll send v2 shortly.

I made the adjustment for you, if it is wrong let me know..

https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-rc&id=2cc82dd58712888758c2d2b405d012f2cd580dd7

Jason

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow
  2018-06-08 23:21     ` Jason Gunthorpe
@ 2018-06-08 23:24       ` Gustavo A. R. Silva
  0 siblings, 0 replies; 5+ messages in thread
From: Gustavo A. R. Silva @ 2018-06-08 23:24 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Raed Salem, Leon Romanovsky, Doug Ledford, linux-rdma, linux-kernel


>>
>> Sure thing. I'll send v2 shortly.
> 
> I made the adjustment for you, if it is wrong let me know..
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-rc&id=2cc82dd58712888758c2d2b405d012f2cd580dd7
> 
> Jason
> 

Seems correct. That's exactly what I was about to send.

Thanks, Jason!
--
Gustavo

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-06-08 23:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07 19:19 [PATCH] IB/mlx5: Fix memory leak in mlx5_ib_create_flow Gustavo A. R. Silva
2018-06-08 23:08 ` Jason Gunthorpe
2018-06-08 23:11   ` Gustavo A. R. Silva
2018-06-08 23:21     ` Jason Gunthorpe
2018-06-08 23:24       ` Gustavo A. R. Silva

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).