| Message ID | 20190114141006.1841-1-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/rdma: Verify that ptr is not NULL before freeing | expand |
Hi Yuval, On 1/14/19 3:10 PM, Yuval Shaia wrote: > Make sure objects are not NULL before calling to non-nulll-safe null > destructors. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > hw/rdma/rdma_backend.c | 6 ++++-- > hw/rdma/rdma_rm.c | 7 ++++++- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c > index b49edaacaf..3ee5172c96 100644 > --- a/hw/rdma/rdma_backend.c > +++ b/hw/rdma/rdma_backend.c > @@ -1066,8 +1066,10 @@ static void mad_fini(RdmaBackendDev *backend_dev) > pr_dbg("Stopping MAD\n"); > disable_rdmacm_mux_async(backend_dev); > qemu_chr_fe_disconnect(backend_dev->rdmacm_mux.chr_be); > - qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); > - qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); > + if (backend_dev->recv_mads_list.list) { This is only reachable when the backend didn't success at initializing, right? > + qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); > + qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); > + } > } > > int rdma_backend_get_gid_index(RdmaBackendDev *backend_dev, > diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c > index f5b1295890..8bf241e91f 100644 > --- a/hw/rdma/rdma_rm.c > +++ b/hw/rdma/rdma_rm.c > @@ -41,6 +41,9 @@ static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl, > > static inline void res_tbl_free(RdmaRmResTbl *tbl) > { > + if (!tbl->bitmap) { > + return; > + } > qemu_mutex_destroy(&tbl->lock); > g_free(tbl->tbl); > g_free(tbl->bitmap); > @@ -655,5 +658,7 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev, > res_tbl_free(&dev_res->cq_tbl); > res_tbl_free(&dev_res->pd_tbl); > > - g_hash_table_destroy(dev_res->qp_hash); > + if (dev_res->qp_hash) { > + g_hash_table_destroy(dev_res->qp_hash); > + } > } >
On Mon, Jan 14, 2019 at 04:11:22PM +0100, Philippe Mathieu-Daudé wrote: > Hi Yuval, > > On 1/14/19 3:10 PM, Yuval Shaia wrote: > > Make sure objects are not NULL before calling to non-nulll-safe > > null will fix > > > destructors. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > --- > > hw/rdma/rdma_backend.c | 6 ++++-- > > hw/rdma/rdma_rm.c | 7 ++++++- > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c > > index b49edaacaf..3ee5172c96 100644 > > --- a/hw/rdma/rdma_backend.c > > +++ b/hw/rdma/rdma_backend.c > > @@ -1066,8 +1066,10 @@ static void mad_fini(RdmaBackendDev *backend_dev) > > pr_dbg("Stopping MAD\n"); > > disable_rdmacm_mux_async(backend_dev); > > qemu_chr_fe_disconnect(backend_dev->rdmacm_mux.chr_be); > > - qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); > > - qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); > > + if (backend_dev->recv_mads_list.list) { > > This is only reachable when the backend didn't success at initializing, > right? Yes, just to cover the case where fini() was called even when init() fails. > > > + qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); > > + qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); > > + } > > } > > > > int rdma_backend_get_gid_index(RdmaBackendDev *backend_dev, > > diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c > > index f5b1295890..8bf241e91f 100644 > > --- a/hw/rdma/rdma_rm.c > > +++ b/hw/rdma/rdma_rm.c > > @@ -41,6 +41,9 @@ static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl, > > > > static inline void res_tbl_free(RdmaRmResTbl *tbl) > > { > > + if (!tbl->bitmap) { > > + return; > > + } > > qemu_mutex_destroy(&tbl->lock); > > g_free(tbl->tbl); > > g_free(tbl->bitmap); > > @@ -655,5 +658,7 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev, > > res_tbl_free(&dev_res->cq_tbl); > > res_tbl_free(&dev_res->pd_tbl); > > > > - g_hash_table_destroy(dev_res->qp_hash); > > + if (dev_res->qp_hash) { > > + g_hash_table_destroy(dev_res->qp_hash); > > + } > > } > >
On 1/16/19 11:03 AM, Yuval Shaia wrote: > On Mon, Jan 14, 2019 at 04:11:22PM +0100, Philippe Mathieu-Daudé wrote: >> Hi Yuval, >> >> On 1/14/19 3:10 PM, Yuval Shaia wrote: >>> Make sure objects are not NULL before calling to non-nulll-safe >> >> null > > will fix > >> >>> destructors. >>> >>> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> >>> --- >>> hw/rdma/rdma_backend.c | 6 ++++-- >>> hw/rdma/rdma_rm.c | 7 ++++++- >>> 2 files changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c >>> index b49edaacaf..3ee5172c96 100644 >>> --- a/hw/rdma/rdma_backend.c >>> +++ b/hw/rdma/rdma_backend.c >>> @@ -1066,8 +1066,10 @@ static void mad_fini(RdmaBackendDev *backend_dev) >>> pr_dbg("Stopping MAD\n"); >>> disable_rdmacm_mux_async(backend_dev); >>> qemu_chr_fe_disconnect(backend_dev->rdmacm_mux.chr_be); >>> - qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); >>> - qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); >>> + if (backend_dev->recv_mads_list.list) { >> >> This is only reachable when the backend didn't success at initializing, >> right? > > Yes, just to cover the case where fini() was called even when init() fails. OK. Can you add a line about this in the commit description please? With the comment updated: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> >>> + qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); >>> + qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); >>> + } >>> } >>> >>> int rdma_backend_get_gid_index(RdmaBackendDev *backend_dev, >>> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c >>> index f5b1295890..8bf241e91f 100644 >>> --- a/hw/rdma/rdma_rm.c >>> +++ b/hw/rdma/rdma_rm.c >>> @@ -41,6 +41,9 @@ static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl, >>> >>> static inline void res_tbl_free(RdmaRmResTbl *tbl) >>> { >>> + if (!tbl->bitmap) { >>> + return; >>> + } >>> qemu_mutex_destroy(&tbl->lock); >>> g_free(tbl->tbl); >>> g_free(tbl->bitmap); >>> @@ -655,5 +658,7 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev, >>> res_tbl_free(&dev_res->cq_tbl); >>> res_tbl_free(&dev_res->pd_tbl); >>> >>> - g_hash_table_destroy(dev_res->qp_hash); >>> + if (dev_res->qp_hash) { >>> + g_hash_table_destroy(dev_res->qp_hash); >>> + } >>> } >>>
diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c index b49edaacaf..3ee5172c96 100644 --- a/hw/rdma/rdma_backend.c +++ b/hw/rdma/rdma_backend.c @@ -1066,8 +1066,10 @@ static void mad_fini(RdmaBackendDev *backend_dev) pr_dbg("Stopping MAD\n"); disable_rdmacm_mux_async(backend_dev); qemu_chr_fe_disconnect(backend_dev->rdmacm_mux.chr_be); - qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); - qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); + if (backend_dev->recv_mads_list.list) { + qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list)); + qemu_mutex_destroy(&backend_dev->recv_mads_list.lock); + } } int rdma_backend_get_gid_index(RdmaBackendDev *backend_dev, diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c index f5b1295890..8bf241e91f 100644 --- a/hw/rdma/rdma_rm.c +++ b/hw/rdma/rdma_rm.c @@ -41,6 +41,9 @@ static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl, static inline void res_tbl_free(RdmaRmResTbl *tbl) { + if (!tbl->bitmap) { + return; + } qemu_mutex_destroy(&tbl->lock); g_free(tbl->tbl); g_free(tbl->bitmap); @@ -655,5 +658,7 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev, res_tbl_free(&dev_res->cq_tbl); res_tbl_free(&dev_res->pd_tbl); - g_hash_table_destroy(dev_res->qp_hash); + if (dev_res->qp_hash) { + g_hash_table_destroy(dev_res->qp_hash); + } }
Make sure objects are not NULL before calling to non-nulll-safe destructors. Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- hw/rdma/rdma_backend.c | 6 ++++-- hw/rdma/rdma_rm.c | 7 ++++++- 2 files changed, 10 insertions(+), 3 deletions(-)