* [RFC] RDMA/siw: Fix xa_mmap helper patch
@ 2019-09-02 14:18 Bernard Metzler
2019-09-02 14:56 ` [EXT] " Michal Kalderon
2019-09-02 15:22 ` Bernard Metzler
0 siblings, 2 replies; 3+ messages in thread
From: Bernard Metzler @ 2019-09-02 14:18 UTC (permalink / raw)
To: linux-rdma; +Cc: mkalderon, Bernard Metzler
- make the siw_user_mmap_entry.address a void *, which
naturally fits with remap_vmalloc_range. also avoids
other casting during resource address assignment.
- do not kfree SQ/RQ/CQ/SRQ in preparation of mmap.
Those resources are always further needed ;)
- Fix check for correct mmap range in siw_mmap().
- entry->length is the object length. We have to
expand to PAGE_ALIGN(entry->length), since mmap
comes with complete page(s) containing the
object.
- put mmap_entry if that check fails. Otherwise
entry object ref counting screws up, and later
crashes during context close.
- simplify siw_mmap_free() - it must just free
the entry.
---
drivers/infiniband/sw/siw/siw.h | 2 +-
drivers/infiniband/sw/siw/siw_verbs.c | 59 +++++++++------------------
2 files changed, 20 insertions(+), 41 deletions(-)
diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index d48cd42ae43e..d62f18f49ac5 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -505,7 +505,7 @@ struct iwarp_msg_info {
struct siw_user_mmap_entry {
struct rdma_user_mmap_entry rdma_entry;
- u64 address;
+ void *address;
u64 length;
};
diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 9e049241051e..24bdf5b508e6 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -36,10 +36,7 @@ static char ib_qp_state_to_string[IB_QPS_ERR + 1][sizeof("RESET")] = {
void siw_mmap_free(struct rdma_user_mmap_entry *rdma_entry)
{
- struct siw_user_mmap_entry *entry = to_siw_mmap_entry(rdma_entry);
-
- vfree((void *)entry->address);
- kfree(entry);
+ kfree(rdma_entry);
}
int siw_mmap(struct ib_ucontext *ctx, struct vm_area_struct *vma)
@@ -56,29 +53,28 @@ int siw_mmap(struct ib_ucontext *ctx, struct vm_area_struct *vma)
*/
if (vma->vm_start & (PAGE_SIZE - 1)) {
pr_warn("siw: mmap not page aligned\n");
- goto out;
+ return -EINVAL;
}
rdma_entry = rdma_user_mmap_entry_get(&uctx->base_ucontext, off,
size, vma);
if (!rdma_entry) {
siw_dbg(&uctx->sdev->base_dev, "mmap lookup failed: %lu, %u\n",
off, size);
- goto out;
+ return -EINVAL;
}
entry = to_siw_mmap_entry(rdma_entry);
- if (entry->length != size) {
+ if (PAGE_ALIGN(entry->length) != size) {
siw_dbg(&uctx->sdev->base_dev,
"key[%#lx] does not have valid length[%#x] expected[%#llx]\n",
off, size, entry->length);
+ rdma_user_mmap_entry_put(&uctx->base_ucontext, rdma_entry);
return -EINVAL;
}
-
- rv = remap_vmalloc_range(vma, (void *)entry->address, 0);
+ rv = remap_vmalloc_range(vma, entry->address, 0);
if (rv) {
pr_warn("remap_vmalloc_range failed: %lu, %u\n", off, size);
rdma_user_mmap_entry_put(&uctx->base_ucontext, rdma_entry);
}
-out:
return rv;
}
@@ -270,7 +266,7 @@ void siw_qp_put_ref(struct ib_qp *base_qp)
}
static int siw_user_mmap_entry_insert(struct ib_ucontext *ucontext,
- u64 address, u64 length,
+ void *address, u64 length,
u64 *key)
{
struct siw_user_mmap_entry *entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -461,30 +457,22 @@ struct ib_qp *siw_create_qp(struct ib_pd *pd,
if (qp->sendq) {
length = num_sqe * sizeof(struct siw_sqe);
rv = siw_user_mmap_entry_insert(&uctx->base_ucontext,
- (uintptr_t)qp->sendq,
- length, &key);
- if (!rv)
+ qp->sendq, length,
+ &key);
+ if (rv)
goto err_out_xa;
- /* If entry was inserted successfully, qp->sendq
- * will be freed by siw_mmap_free
- */
- qp->sendq = NULL;
qp->sq_key = key;
}
if (qp->recvq) {
length = num_rqe * sizeof(struct siw_rqe);
rv = siw_user_mmap_entry_insert(&uctx->base_ucontext,
- (uintptr_t)qp->recvq,
- length, &key);
- if (!rv)
+ qp->recvq, length,
+ &key);
+ if (rv)
goto err_out_mmap_rem;
- /* If entry was inserted successfully, qp->recvq will
- * be freed by siw_mmap_free
- */
- qp->recvq = NULL;
qp->rq_key = key;
}
@@ -1078,16 +1066,11 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr,
sizeof(struct siw_cq_ctrl);
rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
- (uintptr_t)cq->queue,
- length, &cq->cq_key);
- if (!rv)
+ cq->queue, length,
+ &cq->cq_key);
+ if (rv)
goto err_out;
- /* If entry was inserted successfully, cq->queue will be freed
- * by siw_mmap_free
- */
- cq->queue = NULL;
-
uresp.cq_key = cq->cq_key;
uresp.cq_id = cq->id;
uresp.num_cqe = size;
@@ -1535,15 +1518,11 @@ int siw_create_srq(struct ib_srq *base_srq,
u64 length = srq->num_rqe * sizeof(struct siw_rqe);
rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
- (uintptr_t)srq->recvq,
- length, &srq->srq_key);
- if (!rv)
+ srq->recvq, length,
+ &srq->srq_key);
+ if (rv)
goto err_out;
- /* If entry was inserted successfully, srq->recvq will be freed
- * by siw_mmap_free
- */
- srq->recvq = NULL;
uresp.srq_key = srq->srq_key;
uresp.num_rqe = srq->num_rqe;
--
2.17.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [EXT] [RFC] RDMA/siw: Fix xa_mmap helper patch
2019-09-02 14:18 [RFC] RDMA/siw: Fix xa_mmap helper patch Bernard Metzler
@ 2019-09-02 14:56 ` Michal Kalderon
2019-09-02 15:22 ` Bernard Metzler
1 sibling, 0 replies; 3+ messages in thread
From: Michal Kalderon @ 2019-09-02 14:56 UTC (permalink / raw)
To: Bernard Metzler, linux-rdma
> From: Bernard Metzler <bmt@zurich.ibm.com>
> Sent: Monday, September 2, 2019 5:19 PM
> External Email
>
> ----------------------------------------------------------------------
> - make the siw_user_mmap_entry.address a void *, which
> naturally fits with remap_vmalloc_range. also avoids
> other casting during resource address assignment.
>
> - do not kfree SQ/RQ/CQ/SRQ in preparation of mmap.
> Those resources are always further needed ;)
>
> - Fix check for correct mmap range in siw_mmap().
> - entry->length is the object length. We have to
> expand to PAGE_ALIGN(entry->length), since mmap
> comes with complete page(s) containing the
> object.
> - put mmap_entry if that check fails. Otherwise
> entry object ref counting screws up, and later
> crashes during context close.
>
> - simplify siw_mmap_free() - it must just free
> the entry.
Thanks Bernard, I will incorporate this patch into the series, with some minor changes
please see
Some comments below
Do your tests pass with this patch below ?
- I also added back the places that free the memory no longer freed in mmap_free
And also added checks on the key on places that remove them to be closer to original
Code. I have changed the length to size_t
Will send the v9 later on today.
Thanks for your help
Michal
> ---
> drivers/infiniband/sw/siw/siw.h | 2 +-
> drivers/infiniband/sw/siw/siw_verbs.c | 59 +++++++++------------------
> 2 files changed, 20 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw.h
> b/drivers/infiniband/sw/siw/siw.h index d48cd42ae43e..d62f18f49ac5 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -505,7 +505,7 @@ struct iwarp_msg_info {
>
> struct siw_user_mmap_entry {
> struct rdma_user_mmap_entry rdma_entry;
> - u64 address;
> + void *address;
> u64 length;
> };
>
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> b/drivers/infiniband/sw/siw/siw_verbs.c
> index 9e049241051e..24bdf5b508e6 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -36,10 +36,7 @@ static char ib_qp_state_to_string[IB_QPS_ERR +
> 1][sizeof("RESET")] = {
>
> void siw_mmap_free(struct rdma_user_mmap_entry *rdma_entry) {
> - struct siw_user_mmap_entry *entry =
> to_siw_mmap_entry(rdma_entry);
> -
> - vfree((void *)entry->address);
> - kfree(entry);
> + kfree(rdma_entry);
I think it is better practice to free the entry and not rdma_entry for the same reason
We use container_of and not just cast.
> }
>
> int siw_mmap(struct ib_ucontext *ctx, struct vm_area_struct *vma) @@ -
> 56,29 +53,28 @@ int siw_mmap(struct ib_ucontext *ctx, struct
> vm_area_struct *vma)
> */
> if (vma->vm_start & (PAGE_SIZE - 1)) {
> pr_warn("siw: mmap not page aligned\n");
> - goto out;
> + return -EINVAL;
> }
> rdma_entry = rdma_user_mmap_entry_get(&uctx->base_ucontext,
> off,
> size, vma);
> if (!rdma_entry) {
> siw_dbg(&uctx->sdev->base_dev, "mmap lookup failed: %lu,
> %u\n",
> off, size);
> - goto out;
> + return -EINVAL;
> }
> entry = to_siw_mmap_entry(rdma_entry);
> - if (entry->length != size) {
> + if (PAGE_ALIGN(entry->length) != size) {
> siw_dbg(&uctx->sdev->base_dev,
> "key[%#lx] does not have valid length[%#x]
> expected[%#llx]\n",
> off, size, entry->length);
> + rdma_user_mmap_entry_put(&uctx->base_ucontext,
> rdma_entry);
> return -EINVAL;
> }
> -
> - rv = remap_vmalloc_range(vma, (void *)entry->address, 0);
> + rv = remap_vmalloc_range(vma, entry->address, 0);
> if (rv) {
> pr_warn("remap_vmalloc_range failed: %lu, %u\n", off,
> size);
> rdma_user_mmap_entry_put(&uctx->base_ucontext,
> rdma_entry);
> }
> -out:
> return rv;
> }
>
> @@ -270,7 +266,7 @@ void siw_qp_put_ref(struct ib_qp *base_qp) }
>
> static int siw_user_mmap_entry_insert(struct ib_ucontext *ucontext,
> - u64 address, u64 length,
> + void *address, u64 length,
> u64 *key)
> {
> struct siw_user_mmap_entry *entry = kzalloc(sizeof(*entry),
> GFP_KERNEL); @@ -461,30 +457,22 @@ struct ib_qp *siw_create_qp(struct
> ib_pd *pd,
> if (qp->sendq) {
> length = num_sqe * sizeof(struct siw_sqe);
> rv = siw_user_mmap_entry_insert(&uctx-
> >base_ucontext,
> - (uintptr_t)qp-
> >sendq,
> - length, &key);
> - if (!rv)
> + qp->sendq, length,
> + &key);
> + if (rv)
> goto err_out_xa;
>
> - /* If entry was inserted successfully, qp->sendq
> - * will be freed by siw_mmap_free
> - */
> - qp->sendq = NULL;
> qp->sq_key = key;
> }
>
> if (qp->recvq) {
> length = num_rqe * sizeof(struct siw_rqe);
> rv = siw_user_mmap_entry_insert(&uctx-
> >base_ucontext,
> - (uintptr_t)qp->recvq,
> - length, &key);
> - if (!rv)
> + qp->recvq, length,
> + &key);
> + if (rv)
> goto err_out_mmap_rem;
>
> - /* If entry was inserted successfully, qp->recvq will
> - * be freed by siw_mmap_free
> - */
> - qp->recvq = NULL;
> qp->rq_key = key;
> }
>
> @@ -1078,16 +1066,11 @@ int siw_create_cq(struct ib_cq *base_cq, const
> struct ib_cq_init_attr *attr,
> sizeof(struct siw_cq_ctrl);
>
> rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
> - (uintptr_t)cq->queue,
> - length, &cq->cq_key);
> - if (!rv)
> + cq->queue, length,
> + &cq->cq_key);
> + if (rv)
> goto err_out;
>
> - /* If entry was inserted successfully, cq->queue will be freed
> - * by siw_mmap_free
> - */
> - cq->queue = NULL;
> -
> uresp.cq_key = cq->cq_key;
> uresp.cq_id = cq->id;
> uresp.num_cqe = size;
> @@ -1535,15 +1518,11 @@ int siw_create_srq(struct ib_srq *base_srq,
> u64 length = srq->num_rqe * sizeof(struct siw_rqe);
>
> rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
> - (uintptr_t)srq->recvq,
> - length, &srq->srq_key);
> - if (!rv)
> + srq->recvq, length,
> + &srq->srq_key);
> + if (rv)
> goto err_out;
>
> - /* If entry was inserted successfully, srq->recvq will be freed
> - * by siw_mmap_free
> - */
> - srq->recvq = NULL;
> uresp.srq_key = srq->srq_key;
> uresp.num_rqe = srq->num_rqe;
>
> --
> 2.17.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: RE: [EXT] [RFC] RDMA/siw: Fix xa_mmap helper patch
2019-09-02 14:18 [RFC] RDMA/siw: Fix xa_mmap helper patch Bernard Metzler
2019-09-02 14:56 ` [EXT] " Michal Kalderon
@ 2019-09-02 15:22 ` Bernard Metzler
1 sibling, 0 replies; 3+ messages in thread
From: Bernard Metzler @ 2019-09-02 15:22 UTC (permalink / raw)
To: Michal Kalderon; +Cc: linux-rdma
-----"Michal Kalderon" <mkalderon@marvell.com> wrote: -----
>To: "Bernard Metzler" <bmt@zurich.ibm.com>,
>"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
>From: "Michal Kalderon" <mkalderon@marvell.com>
>Date: 09/02/2019 04:56PM
>Subject: [EXTERNAL] RE: [EXT] [RFC] RDMA/siw: Fix xa_mmap helper
>patch
>
>> From: Bernard Metzler <bmt@zurich.ibm.com>
>> Sent: Monday, September 2, 2019 5:19 PM
>> External Email
>>
>>
>---------------------------------------------------------------------
>-
>> - make the siw_user_mmap_entry.address a void *, which
>> naturally fits with remap_vmalloc_range. also avoids
>> other casting during resource address assignment.
>>
>> - do not kfree SQ/RQ/CQ/SRQ in preparation of mmap.
>> Those resources are always further needed ;)
>>
>> - Fix check for correct mmap range in siw_mmap().
>> - entry->length is the object length. We have to
>> expand to PAGE_ALIGN(entry->length), since mmap
>> comes with complete page(s) containing the
>> object.
>> - put mmap_entry if that check fails. Otherwise
>> entry object ref counting screws up, and later
>> crashes during context close.
>>
>> - simplify siw_mmap_free() - it must just free
>> the entry.
>Thanks Bernard, I will incorporate this patch into the series, with
>some minor changes
>please see
>Some comments below
>Do your tests pass with this patch below ?
>
>- I also added back the places that free the memory no longer freed
>in mmap_free
>And also added checks on the key on places that remove them to be
>closer to original
>Code. I have changed the length to size_t
>
>Will send the v9 later on today.
>
Sounds good!
The patch I sent worked on top of yours for
me. Even better if you fixed a new memory leak! ;)
Thanks!
Bernard.
>Thanks for your help
>Michal
>
>> ---
>> drivers/infiniband/sw/siw/siw.h | 2 +-
>> drivers/infiniband/sw/siw/siw_verbs.c | 59
>+++++++++------------------
>> 2 files changed, 20 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw.h
>> b/drivers/infiniband/sw/siw/siw.h index d48cd42ae43e..d62f18f49ac5
>100644
>> --- a/drivers/infiniband/sw/siw/siw.h
>> +++ b/drivers/infiniband/sw/siw/siw.h
>> @@ -505,7 +505,7 @@ struct iwarp_msg_info {
>>
>> struct siw_user_mmap_entry {
>> struct rdma_user_mmap_entry rdma_entry;
>> - u64 address;
>> + void *address;
>> u64 length;
>> };
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>> b/drivers/infiniband/sw/siw/siw_verbs.c
>> index 9e049241051e..24bdf5b508e6 100644
>> --- a/drivers/infiniband/sw/siw/siw_verbs.c
>> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
>> @@ -36,10 +36,7 @@ static char ib_qp_state_to_string[IB_QPS_ERR +
>> 1][sizeof("RESET")] = {
>>
>> void siw_mmap_free(struct rdma_user_mmap_entry *rdma_entry) {
>> - struct siw_user_mmap_entry *entry =
>> to_siw_mmap_entry(rdma_entry);
>> -
>> - vfree((void *)entry->address);
>> - kfree(entry);
>> + kfree(rdma_entry);
>I think it is better practice to free the entry and not rdma_entry
>for the same reason
>We use container_of and not just cast.
>> }
>>
>> int siw_mmap(struct ib_ucontext *ctx, struct vm_area_struct *vma)
>@@ -
>> 56,29 +53,28 @@ int siw_mmap(struct ib_ucontext *ctx, struct
>> vm_area_struct *vma)
>> */
>> if (vma->vm_start & (PAGE_SIZE - 1)) {
>> pr_warn("siw: mmap not page aligned\n");
>> - goto out;
>> + return -EINVAL;
>> }
>> rdma_entry = rdma_user_mmap_entry_get(&uctx->base_ucontext,
>> off,
>> size, vma);
>> if (!rdma_entry) {
>> siw_dbg(&uctx->sdev->base_dev, "mmap lookup failed: %lu,
>> %u\n",
>> off, size);
>> - goto out;
>> + return -EINVAL;
>> }
>> entry = to_siw_mmap_entry(rdma_entry);
>> - if (entry->length != size) {
>> + if (PAGE_ALIGN(entry->length) != size) {
>> siw_dbg(&uctx->sdev->base_dev,
>> "key[%#lx] does not have valid length[%#x]
>> expected[%#llx]\n",
>> off, size, entry->length);
>> + rdma_user_mmap_entry_put(&uctx->base_ucontext,
>> rdma_entry);
>> return -EINVAL;
>> }
>> -
>> - rv = remap_vmalloc_range(vma, (void *)entry->address, 0);
>> + rv = remap_vmalloc_range(vma, entry->address, 0);
>> if (rv) {
>> pr_warn("remap_vmalloc_range failed: %lu, %u\n", off,
>> size);
>> rdma_user_mmap_entry_put(&uctx->base_ucontext,
>> rdma_entry);
>> }
>> -out:
>> return rv;
>> }
>>
>> @@ -270,7 +266,7 @@ void siw_qp_put_ref(struct ib_qp *base_qp) }
>>
>> static int siw_user_mmap_entry_insert(struct ib_ucontext
>*ucontext,
>> - u64 address, u64 length,
>> + void *address, u64 length,
>> u64 *key)
>> {
>> struct siw_user_mmap_entry *entry = kzalloc(sizeof(*entry),
>> GFP_KERNEL); @@ -461,30 +457,22 @@ struct ib_qp
>*siw_create_qp(struct
>> ib_pd *pd,
>> if (qp->sendq) {
>> length = num_sqe * sizeof(struct siw_sqe);
>> rv = siw_user_mmap_entry_insert(&uctx-
>> >base_ucontext,
>> - (uintptr_t)qp-
>> >sendq,
>> - length, &key);
>> - if (!rv)
>> + qp->sendq, length,
>> + &key);
>> + if (rv)
>> goto err_out_xa;
>>
>> - /* If entry was inserted successfully, qp->sendq
>> - * will be freed by siw_mmap_free
>> - */
>> - qp->sendq = NULL;
>> qp->sq_key = key;
>> }
>>
>> if (qp->recvq) {
>> length = num_rqe * sizeof(struct siw_rqe);
>> rv = siw_user_mmap_entry_insert(&uctx-
>> >base_ucontext,
>> - (uintptr_t)qp->recvq,
>> - length, &key);
>> - if (!rv)
>> + qp->recvq, length,
>> + &key);
>> + if (rv)
>> goto err_out_mmap_rem;
>>
>> - /* If entry was inserted successfully, qp->recvq will
>> - * be freed by siw_mmap_free
>> - */
>> - qp->recvq = NULL;
>> qp->rq_key = key;
>> }
>>
>> @@ -1078,16 +1066,11 @@ int siw_create_cq(struct ib_cq *base_cq,
>const
>> struct ib_cq_init_attr *attr,
>> sizeof(struct siw_cq_ctrl);
>>
>> rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
>> - (uintptr_t)cq->queue,
>> - length, &cq->cq_key);
>> - if (!rv)
>> + cq->queue, length,
>> + &cq->cq_key);
>> + if (rv)
>> goto err_out;
>>
>> - /* If entry was inserted successfully, cq->queue will be freed
>> - * by siw_mmap_free
>> - */
>> - cq->queue = NULL;
>> -
>> uresp.cq_key = cq->cq_key;
>> uresp.cq_id = cq->id;
>> uresp.num_cqe = size;
>> @@ -1535,15 +1518,11 @@ int siw_create_srq(struct ib_srq *base_srq,
>> u64 length = srq->num_rqe * sizeof(struct siw_rqe);
>>
>> rv = siw_user_mmap_entry_insert(&ctx->base_ucontext,
>> - (uintptr_t)srq->recvq,
>> - length, &srq->srq_key);
>> - if (!rv)
>> + srq->recvq, length,
>> + &srq->srq_key);
>> + if (rv)
>> goto err_out;
>>
>> - /* If entry was inserted successfully, srq->recvq will be freed
>> - * by siw_mmap_free
>> - */
>> - srq->recvq = NULL;
>> uresp.srq_key = srq->srq_key;
>> uresp.num_rqe = srq->num_rqe;
>>
>> --
>> 2.17.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-02 15:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 14:18 [RFC] RDMA/siw: Fix xa_mmap helper patch Bernard Metzler
2019-09-02 14:56 ` [EXT] " Michal Kalderon
2019-09-02 15:22 ` Bernard Metzler
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).