| Message ID | 1469732963-58257-1-git-send-email-jarod@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Jarod, Thank you for the patch, just a couple of things. Please send a V2 and we will include it. > diff --git a/src/i40iw_uverbs.c b/src/i40iw_uverbs.c index ec5f77e..e5753e0 > 100644 > --- a/src/i40iw_uverbs.c > +++ b/src/i40iw_uverbs.c > @@ -698,6 +698,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); > if (!info.rq_wrid_array) { > fprintf(stderr, PFX "%s: failed to allocate memory for RQ work > array\n", __func__); > + free(iwuqp); [Chien] Not needed, see changes below. > goto err_free_sq_wrtrk; > } > > @@ -707,6 +708,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > > if (!status) { > fprintf(stderr, PFX "%s: failed to map QP\n", __func__); > + free(iwuqp); [Chien] Not needed, see changes below. > goto err_free_rq_wrid; > } > info.qp_id = resp.qp_id; > @@ -728,12 +730,12 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > > err_destroy_qp: > i40iw_udestroy_qp(&iwuqp->ibv_qp); > - return NULL; > - > err_free_rq_wrid: > free(info.rq_wrid_array); > err_free_sq_wrtrk: > free(info.sq_wrtrk_array); > + return NULL; [Chien] Need to remove this. We don't want to return when unwinding resources Same issue as the return NULL I missed. > + > err_destroy_lock: > if (pthread_spin_destroy(&iwuqp->lock)) > return NULL; [Chien] Not sure why I would bother with the if statement here. Remove the if check and return NULL. Now everything should flow correctly in the error path. > -- > 1.8.3.1 -- 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 Thu, Jul 28, 2016 at 09:10:07PM +0000, Tung, Chien Tin wrote: > Hi Jarod, > > Thank you for the patch, just a couple of things. Please send a V2 and we will include it. > > > diff --git a/src/i40iw_uverbs.c b/src/i40iw_uverbs.c index ec5f77e..e5753e0 > > 100644 > > --- a/src/i40iw_uverbs.c > > +++ b/src/i40iw_uverbs.c > > @@ -698,6 +698,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); > > if (!info.rq_wrid_array) { > > fprintf(stderr, PFX "%s: failed to allocate memory for RQ work > > array\n", __func__); > > + free(iwuqp); > [Chien] Not needed, see changes below. > > > goto err_free_sq_wrtrk; > > } > > > > @@ -707,6 +708,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > > > if (!status) { > > fprintf(stderr, PFX "%s: failed to map QP\n", __func__); > > + free(iwuqp); > [Chien] Not needed, see changes below. > > > goto err_free_rq_wrid; > > } > > info.qp_id = resp.qp_id; > > @@ -728,12 +730,12 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > > > err_destroy_qp: > > i40iw_udestroy_qp(&iwuqp->ibv_qp); > > - return NULL; > > - > > err_free_rq_wrid: > > free(info.rq_wrid_array); > > err_free_sq_wrtrk: > > free(info.sq_wrtrk_array); > > + return NULL; > [Chien] Need to remove this. We don't want to return when unwinding resources > Same issue as the return NULL I missed. > > > + > > err_destroy_lock: > > if (pthread_spin_destroy(&iwuqp->lock)) > > return NULL; > [Chien] Not sure why I would bother with the if statement here. > Remove the if check and return NULL. Now everything should flow correctly in the error path. Hm. So one issue I still see, is that if you jump to err_destroy_qp, then pthread_spin_destroy(&iwuqp->lock) will have already been called by i40iw_udestroy_qp(), along with free(iwuqp), and I was thinking there could be a null pointer dereference issue.
diff --git a/src/i40iw_uverbs.c b/src/i40iw_uverbs.c index ec5f77e..e5753e0 100644 --- a/src/i40iw_uverbs.c +++ b/src/i40iw_uverbs.c @@ -698,6 +698,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); if (!info.rq_wrid_array) { fprintf(stderr, PFX "%s: failed to allocate memory for RQ work array\n", __func__); + free(iwuqp); goto err_free_sq_wrtrk; } @@ -707,6 +708,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr if (!status) { fprintf(stderr, PFX "%s: failed to map QP\n", __func__); + free(iwuqp); goto err_free_rq_wrid; } info.qp_id = resp.qp_id; @@ -728,12 +730,12 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr err_destroy_qp: i40iw_udestroy_qp(&iwuqp->ibv_qp); - return NULL; - err_free_rq_wrid: free(info.rq_wrid_array); err_free_sq_wrtrk: free(info.sq_wrtrk_array); + return NULL; + err_destroy_lock: if (pthread_spin_destroy(&iwuqp->lock)) return NULL;
If we get to err_destroy_qp, we still need to free info.rq_wrid_array and info.sq_wrtrk_array, so just slide the return down after those respective free() calls, and free iwuqp before gotos below err_destroy_qp, so we do call free() iwuqp, but only once (i40iw_udestroy_qp() also free()'s it). Fixes: 5f13b882d (libi40iw: Add return code check for pthread_spin calls) CC: Chien Tin Tung <chien.tin.tung@intel.com> CC: Tatyana Nikolova <tatyana.e.nikolova@intel.com> Signed-off-by: Jarod Wilson <jarod@redhat.com> --- src/i40iw_uverbs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)