Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753440AbdLSTmK (ORCPT ); Tue, 19 Dec 2017 14:42:10 -0500 Received: from mga01.intel.com ([192.55.52.88]:17647 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753241AbdLSTg7 (ORCPT ); Tue, 19 Dec 2017 14:36:59 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,428,1508828400"; d="scan'208";a="4018629" From: Dongwon Kim To: linux-kernel@vger.kernel.org Cc: dri-devel@lists.freedesktop.org, xen-devel@lists.xenproject.org, mateuszx.potrola@intel.com, dongwon.kim@intel.com Subject: [RFC PATCH 43/60] hyper_dmabuf: fixes on memory leaks in various places Date: Tue, 19 Dec 2017 11:29:59 -0800 Message-Id: <1513711816-2618-43-git-send-email-dongwon.kim@intel.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1513711816-2618-1-git-send-email-dongwon.kim@intel.com> References: <1513711816-2618-1-git-send-email-dongwon.kim@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8680 Lines: 289 Make sure to free buffers before returning to prevent memory leaks Signed-off-by: Dongwon Kim --- drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c | 19 +++++++- drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c | 9 +++- drivers/xen/hyper_dmabuf/hyper_dmabuf_ops.c | 6 ++- drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c | 4 +- .../xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c | 52 +++++++++++++++++++--- 5 files changed, 78 insertions(+), 12 deletions(-) diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c index 283fe5a..3215003 100644 --- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c +++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c @@ -282,6 +282,7 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data) /* free msg */ kfree(req); + /* free page_info */ kfree(page_info->pages); kfree(page_info); @@ -298,6 +299,10 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data) fail_map_req: hyper_dmabuf_remove_exported(sgt_info->hid); + /* free page_info */ + kfree(page_info->pages); + kfree(page_info); + fail_export: kfree(sgt_info->va_vmapped); @@ -433,6 +438,13 @@ static int hyper_dmabuf_export_fd_ioctl(struct file *filp, void *data) sgt_info->num_importers--; req = kcalloc(1, sizeof(*req), GFP_KERNEL); + + if (!req) { + dev_err(hyper_dmabuf_private.device, + "No more space left\n"); + return -ENOMEM; + } + hyper_dmabuf_create_request(req, HYPER_DMABUF_EXPORT_FD_FAILED, &operands[0]); ops->send_req(HYPER_DMABUF_DOM_ID(sgt_info->hid), req, false); kfree(req); @@ -681,16 +693,19 @@ long hyper_dmabuf_ioctl(struct file *filp, if (copy_from_user(kdata, (void __user *)param, _IOC_SIZE(cmd)) != 0) { dev_err(hyper_dmabuf_private.device, "failed to copy from user arguments\n"); - return -EFAULT; + ret = -EFAULT; + goto ioctl_error; } ret = func(filp, kdata); if (copy_to_user((void __user *)param, kdata, _IOC_SIZE(cmd)) != 0) { dev_err(hyper_dmabuf_private.device, "failed to copy to user arguments\n"); - return -EFAULT; + ret = -EFAULT; + goto ioctl_error; } +ioctl_error: kfree(kdata); return ret; diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c index c516df8..46cf9a4 100644 --- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c +++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c @@ -191,8 +191,7 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_req *req) struct hyper_dmabuf_req *temp_req; struct hyper_dmabuf_imported_sgt_info *sgt_info; struct hyper_dmabuf_sgt_info *exp_sgt_info; - hyper_dmabuf_id_t hid = {req->operands[0], /* hid.id */ - {req->operands[1], req->operands[2], req->operands[3]}}; /* hid.rng_key */ + hyper_dmabuf_id_t hid; int ret; if (!req) { @@ -200,6 +199,11 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_req *req) return -EINVAL; } + hid.id = req->operands[0]; + hid.rng_key[0] = req->operands[1]; + hid.rng_key[1] = req->operands[2]; + hid.rng_key[2] = req->operands[3]; + if ((req->command < HYPER_DMABUF_EXPORT) || (req->command > HYPER_DMABUF_OPS_TO_SOURCE)) { dev_err(hyper_dmabuf_private.device, "invalid command\n"); @@ -332,6 +336,7 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_req *req) if (!proc) { dev_err(hyper_dmabuf_private.device, "No memory left to be allocated\n"); + kfree(temp_req); return -ENOMEM; } diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ops.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ops.c index 81cb09f..9313c42 100644 --- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ops.c +++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ops.c @@ -148,9 +148,8 @@ static struct sg_table* hyper_dmabuf_ops_map(struct dma_buf_attachment *attachme if (!st) goto err_free_sg; - if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) { + if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) goto err_free_sg; - } ret = hyper_dmabuf_sync_request(sgt_info->hid, HYPER_DMABUF_OPS_MAP); @@ -171,6 +170,9 @@ static struct sg_table* hyper_dmabuf_ops_map(struct dma_buf_attachment *attachme kfree(st); } + kfree(page_info->pages); + kfree(page_info); + return NULL; } diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c index c2d013a..dd17d26 100644 --- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c +++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_sgl_proc.c @@ -89,8 +89,10 @@ struct hyper_dmabuf_pages_info *hyper_dmabuf_ext_pgs(struct sg_table *sgt) return NULL; pinfo->pages = kmalloc(sizeof(struct page *)*hyper_dmabuf_get_num_pgs(sgt), GFP_KERNEL); - if (!pinfo->pages) + if (!pinfo->pages) { + kfree(pinfo); return NULL; + } sgl = sgt->sgl; diff --git a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c index 43dd3b6..9689346 100644 --- a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c +++ b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c @@ -229,9 +229,16 @@ int hyper_dmabuf_xen_init_tx_rbuf(int domid) ring_info = kmalloc(sizeof(*ring_info), GFP_KERNEL); + if (!ring_info) { + dev_err(hyper_dmabuf_private.device, + "No more spae left\n"); + return -ENOMEM; + } + /* from exporter to importer */ shared_ring = (void *)__get_free_pages(GFP_KERNEL, 1); if (shared_ring == 0) { + kfree(ring_info); return -ENOMEM; } @@ -246,6 +253,7 @@ int hyper_dmabuf_xen_init_tx_rbuf(int domid) 0); if (ring_info->gref_ring < 0) { /* fail to get gref */ + kfree(ring_info); return -EFAULT; } @@ -256,6 +264,7 @@ int hyper_dmabuf_xen_init_tx_rbuf(int domid) if (ret) { dev_err(hyper_dmabuf_private.device, "Cannot allocate event channel\n"); + kfree(ring_info); return -EIO; } @@ -271,6 +280,7 @@ int hyper_dmabuf_xen_init_tx_rbuf(int domid) HYPERVISOR_event_channel_op(EVTCHNOP_close, &close); gnttab_end_foreign_access(ring_info->gref_ring, 0, virt_to_mfn(shared_ring)); + kfree(ring_info); return -EIO; } @@ -299,6 +309,14 @@ int hyper_dmabuf_xen_init_tx_rbuf(int domid) */ ring_info->watch.callback = remote_dom_exporter_watch_cb; ring_info->watch.node = (const char*) kmalloc(sizeof(char) * 255, GFP_KERNEL); + + if (!ring_info->watch.node) { + dev_err(hyper_dmabuf_private.device, + "No more space left\n"); + kfree(ring_info); + return -ENOMEM; + } + sprintf((char*)ring_info->watch.node, "/local/domain/%d/data/hyper_dmabuf/%d/port", domid, hyper_dmabuf_xen_get_domid()); @@ -392,8 +410,16 @@ int hyper_dmabuf_xen_init_rx_rbuf(int domid) map_ops = kmalloc(sizeof(*map_ops), GFP_KERNEL); + if (!map_ops) { + dev_err(hyper_dmabuf_private.device, + "No memory left to be allocated\n"); + ret = -ENOMEM; + goto fail_no_map_ops; + } + if (gnttab_alloc_pages(1, &shared_ring)) { - return -ENOMEM; + ret = -ENOMEM; + goto fail_others; } gnttab_set_map_op(&map_ops[0], (unsigned long)pfn_to_kaddr(page_to_pfn(shared_ring)), @@ -405,12 +431,14 @@ int hyper_dmabuf_xen_init_rx_rbuf(int domid) ret = gnttab_map_refs(map_ops, NULL, &shared_ring, 1); if (ret < 0) { dev_err(hyper_dmabuf_private.device, "Cannot map ring\n"); - return -EFAULT; + ret = -EFAULT; + goto fail_others; } if (map_ops[0].status) { dev_err(hyper_dmabuf_private.device, "Ring mapping failed\n"); - return -EFAULT; + ret = -EFAULT; + goto fail_others; } else { ring_info->unmap_op.handle = map_ops[0].handle; } @@ -424,7 +452,8 @@ int hyper_dmabuf_xen_init_rx_rbuf(int domid) ret = bind_interdomain_evtchn_to_irq(domid, rx_port); if (ret < 0) { - return -EIO; + ret = -EIO; + goto fail_others; } ring_info->irq = ret; @@ -445,6 +474,12 @@ int hyper_dmabuf_xen_init_rx_rbuf(int domid) back_ring_isr, 0, NULL, (void*)ring_info); +fail_others: + kfree(map_ops); + +fail_no_map_ops: + kfree(ring_info); + return ret; } @@ -520,15 +555,22 @@ int hyper_dmabuf_xen_send_req(int domid, struct hyper_dmabuf_req *req, int wait) return -ENOENT; } - mutex_lock(&ring_info->lock); ring = &ring_info->ring_front; while (RING_FULL(ring)) { + if (timeout == 0) { + dev_err(hyper_dmabuf_private.device, + "Timeout while waiting for an entry in the ring\n"); + return -EIO; + } usleep_range(100, 120); + timeout--; } + timeout = 1000; + new_req = RING_GET_REQUEST(ring, ring->req_prod_pvt); if (!new_req) { mutex_unlock(&ring_info->lock); -- 2.7.4