| Message ID | b19e44b3-601f-c642-b8d3-12c000511ce2@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 02/10/2017 04:04 PM, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 10 Feb 2017 21:01:55 +0100 > > Reuse existing functionality from memdup_user() instead of keeping > duplicate source code. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Thanks for the patch, but this one is already taken care of along with other similar uses of kmalloc/copy: http://marc.info/?l=linux-rdma&m=148656088729538&w=2 Will review the rest of the patch series soon. -Denny -- 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
> Thanks for the patch, but this one is already taken care of along with other similar uses of kmalloc/copy: > http://marc.info/?l=linux-rdma&m=148656088729538&w=2 Thanks for your information. The shown source code is reasonable in the update step “[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy”. https://patchwork.kernel.org/patch/9562565/ https://lkml.kernel.org/r/<20170208132830.16442.93943.stgit@scvm10.sc.intel.com> I find the commit subject and message partly inappropriate. How do you think about to mention other function names there? Regards, Markus -- 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 Sat, Feb 11, 2017 at 10:32:59AM -0500, Dennis Dalessandro wrote: > On 02/10/2017 04:04 PM, SF Markus Elfring wrote: > >From: Markus Elfring <elfring@users.sourceforge.net> > >Date: Fri, 10 Feb 2017 21:01:55 +0100 > > > >Reuse existing functionality from memdup_user() instead of keeping > >duplicate source code. > > > >Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > Thanks for the patch, but this one is already taken care of along > with other similar uses of kmalloc/copy: > > http://marc.info/?l=linux-rdma&m=148656088729538&w=2 > Michael's patch doesn't change user_sdma_free_request() so it introduces a kfreeing an error pointer bug. regards, dan carpenter -- 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
>> Thanks for the patch, but this one is already taken care of along >> with other similar uses of kmalloc/copy: >> >> http://marc.info/?l=linux-rdma&m=148656088729538&w=2 >> > > Michael's patch doesn't change user_sdma_free_request() so it introduces > a kfreeing an error pointer bug. Did you notice that another local variable “tmp” was introduced in the update step “[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that the mentioned function will usually get a null pointer after a failure there? Regards, Markus -- 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 Mon, Feb 13, 2017 at 12:12:51PM +0100, SF Markus Elfring wrote: > >> Thanks for the patch, but this one is already taken care of along > >> with other similar uses of kmalloc/copy: > >> > >> http://marc.info/?l=linux-rdma&m=148656088729538&w=2 > >> > > > > Michael's patch doesn't change user_sdma_free_request() so it introduces > > a kfreeing an error pointer bug. > > Did you notice that another local variable “tmp” was introduced in the update step > “[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that > the mentioned function will usually get a null pointer after a failure there? > Ah right. Thanks. I missed that. regards, dan carpenter -- 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/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c index 991e7f3d8e18..5a73d738f2ba 100644 --- a/drivers/infiniband/hw/hfi1/user_sdma.c +++ b/drivers/infiniband/hw/hfi1/user_sdma.c @@ -731,23 +731,18 @@ int hfi1_user_sdma_process_request(struct file *fp, struct iovec *iovec, ret = -EINVAL; goto free_req; } - req->tids = kcalloc(ntids, sizeof(*req->tids), GFP_KERNEL); - if (!req->tids) { - ret = -ENOMEM; - goto free_req; - } /* * We have to copy all of the tids because they may vary * in size and, therefore, the TID count might not be * equal to the pkt count. However, there is no way to * tell at this point. */ - ret = copy_from_user(req->tids, iovec[idx].iov_base, - ntids * sizeof(*req->tids)); - if (ret) { + req->tids = memdup_user(iovec[idx].iov_base, + sizeof(*req->tids) * ntids); + if (IS_ERR(req->tids)) { + ret = PTR_ERR(req->tids); SDMA_DBG(req, "Failed to copy %d TIDs (%d)", ntids, ret); - ret = -EFAULT; goto free_req; } req->n_tids = ntids; @@ -1606,7 +1601,8 @@ static void user_sdma_free_request(struct user_sdma_request *req, bool unpin) atomic_dec(&node->refcount); } } - kfree(req->tids); + if (!IS_ERR(req->tids)) + kfree(req->tids); clear_bit(req->info.comp_idx, req->pq->req_in_use); }