From 9a64d016e5e11afd96a61261866f0fb3282d14bc Mon Sep 17 00:00:00 2001 From: "Isaac J. Manjarres" Date: Wed, 9 Dec 2020 23:45:26 -0800 Subject: [PATCH 1/3] soc: qcom: mem-buf: Fix message processing race condition mem-buf currently uses a workqueue with multiple worker threads associated with it for handling allocation requests and reclaiming memory. This can cause overlaps in time where an allocation request can overlap with memory reclaim. This is not ideal, as the memory allocation request may fail if the memory that is supposed to have been reclaimed has not been reclaimed yet. Thus, use an ordered workqueue to make sure that there is no overlap between processing allocation and memory reclaim messages. Change-Id: I4a76c74b1785d2448c6c2f97a43efaa5bf6ad99a Signed-off-by: Isaac J. Manjarres --- drivers/soc/qcom/mem-buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/qcom/mem-buf.c b/drivers/soc/qcom/mem-buf.c index 2c9228f34155..c7cb2d71f44a 100644 --- a/drivers/soc/qcom/mem-buf.c +++ b/drivers/soc/qcom/mem-buf.c @@ -2146,7 +2146,7 @@ static int mem_buf_probe(struct platform_device *pdev) return ret; } - mem_buf_wq = alloc_workqueue("mem_buf_wq", WQ_HIGHPRI | WQ_UNBOUND, 0); + mem_buf_wq = alloc_ordered_workqueue("mem_buf_wq", WQ_HIGHPRI); if (!mem_buf_wq) { dev_err(dev, "Unable to initialize workqueue\n"); return -EINVAL; From 2c04df9e33ec7baee783234adf911d59e52256a8 Mon Sep 17 00:00:00 2001 From: "Isaac J. Manjarres" Date: Wed, 9 Dec 2020 23:04:44 -0800 Subject: [PATCH 2/3] dma-buf: Introduce dma_buf_put_sync() dma_buf_put() is supposed to invoke the release dma-buf callback for a dma-buf and free the memory when the dma-buf's refcount reaches 0. However, when dma_buf_put() is invoked from a kernel thread (e.g. kthread or kworker), the dma-buf is actually freed later on in time, asynchronously, by a worker thread. This behavior can cause issues where memory is allocated from an ION heap, such as a CMA heap, and released from the context of a worker thread, and the memory is allocated soon after the call to dma_buf_put(). In that case, it is possible for the memory to not have been freed yet. Thus, introduce dma_buf_put_sync(), which ensures that the buffer is freed when the function is called, and the buffer does not have any outstanding references. Change-Id: Iba63c968b16669013684861afd60c0212062412e Signed-off-by: Isaac J. Manjarres --- drivers/dma-buf/dma-buf.c | 31 +++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 1 + 2 files changed, 32 insertions(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 79d8b75310a8..d510b8fb5f06 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -688,6 +688,37 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put); +/** + * dma_buf_put_sync - decreases refcount of the buffer + * @dmabuf: [in] buffer to reduce refcount of + * + * Uses file's refcounting done implicitly by __fput_sync(). + * + * If, as a result of this call, the refcount becomes 0, the 'release' file + * operation related to this fd is called. It calls &dma_buf_ops.release vfunc + * in turn, and frees the memory allocated for dmabuf when exported. + * + * This function is different than dma_buf_put() in the sense that it guarantees + * that the 'release' file operation related to this fd is called, and that the + * memory is released, when the refcount becomes 0. dma_buf_put() does not + * have the same guarantee when invoked by a kernel thread (e.g. a worker + * thread), and the refcount reaches 0; in that case, the buffer is added to + * the delayed_fput_list, and freed asynchronously. + * + * This function should not be called in atomic context, and should only be + * called by kernel threads. If in doubt, use dma_buf_put(). + */ +void dma_buf_put_sync(struct dma_buf *dmabuf) +{ + if (WARN_ON(!dmabuf || !dmabuf->file)) + return; + + might_sleep(); + + dma_buf_ref_mod(to_msm_dma_buf(dmabuf), -1); + __fput_sync(dmabuf->file); +} + /** * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index a7da54d72150..accce2b534e7 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -531,6 +531,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info); int dma_buf_fd(struct dma_buf *dmabuf, int flags); struct dma_buf *dma_buf_get(int fd); void dma_buf_put(struct dma_buf *dmabuf); +void dma_buf_put_sync(struct dma_buf *dmabuf); struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); From a7234c9b81a56455159b8579aea31ee14efa450b Mon Sep 17 00:00:00 2001 From: "Isaac J. Manjarres" Date: Wed, 9 Dec 2020 23:22:00 -0800 Subject: [PATCH 3/3] soc: qcom: mem-buf: Ensure dma-bufs are freed when refcount is 0 When mem-buf frees a dma-buf (i.e. calls dma_buf_put() and it is guaranteed that the refcount will be 0), it does so from the context of a kworker. As a result of this, dma_buf_put() will free the dma-buf asynchronously, which means that it is possible that the buffer has not been freed when dma_buf_put() is invoked. In the case where memory is being reclaimed by the primary VM, and another VM requests the memory again soon after relinquishing it, this situation results in failing the memory allocation, as the buffer is still in the process of being freed. Thus, use dma_buf_put_sync() to ensure that the buffer is freed whenever mem-buf needs to free a dma-buf. Change-Id: I60af08a143b91eddb6738f65cc8e943cf04065ad Signed-off-by: Isaac J. Manjarres --- drivers/soc/qcom/mem-buf.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/soc/qcom/mem-buf.c b/drivers/soc/qcom/mem-buf.c index c7cb2d71f44a..92bf52b7e7ce 100644 --- a/drivers/soc/qcom/mem-buf.c +++ b/drivers/soc/qcom/mem-buf.c @@ -277,6 +277,12 @@ static int mem_buf_rmt_alloc_ion_mem(struct mem_buf_xfer_mem *xfer_mem) xfer_mem->secure_alloc = false; } + /* + * If the buffer needs to be freed because of error handling, ensure + * that dma_buf_put_sync() is invoked, instead of dma_buf_put(). Doing + * so ensures that the memory is freed before the next allocation + * request is serviced. + */ dmabuf = ion_alloc(xfer_mem->size, heap_id, ion_flags); if (IS_ERR(dmabuf)) { pr_err("%s ion_alloc failure sz: 0x%x heap_id: %d flags: 0x%x rc: %d\n", @@ -289,7 +295,7 @@ static int mem_buf_rmt_alloc_ion_mem(struct mem_buf_xfer_mem *xfer_mem) if (IS_ERR(attachment)) { pr_err("%s dma_buf_attach failure rc: %d\n", __func__, PTR_ERR(attachment)); - dma_buf_put(dmabuf); + dma_buf_put_sync(dmabuf); return PTR_ERR(attachment); } @@ -298,7 +304,7 @@ static int mem_buf_rmt_alloc_ion_mem(struct mem_buf_xfer_mem *xfer_mem) pr_err("%s dma_buf_map_attachment failure rc: %d\n", __func__, PTR_ERR(mem_sgt)); dma_buf_detach(dmabuf, attachment); - dma_buf_put(dmabuf); + dma_buf_put_sync(dmabuf); return PTR_ERR(mem_sgt); } @@ -330,7 +336,11 @@ static void mem_buf_rmt_free_ion_mem(struct mem_buf_xfer_mem *xfer_mem) pr_debug("%s: Freeing ION memory\n", __func__); dma_buf_unmap_attachment(attachment, mem_sgt, DMA_BIDIRECTIONAL); dma_buf_detach(dmabuf, attachment); - dma_buf_put(ion_mem_data->dmabuf); + /* + * Use dma_buf_put_sync() instead of dma_buf_put() to ensure that the + * memory is actually freed, before the next allocation request. + */ + dma_buf_put_sync(ion_mem_data->dmabuf); pr_debug("%s: ION memory freed\n", __func__); }