| Message ID | 20181211132642.3027-5-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | rdma: various issues in rdma/pvrdma backend | expand |
On Tue, Dec 11, 2018 at 06:56:41PM +0530, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > create_cq and create_qp routines allocate ring object, but it's > not released in case of an error, leading to memory leakage. > > Reported-by: Li Qiang <liq3ea@163.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/rdma/vmw/pvrdma_cmd.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > index ee2888259c..e8d99f29fa 100644 > --- a/hw/rdma/vmw/pvrdma_cmd.c > +++ b/hw/rdma/vmw/pvrdma_cmd.c > @@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req, > > resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev, > cmd->cqe, &resp->cq_handle, ring); > - resp->cqe = cmd->cqe; > + if (resp->hdr.err) { > + g_free(ring); This is not enough since all ring's resources (ring state and ring's pages) left mapped. The steps needed are the steps detailed in destroy_cq. > + } > > out: > pr_dbg("ret=%d\n", resp->hdr.err); > @@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, > cmd->max_send_sge, cmd->send_cq_handle, > cmd->max_recv_wr, cmd->max_recv_sge, > cmd->recv_cq_handle, rings, &resp->qpn); > + if (resp->hdr.err) { > + g_free(rings); Ditto, here send rind and recv rings stays mapped. Look at how QP's ring is destroyed in destroy_qp. For both case suggesting to define a new static function that destroy rings and call it from both error flow of create_* and from destroy_* > + goto out; > + } > > resp->max_send_wr = cmd->max_send_wr; > resp->max_recv_wr = cmd->max_recv_wr; > -- > 2.19.2 >
On Tue, Dec 11, 2018 at 06:47:43PM +0200, Yuval Shaia wrote: > On Tue, Dec 11, 2018 at 06:56:41PM +0530, P J P wrote: > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > create_cq and create_qp routines allocate ring object, but it's > > not released in case of an error, leading to memory leakage. > > > > Reported-by: Li Qiang <liq3ea@163.com> > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > --- > > hw/rdma/vmw/pvrdma_cmd.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > > index ee2888259c..e8d99f29fa 100644 > > --- a/hw/rdma/vmw/pvrdma_cmd.c > > +++ b/hw/rdma/vmw/pvrdma_cmd.c > > @@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req, > > > > resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev, > > cmd->cqe, &resp->cq_handle, ring); > > - resp->cqe = cmd->cqe; > > + if (resp->hdr.err) { > > + g_free(ring); > > This is not enough since all ring's resources (ring state and ring's pages) > left mapped. > > The steps needed are the steps detailed in destroy_cq. > > > + } > > > > out: > > pr_dbg("ret=%d\n", resp->hdr.err); > > @@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, > > cmd->max_send_sge, cmd->send_cq_handle, > > cmd->max_recv_wr, cmd->max_recv_sge, > > cmd->recv_cq_handle, rings, &resp->qpn); > > + if (resp->hdr.err) { > > + g_free(rings); > > Ditto, here send rind and recv rings stays mapped. > Look at how QP's ring is destroyed in destroy_qp. > > For both case suggesting to define a new static function that destroy rings > and call it from both error flow of create_* and from destroy_* > > > + goto out; > > + } > > > > resp->max_send_wr = cmd->max_send_wr; > > resp->max_recv_wr = cmd->max_recv_wr; Also, can you rebase this patch on top of the patchset i posted last week: https://patchwork.kernel.org/patch/10705439/ Thanks, > > -- > > 2.19.2 > >
Hello Yuval, +-- On Tue, 11 Dec 2018, Yuval Shaia wrote --+ | > Ditto, here send rind and recv rings stays mapped. | > Look at how QP's ring is destroyed in destroy_qp. | > | > For both case suggesting to define a new static function that destroy rings | > and call it from both error flow of create_* and from destroy_* | > I see, okay. Similar resource clean-up required in pvrdma_realize(). In case of an error it does: 'goto out;', but nothing is free'd there. Is another destroy_ routine required there? | Also, can you rebase this patch on top of the patchset i posted last week: | https://patchwork.kernel.org/patch/10705439/ Okay, I'll send revised patch set. Thanks so much for the prompt review. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
+-- On Wed, 12 Dec 2018, P J P wrote --+
| | Also, can you rebase this patch on top of the patchset i posted last week:
| | https://patchwork.kernel.org/patch/10705439/
|
| Okay, I'll send revised patch set. Thanks so much for the prompt review.
I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But
it does not seem to apply cleanly.
===
$ git branch rdma v3.1.0-rc0
$ git checkout rdma
Switched to branch 'rdma'
$ git apply --check /tmp/add-support-for-RDMA-MAD.patch
error: patch failed: Makefile.objs:1
error: Makefile.objs: patch does not apply
error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
===
I skipped one of the makefile.objs/.json patch, applied others and then
hand-fixed one of the patch pvrdma_cmd.c.
After that when I try to build QEMU, it fails with
===
$ ./configure --prefix=/tmp/ --enable-debug
--target-list='x86_64-softmmu x86_64-linux-user'
--enable-rdma --with-sdlabi=2.0
$ make
../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function
‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration] qapi_event_send_rdma_gid_status_changed(ifname, true,
...
../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No
such file or directory
#include "qapi/qapi-events-rdma.h"
^~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
===
Not sure if other patches are required. Any idea?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Wed, Dec 12, 2018 at 03:09:19PM +0530, P J P wrote: > +-- On Wed, 12 Dec 2018, P J P wrote --+ > | | Also, can you rebase this patch on top of the patchset i posted last week: > | | https://patchwork.kernel.org/patch/10705439/ > | > | Okay, I'll send revised patch set. Thanks so much for the prompt review. > > I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But > it does not seem to apply cleanly. > > === > $ git branch rdma v3.1.0-rc0 > $ git checkout rdma > Switched to branch 'rdma' > $ git apply --check /tmp/add-support-for-RDMA-MAD.patch > error: patch failed: Makefile.objs:1 > error: Makefile.objs: patch does not apply > error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224 > error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply > === > > I skipped one of the makefile.objs/.json patch, applied others and then > hand-fixed one of the patch pvrdma_cmd.c. > > After that when I try to build QEMU, it fails with > > === > $ ./configure --prefix=/tmp/ --enable-debug > --target-list='x86_64-softmmu x86_64-linux-user' > --enable-rdma --with-sdlabi=2.0 > $ make > ../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function > ‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration] qapi_event_send_rdma_gid_status_changed(ifname, true, > ... > ../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No > such file or directory > #include "qapi/qapi-events-rdma.h" > ^~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > === > > Not sure if other patches are required. Any idea? Ok, you rebased your patches on latest upstream, lets deal with merge conflicts later. Marcel, any idea? Can Prasad take from your tree? Thanks, > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Wed, Dec 12, 2018 at 03:09:19PM +0530, P J P wrote: > +-- On Wed, 12 Dec 2018, P J P wrote --+ > | | Also, can you rebase this patch on top of the patchset i posted last week: > | | https://patchwork.kernel.org/patch/10705439/ > | > | Okay, I'll send revised patch set. Thanks so much for the prompt review. > > I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But > it does not seem to apply cleanly. > > === > $ git branch rdma v3.1.0-rc0 Can you try master? I just did a rebase on top of master and had no conflicts. > $ git checkout rdma > Switched to branch 'rdma' > $ git apply --check /tmp/add-support-for-RDMA-MAD.patch > error: patch failed: Makefile.objs:1 > error: Makefile.objs: patch does not apply > error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224 > error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply > === > > I skipped one of the makefile.objs/.json patch, applied others and then > hand-fixed one of the patch pvrdma_cmd.c. > > After that when I try to build QEMU, it fails with > > === > $ ./configure --prefix=/tmp/ --enable-debug > --target-list='x86_64-softmmu x86_64-linux-user' > --enable-rdma --with-sdlabi=2.0 > $ make > ../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function > ‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration] qapi_event_send_rdma_gid_status_changed(ifname, true, > ... > ../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No > such file or directory > #include "qapi/qapi-events-rdma.h" > ^~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > === > > Not sure if other patches are required. Any idea? > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
+-- On Wed, 12 Dec 2018, Yuval Shaia wrote --+ | Can you try master? | I just did a rebase on top of master and had no conflicts. Yes, I tried with master first, got the same errors, that's when I tried earlier versions. Preparing patch-set v2 with the suggested updates. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c index ee2888259c..e8d99f29fa 100644 --- a/hw/rdma/vmw/pvrdma_cmd.c +++ b/hw/rdma/vmw/pvrdma_cmd.c @@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req, resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev, cmd->cqe, &resp->cq_handle, ring); - resp->cqe = cmd->cqe; + if (resp->hdr.err) { + g_free(ring); + } out: pr_dbg("ret=%d\n", resp->hdr.err); @@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, cmd->max_send_sge, cmd->send_cq_handle, cmd->max_recv_wr, cmd->max_recv_sge, cmd->recv_cq_handle, rings, &resp->qpn); + if (resp->hdr.err) { + g_free(rings); + goto out; + } resp->max_send_wr = cmd->max_send_wr; resp->max_recv_wr = cmd->max_recv_wr;