From 67b38b0496484ac2b862c9514cff40b82a92a622 Mon Sep 17 00:00:00 2001 From: Yajun Li Date: Thu, 25 Oct 2018 19:13:54 +0800 Subject: [PATCH] soc: hab: Fix a memory leakage when unexport When the remote vchan is closed and unexport happens in local at the same time, function hab_get_vchan_fromvcid will return null in hab unexport, which will cause memory leak. Change-Id: I8dac4f4154f24734dc2a11aa73f726cd705cc291 Signed-off-by: Yajun Li --- drivers/soc/qcom/hab/hab.c | 35 +++++++++++------------------------ drivers/soc/qcom/hab/hab.h | 2 +- drivers/soc/qcom/hab/hab_mimex.c | 8 ++++---- drivers/soc/qcom/hab/hab_msg.c | 23 +++++++++++++++++++++++ drivers/soc/qcom/hab/hab_open.c | 12 ++++++++++++ drivers/soc/qcom/hab/hab_vchan.c | 23 ++++++++++++++++------- drivers/soc/qcom/hab/qvm_comm.c | 5 ++++- 7 files changed, 71 insertions(+), 37 deletions(-) diff --git a/drivers/soc/qcom/hab/hab.c b/drivers/soc/qcom/hab/hab.c index ee7e9881b193..7d53e6162172 100644 --- a/drivers/soc/qcom/hab/hab.c +++ b/drivers/soc/qcom/hab/hab.c @@ -209,14 +209,15 @@ void hab_ctx_free(struct kref *ref) * the local ioctl access based on ctx */ struct virtual_channel *hab_get_vchan_fromvcid(int32_t vcid, - struct uhab_context *ctx) + struct uhab_context *ctx, int ignore_remote) { struct virtual_channel *vchan; read_lock(&ctx->ctx_lock); list_for_each_entry(vchan, &ctx->vchannels, node) { if (vcid == vchan->id) { - if (vchan->otherend_closed || vchan->closed || + if ((ignore_remote ? 0 : vchan->otherend_closed) || + vchan->closed || !kref_get_unless_zero(&vchan->refcount)) { pr_debug("failed to inc vcid %x remote %x session %d refcnt %d close_flg remote %d local %d\n", vchan->id, vchan->otherend_id, @@ -550,7 +551,7 @@ long hab_vchan_send(struct uhab_context *ctx, return -EINVAL; } - vchan = hab_get_vchan_fromvcid(vcid, ctx); + vchan = hab_get_vchan_fromvcid(vcid, ctx, 0); if (!vchan || vchan->otherend_closed) { ret = -ENODEV; goto err; @@ -597,7 +598,7 @@ int hab_vchan_recv(struct uhab_context *ctx, int ret = 0; int nonblocking_flag = flags & HABMM_SOCKET_RECV_FLAGS_NON_BLOCKING; - vchan = hab_get_vchan_fromvcid(vcid, ctx); + vchan = hab_get_vchan_fromvcid(vcid, ctx, 1); if (!vchan) { pr_err("vcid %X vchan 0x%pK ctx %pK\n", vcid, vchan, ctx); return -ENODEV; @@ -719,24 +720,13 @@ void hab_vchan_close(struct uhab_context *ctx, int32_t vcid) write_lock(&ctx->ctx_lock); list_for_each_entry_safe(vchan, tmp, &ctx->vchannels, node) { if (vchan->id == vcid) { + vchan->closed = 1; write_unlock(&ctx->ctx_lock); pr_debug("vcid %x remote %x session %d refcnt %d\n", vchan->id, vchan->otherend_id, vchan->session_id, get_refcnt(vchan->refcount)); - /* - * only set when vc close is called locally by user - * explicity. Used to block remote msg. if forked once - * before, this local close is skipped due to child - * usage. if forked but not closed locally, the local - * context could NOT be closed, vchan can be prolonged - * by arrived remote msgs - */ - if (vchan->forked) - vchan->forked = 0; - else { - vchan->closed = 1; - hab_vchan_stop_notify(vchan); - } + /* unblocking blocked in-calls */ + hab_vchan_stop_notify(vchan); hab_vchan_put(vchan); /* there is a lock inside */ write_lock(&ctx->ctx_lock); break; @@ -1090,13 +1080,16 @@ static int hab_release(struct inode *inodep, struct file *filep) vchan->closed, vchan->otherend_closed); continue; /* vchan is already being freed */ } else { + write_unlock(&ctx->ctx_lock); hab_vchan_stop_notify(vchan); /* put for notify. shouldn't cause free */ hab_vchan_put(vchan); + write_lock(&ctx->ctx_lock); } } else continue; + /* for the missing local vchan close */ write_unlock(&ctx->ctx_lock); hab_vchan_put(vchan); /* there is a lock inside */ write_lock(&ctx->ctx_lock); @@ -1117,12 +1110,6 @@ static int hab_release(struct inode *inodep, struct file *filep) hab_ctx_put(ctx); filep->private_data = NULL; - /* ctx leak check */ - if (get_refcnt(ctx->refcount)) - pr_warn("pending ctx release owner %d refcnt %d total %d\n", - ctx->owner, get_refcnt(ctx->refcount), - hab_driver.ctx_cnt); - return 0; } diff --git a/drivers/soc/qcom/hab/hab.h b/drivers/soc/qcom/hab/hab.h index 18bcd0af8058..a2df3b64cce6 100644 --- a/drivers/soc/qcom/hab/hab.h +++ b/drivers/soc/qcom/hab/hab.h @@ -494,7 +494,7 @@ struct virtual_channel *hab_vchan_get(struct physical_channel *pchan, void hab_vchan_put(struct virtual_channel *vchan); struct virtual_channel *hab_get_vchan_fromvcid(int32_t vcid, - struct uhab_context *ctx); + struct uhab_context *ctx, int ignore_remote); struct physical_channel *hab_pchan_alloc(struct hab_device *habdev, int otherend_id); struct physical_channel *hab_pchan_find_domid(struct hab_device *dev, diff --git a/drivers/soc/qcom/hab/hab_mimex.c b/drivers/soc/qcom/hab/hab_mimex.c index e7ce3ebdddc8..b03739974390 100644 --- a/drivers/soc/qcom/hab/hab_mimex.c +++ b/drivers/soc/qcom/hab/hab_mimex.c @@ -246,7 +246,7 @@ int hab_mem_export(struct uhab_context *ctx, if (!ctx || !param || !param->buffer) return -EINVAL; - vchan = hab_get_vchan_fromvcid(param->vcid, ctx); + vchan = hab_get_vchan_fromvcid(param->vcid, ctx, 0); if (!vchan || !vchan->pchan) { ret = -ENODEV; goto err; @@ -313,7 +313,7 @@ int hab_mem_unexport(struct uhab_context *ctx, return -EINVAL; /* refcnt on the access */ - vchan = hab_get_vchan_fromvcid(param->vcid, ctx); + vchan = hab_get_vchan_fromvcid(param->vcid, ctx, 1); if (!vchan || !vchan->pchan) { ret = -ENODEV; goto err_novchan; @@ -360,7 +360,7 @@ int hab_mem_import(struct uhab_context *ctx, if (!ctx || !param) return -EINVAL; - vchan = hab_get_vchan_fromvcid(param->vcid, ctx); + vchan = hab_get_vchan_fromvcid(param->vcid, ctx, 0); if (!vchan || !vchan->pchan) { ret = -ENODEV; goto err_imp; @@ -420,7 +420,7 @@ int hab_mem_unimport(struct uhab_context *ctx, if (!ctx || !param) return -EINVAL; - vchan = hab_get_vchan_fromvcid(param->vcid, ctx); + vchan = hab_get_vchan_fromvcid(param->vcid, ctx, 1); if (!vchan || !vchan->pchan) { if (vchan) hab_vchan_put(vchan); diff --git a/drivers/soc/qcom/hab/hab_msg.c b/drivers/soc/qcom/hab/hab_msg.c index 9323cf948fb9..40ff1a9d6415 100644 --- a/drivers/soc/qcom/hab/hab_msg.c +++ b/drivers/soc/qcom/hab/hab_msg.c @@ -27,6 +27,12 @@ hab_msg_alloc(struct physical_channel *pchan, size_t sizebytes) { struct hab_message *message; + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("pchan %s send size too large %zd\n", + pchan->name, sizebytes); + return NULL; + } + message = kzalloc(sizeof(*message) + sizebytes, GFP_ATOMIC); if (!message) return NULL; @@ -153,6 +159,12 @@ static int hab_receive_create_export_ack(struct physical_channel *pchan, pr_err("exp ack size %zu is not as arrived %zu\n", sizeof(ack_recvd->ack), sizebytes); + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("pchan %s read size too large %zd\n", + pchan->name, sizebytes); + return -EINVAL; + } + if (physical_channel_read(pchan, &ack_recvd->ack, sizebytes) != sizebytes) @@ -169,6 +181,11 @@ static void hab_msg_drop(struct physical_channel *pchan, size_t sizebytes) { uint8_t *data = NULL; + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("%s read size too large %zd\n", pchan->name, sizebytes); + return; + } + data = kmalloc(sizebytes, GFP_ATOMIC); if (data == NULL) return; @@ -275,6 +292,12 @@ int hab_msg_recv(struct physical_channel *pchan, break; case HAB_PAYLOAD_TYPE_EXPORT: + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("%s exp size too large %zd\n", + pchan->name, sizebytes); + break; + } + exp_desc = kzalloc(sizebytes, GFP_ATOMIC); if (!exp_desc) break; diff --git a/drivers/soc/qcom/hab/hab_open.c b/drivers/soc/qcom/hab/hab_open.c index f09a1df8aa57..bc2b774883f4 100644 --- a/drivers/soc/qcom/hab/hab_open.c +++ b/drivers/soc/qcom/hab/hab_open.c @@ -47,6 +47,12 @@ int hab_open_request_add(struct physical_channel *pchan, struct hab_open_request *request; struct timeval tv; + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("pchan %s request size too large %zd\n", + pchan->name, sizebytes); + return -EINVAL; + } + node = kzalloc(sizeof(*node), GFP_ATOMIC); if (!node) return -ENOMEM; @@ -187,6 +193,12 @@ int hab_open_receive_cancel(struct physical_channel *pchan, int bfound = 0; struct timeval tv; + if (sizebytes > HAB_HEADER_SIZE_MASK) { + pr_err("pchan %s cancel size too large %zd\n", + pchan->name, sizebytes); + return -EINVAL; + } + if (physical_channel_read(pchan, &data, sizebytes) != sizebytes) return -EIO; diff --git a/drivers/soc/qcom/hab/hab_vchan.c b/drivers/soc/qcom/hab/hab_vchan.c index f4145c5869f8..770c6ba3dadd 100644 --- a/drivers/soc/qcom/hab/hab_vchan.c +++ b/drivers/soc/qcom/hab/hab_vchan.c @@ -206,15 +206,22 @@ static int hab_vchans_per_pchan_empty(struct physical_channel *pchan) empty = list_empty(&pchan->vchannels); if (!empty) { struct virtual_channel *vchan; + int vcnt = pchan->vcnt; list_for_each_entry(vchan, &pchan->vchannels, pnode) { - pr_err("vchan %pK id %x remote id %x session %d ref %d closed %d remote close %d\n", - vchan, vchan->id, vchan->otherend_id, - vchan->session_id, - get_refcnt(vchan->refcount), vchan->closed, - vchan->otherend_closed); + /* discount open-pending unpaired vchan */ + if (!vchan->session_id) + vcnt--; + else + pr_err("vchan %pK %x rm %x sn %d rf %d clsd %d rm clsd %d\n", + vchan, vchan->id, + vchan->otherend_id, + vchan->session_id, + get_refcnt(vchan->refcount), + vchan->closed, vchan->otherend_closed); } - + if (!vcnt) + empty = 1;/* unpaired vchan can exist at init time */ } read_unlock(&pchan->vchans_lock); @@ -308,7 +315,9 @@ void hab_vchan_put(struct virtual_channel *vchan) int hab_vchan_query(struct uhab_context *ctx, int32_t vcid, uint64_t *ids, char *names, size_t name_size, uint32_t flags) { - struct virtual_channel *vchan = hab_get_vchan_fromvcid(vcid, ctx); + struct virtual_channel *vchan; + + vchan = hab_get_vchan_fromvcid(vcid, ctx, 1); if (!vchan) return -EINVAL; diff --git a/drivers/soc/qcom/hab/qvm_comm.c b/drivers/soc/qcom/hab/qvm_comm.c index cce24d72c28e..ab57f2758aed 100644 --- a/drivers/soc/qcom/hab/qvm_comm.c +++ b/drivers/soc/qcom/hab/qvm_comm.c @@ -56,6 +56,7 @@ int physical_channel_send(struct physical_channel *pchan, return -EAGAIN; /* not enough free space */ } + header->sequence = pchan->sequence_tx + 1; header->signature = HAB_HEAD_SIGNATURE; if (hab_pipe_write(dev->pipe_ep, @@ -92,7 +93,7 @@ int physical_channel_send(struct physical_channel *pchan, hab_pipe_write_commit(dev->pipe_ep); spin_unlock_bh(&dev->io_lock); habhyp_notify(dev); - + ++pchan->sequence_tx; return 0; } @@ -117,6 +118,8 @@ void physical_channel_rx_dispatch(unsigned long data) header.sequence); } + pchan->sequence_rx = header.sequence; + hab_msg_recv(pchan, &header); } spin_unlock_bh(&pchan->rxbuf_lock); -- 2.11.0