| Message ID | 20180607191915.GA9092@embeddedor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)