[PATCH v1 0/5] misc: fastrpc: Add missing bug fixes

[PATCH v1 0/5] misc: fastrpc: Add missing bug fixes

[Ekansh Gupta]

Add missing bug fixes in memory areas. This patch series carries
following fixes:
- Add proper checks to fastrpc_buf_free to avoid potential issues.
- Add multiple fixes for remote heap which is used by Audio PD.

Ekansh Gupta (5):
  misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash
  misc: fastrpc: Move all remote heap allocations to a new list
  misc: fastrpc: Fix initial memory allocation for Audio PD memory pool
  misc: fastrpc: Remove buffer from list prior to unmap operation
  misc: fastrpc: Add missing unmapping user-requested remote heap

 drivers/misc/fastrpc.c | 182 +++++++++++++++++++++++++++++++++--------
 1 file changed, 146 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v1 1/5] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Ekansh Gupta]

The fastrpc_buf_free function currently does not handle the case where
the input buffer pointer (buf) is NULL. This can lead to a null pointer
dereference, causing a crash or undefined behavior when the function
attempts to access members of the buf structure. Add a NULL check to
ensure safe handling of NULL pointers and prevent potential crashes.

Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7b7a22c91fe4..ca3721365ddc 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -394,6 +394,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
 
 static void fastrpc_buf_free(struct fastrpc_buf *buf)
 {
+	if (!buf)
+		return;
+
 	dma_free_coherent(buf->dev, buf->size, buf->virt,
 			  FASTRPC_PHYS(buf->phys));
 	kfree(buf);
-- 
2.34.1

[PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Ekansh Gupta]

Remote heap allocations are not organized in a maintainable manner,
leading to potential issues with memory management. As the remote
heap allocations are maintained in fl mmaps list, the allocations
will go away if the audio daemon process is killed but there are
chances that audio PD might still be using the memory. Move all
remote heap allocations to a dedicated list where the entries are
cleaned only for user requests and subsystem shutdown.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 21 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index ca3721365ddc..d4e38b5e5e6c 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -273,10 +273,12 @@ struct fastrpc_channel_ctx {
 	struct kref refcount;
 	/* Flag if dsp attributes are cached */
 	bool valid_attributes;
+	/* Flag if audio PD init mem was allocated */
+	bool audio_init_mem;
 	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
 	struct fastrpc_device *secure_fdevice;
 	struct fastrpc_device *fdevice;
-	struct fastrpc_buf *remote_heap;
+	struct list_head rhmaps;
 	struct list_head invoke_interrupted_mmaps;
 	bool secure;
 	bool unsigned_support;
@@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
 	return false;
 }
 
+static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx)
+{
+	struct fastrpc_buf *buf, *b;
+	struct list_head temp_list;
+	int err;
+	unsigned long flags;
+
+	INIT_LIST_HEAD(&temp_list);
+
+	spin_lock_irqsave(&cctx->lock, flags);
+	list_splice_init(&cctx->rhmaps, &temp_list);
+	spin_unlock_irqrestore(&cctx->lock, flags);
+
+	list_for_each_entry_safe(buf, b, &temp_list, node) {
+		if (cctx->vmcount) {
+			u64 src_perms = 0;
+			struct qcom_scm_vmperm dst_perms;
+			u32 i;
+
+			for (i = 0; i < cctx->vmcount; i++)
+				src_perms |= BIT(cctx->vmperms[i].vmid);
+
+			dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+			dst_perms.perm = QCOM_SCM_PERM_RWX;
+			err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+						  &src_perms, &dst_perms, 1);
+			if (err)
+				continue;
+		}
+		fastrpc_buf_free(buf);
+	}
+}
+
 static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 					      char __user *argp)
 {
 	struct fastrpc_init_create_static init;
 	struct fastrpc_invoke_args *args;
 	struct fastrpc_phy_page pages[1];
+	struct fastrpc_buf *buf = NULL;
+	u64 phys = 0, size = 0;
 	char *name;
 	int err;
 	bool scm_done = false;
@@ -1252,6 +1289,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		u32 pageslen;
 	} inbuf;
 	u32 sc;
+	unsigned long flags;
 
 	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
 	if (!args)
@@ -1273,26 +1311,30 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
-	if (!fl->cctx->remote_heap) {
+	if (!fl->cctx->audio_init_mem) {
 		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
-						&fl->cctx->remote_heap);
+						&buf);
 		if (err)
 			goto err_name;
 
+		phys = buf->phys;
+		size = buf->size;
 		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
 		if (fl->cctx->vmcount) {
 			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
 
-			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
-							(u64)fl->cctx->remote_heap->size,
-							&src_perms,
-							fl->cctx->vmperms, fl->cctx->vmcount);
+			err = qcom_scm_assign_mem(phys, size, &src_perms,
+						  fl->cctx->vmperms, fl->cctx->vmcount);
 			if (err) {
 				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
-					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+					phys, size, err);
 				goto err_map;
 			}
 			scm_done = true;
+			spin_lock_irqsave(&fl->cctx->lock, flags);
+			list_add_tail(&buf->node, &fl->cctx->rhmaps);
+			spin_unlock_irqrestore(&fl->cctx->lock, flags);
+			fl->cctx->audio_init_mem = true;
 		}
 	}
 
@@ -1309,8 +1351,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	args[1].length = inbuf.namelen;
 	args[1].fd = -1;
 
-	pages[0].addr = fl->cctx->remote_heap->phys;
-	pages[0].size = fl->cctx->remote_heap->size;
+	pages[0].addr = phys;
+	pages[0].size = size;
 
 	args[2].ptr = (u64)(uintptr_t) pages;
 	args[2].length = sizeof(*pages);
@@ -1328,6 +1370,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 	return 0;
 err_invoke:
+	if (buf) {
+		spin_lock_irqsave(&fl->cctx->lock, flags);
+		list_del(&buf->node);
+		spin_unlock_irqrestore(&fl->cctx->lock, flags);
+	}
 	if (fl->cctx->vmcount && scm_done) {
 		u64 src_perms = 0;
 		struct qcom_scm_vmperm dst_perms;
@@ -1338,15 +1385,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
 		dst_perms.perm = QCOM_SCM_PERM_RWX;
-		err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
-						(u64)fl->cctx->remote_heap->size,
-						&src_perms, &dst_perms, 1);
+		err = qcom_scm_assign_mem(phys, size, &src_perms,
+					  &dst_perms, 1);
 		if (err)
 			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
-				fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+				phys, size, err);
 	}
 err_map:
-	fastrpc_buf_free(fl->cctx->remote_heap);
+	fl->cctx->audio_init_mem = false;
+	fastrpc_buf_free(buf);
 err_name:
 	kfree(name);
 err:
@@ -1869,6 +1916,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 	struct device *dev = fl->sctx->dev;
 	int err;
 	u32 sc;
+	unsigned long flags;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -1937,12 +1985,15 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 					buf->phys, buf->size, err);
 			goto err_assign;
 		}
+		spin_lock_irqsave(&fl->cctx->lock, flags);
+		list_add_tail(&buf->node, &fl->cctx->rhmaps);
+		spin_unlock_irqrestore(&fl->cctx->lock, flags);
+	} else {
+		spin_lock(&fl->lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(&fl->lock);
 	}
 
-	spin_lock(&fl->lock);
-	list_add_tail(&buf->node, &fl->mmaps);
-	spin_unlock(&fl->lock);
-
 	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
 		err = -EFAULT;
 		goto err_assign;
@@ -2362,6 +2413,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	rdev->dma_mask = &data->dma_mask;
 	dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32));
 	INIT_LIST_HEAD(&data->users);
+	INIT_LIST_HEAD(&data->rhmaps);
 	INIT_LIST_HEAD(&data->invoke_interrupted_mmaps);
 	spin_lock_init(&data->lock);
 	idr_init(&data->ctx_idr);
@@ -2420,8 +2472,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
 		list_del(&buf->node);
 
-	if (cctx->remote_heap)
-		fastrpc_buf_free(cctx->remote_heap);
+	fastrpc_cleanup_rhmaps(cctx);
 
 	of_platform_depopulate(&rpdev->dev);
 
-- 
2.34.1

[PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Ekansh Gupta]

The initially allocated memory is not properly included in the pool,
leading to potential issues with memory management. Set the number
of pages to one to ensure that the initially allocated memory is
correctly added to the Audio PD memory pool.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index d4e38b5e5e6c..b629e24f00bc 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
+	inbuf.client_id = fl->client_id;
+	inbuf.namelen = init.namelen;
+	inbuf.pageslen = 0;
 	if (!fl->cctx->audio_init_mem) {
 		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
 						&buf);
@@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 			list_add_tail(&buf->node, &fl->cctx->rhmaps);
 			spin_unlock_irqrestore(&fl->cctx->lock, flags);
 			fl->cctx->audio_init_mem = true;
+			inbuf.pageslen = 1;
 		}
 	}
 
-	inbuf.client_id = fl->client_id;
-	inbuf.namelen = init.namelen;
-	inbuf.pageslen = 0;
 	fl->pd = USER_PD;
 
 	args[0].ptr = (u64)(uintptr_t)&inbuf;
-- 
2.34.1

[PATCH v1 4/5] misc: fastrpc: Remove buffer from list prior to unmap operation

[Ekansh Gupta]

fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
getting removed from the list after it is unmapped from DSP. This can
create potential race conditions if any other thread removes the entry
from list while unmap operation is ongoing. Remove the entry before
calling unmap operation.

Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index b629e24f00bc..d54368bf8c5c 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1868,9 +1868,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 				      &args[0]);
 	if (!err) {
 		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
-		spin_lock(&fl->lock);
-		list_del(&buf->node);
-		spin_unlock(&fl->lock);
 		fastrpc_buf_free(buf);
 	} else {
 		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
@@ -1884,13 +1881,15 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	struct fastrpc_buf *buf = NULL, *iter, *b;
 	struct fastrpc_req_munmap req;
 	struct device *dev = fl->sctx->dev;
+	int err;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
 
 	spin_lock(&fl->lock);
 	list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
-		if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
+		if (iter->raddr == req.vaddrout && iter->size == req.size) {
+			list_del(&iter->node);
 			buf = iter;
 			break;
 		}
@@ -1903,7 +1902,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 		return -EINVAL;
 	}
 
-	return fastrpc_req_munmap_impl(fl, buf);
+	err = fastrpc_req_munmap_impl(fl, buf);
+	if (err) {
+		spin_lock(&fl->lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(&fl->lock);
+	}
+
+	return err;
 }
 
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
@@ -1997,14 +2003,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 
 	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
 		err = -EFAULT;
-		goto err_assign;
+		goto err_copy;
 	}
 
 	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
 		buf->raddr, buf->size);
 
 	return 0;
-
+err_copy:
+	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+		spin_lock_irqsave(&fl->cctx->lock, flags);
+		list_del(&buf->node);
+		spin_unlock_irqrestore(&fl->cctx->lock, flags);
+	} else {
+		spin_lock(&fl->lock);
+		list_del(&buf->node);
+		spin_unlock(&fl->lock);
+	}
 err_assign:
 	fastrpc_req_munmap_impl(fl, buf);
 
-- 
2.34.1

[PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

User request for remote heap allocation is supported using ioctl
interface but support for unmap is missing. This could result in
memory leak issues. Add unmap user request support for remote heap.

Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index d54368bf8c5c..b64c5b9e07b5 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -197,6 +197,8 @@ struct fastrpc_buf {
 	struct dma_buf *dmabuf;
 	struct device *dev;
 	void *virt;
+	/* Type of buffer */
+	u32 flag;
 	u64 phys;
 	u64 size;
 	/* Lock for dma buf attachments */
@@ -1867,8 +1869,26 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
 				      &args[0]);
 	if (!err) {
-		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
+		if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
+			u64 src_perms = 0;
+			struct qcom_scm_vmperm dst_perms;
+			u32 i;
+
+			for (i = 0; i < fl->cctx->vmcount; i++)
+				src_perms |= BIT(fl->cctx->vmperms[i].vmid);
+
+			dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+			dst_perms.perm = QCOM_SCM_PERM_RWX;
+			err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+						  &src_perms, &dst_perms, 1);
+			if (err) {
+				dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+					buf->phys, buf->size, err);
+				return err;
+			}
+		}
 		fastrpc_buf_free(buf);
+		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
 	} else {
 		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
 	}
@@ -1882,6 +1902,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	struct fastrpc_req_munmap req;
 	struct device *dev = fl->sctx->dev;
 	int err;
+	unsigned long flags;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -1896,20 +1917,38 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	}
 	spin_unlock(&fl->lock);
 
-	if (!buf) {
-		dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
-			req.vaddrout, req.size);
-		return -EINVAL;
+	if (buf) {
+		err = fastrpc_req_munmap_impl(fl, buf);
+		if (err) {
+			spin_lock(&fl->lock);
+			list_add_tail(&buf->node, &fl->mmaps);
+			spin_unlock(&fl->lock);
+		}
+		return err;
 	}
 
-	err = fastrpc_req_munmap_impl(fl, buf);
-	if (err) {
-		spin_lock(&fl->lock);
-		list_add_tail(&buf->node, &fl->mmaps);
-		spin_unlock(&fl->lock);
+	spin_lock_irqsave(&fl->cctx->lock, flags);
+	list_for_each_entry_safe(iter, b, &fl->cctx->rhmaps, node) {
+		if (iter->raddr == req.vaddrout && iter->size == req.size) {
+			list_del(&iter->node);
+			buf = iter;
+			break;
+		}
 	}
+	spin_unlock_irqrestore(&fl->cctx->lock, flags);
+	if (buf) {
+		err = fastrpc_req_munmap_impl(fl, buf);
+		if (err) {
+			spin_lock_irqsave(&fl->cctx->lock, flags);
+			list_add_tail(&buf->node, &fl->cctx->rhmaps);
+			spin_unlock_irqrestore(&fl->cctx->lock, flags);
+		}
+		return err;
+	}
+	dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
+		req.vaddrout, req.size);
 
-	return err;
+	return -EINVAL;
 }
 
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
@@ -1977,6 +2016,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 
 	/* update the buffer to be able to deallocate the memory on the DSP */
 	buf->raddr = (uintptr_t) rsp_msg.vaddr;
+	buf->flag = req.flags;
 
 	/* let the client know the address to use */
 	req.vaddrout = rsp_msg.vaddr;
-- 
2.34.1

Re: [PATCH v1 1/5] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Srinivas Kandagatla]

On 5/13/25 05:28, Ekansh Gupta wrote:
> The fastrpc_buf_free function currently does not handle the case where
> the input buffer pointer (buf) is NULL. This can lead to a null pointer
> dereference, causing a crash or undefined behavior when the function
> attempts to access members of the buf structure. Add a NULL check to
> ensure safe handling of NULL pointers and prevent potential crashes.
> 
You are mostly defining the code here, but not the root cause of it,
What exactly is the call trace for this crash?

> Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7b7a22c91fe4..ca3721365ddc 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -394,6 +394,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>  
>  static void fastrpc_buf_free(struct fastrpc_buf *buf)
>  {
> +	if (!buf)
> +		return;
> +
Most of the users of the fastrpc_buf_free() already have the null
checks, It will be Interesting to know.

If we decide to make this function to do null null check, then the
existing checks in the caller are redundant.

--srini
>  	dma_free_coherent(buf->dev, buf->size, buf->virt,
>  			  FASTRPC_PHYS(buf->phys));
>  	kfree(buf);

Re: [PATCH v1 1/5] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Dmitry Baryshkov]

On Mon, May 19, 2025 at 10:25:46AM +0100, Srinivas Kandagatla wrote:
> On 5/13/25 05:28, Ekansh Gupta wrote:
> > The fastrpc_buf_free function currently does not handle the case where
> > the input buffer pointer (buf) is NULL. This can lead to a null pointer
> > dereference, causing a crash or undefined behavior when the function
> > attempts to access members of the buf structure. Add a NULL check to
> > ensure safe handling of NULL pointers and prevent potential crashes.
> > 
> You are mostly defining the code here, but not the root cause of it,
> What exactly is the call trace for this crash?
> 
> > Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
> > Cc: stable@kernel.org
> > Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> > ---
> >  drivers/misc/fastrpc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > index 7b7a22c91fe4..ca3721365ddc 100644
> > --- a/drivers/misc/fastrpc.c
> > +++ b/drivers/misc/fastrpc.c
> > @@ -394,6 +394,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
> >  
> >  static void fastrpc_buf_free(struct fastrpc_buf *buf)
> >  {
> > +	if (!buf)
> > +		return;
> > +
> Most of the users of the fastrpc_buf_free() already have the null
> checks, It will be Interesting to know.
> 
> If we decide to make this function to do null null check, then the
> existing checks in the caller are redundant.

I think it was a primary reason for a change: to eliminate NULL checks
on the caller side, as we do in a lot of other kernel API.

> 
> --srini
> >  	dma_free_coherent(buf->dev, buf->size, buf->virt,
> >  			  FASTRPC_PHYS(buf->phys));
> >  	kfree(buf);
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Dmitry Baryshkov]

On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
> Remote heap allocations are not organized in a maintainable manner,
> leading to potential issues with memory management. As the remote

Which issues? I think I have been asking this question previously.
Please expand the commit message here.

> heap allocations are maintained in fl mmaps list, the allocations
> will go away if the audio daemon process is killed but there are

What is audio daemon process?

> chances that audio PD might still be using the memory. Move all
> remote heap allocations to a dedicated list where the entries are
> cleaned only for user requests and subsystem shutdown.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 72 insertions(+), 21 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Dmitry Baryshkov]

On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote:
> The initially allocated memory is not properly included in the pool,
> leading to potential issues with memory management. Set the number

What is 'properly'? Which issues?

> of pages to one to ensure that the initially allocated memory is
> correctly added to the Audio PD memory pool.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d4e38b5e5e6c..b629e24f00bc 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  		goto err;
>  	}
>  
> +	inbuf.client_id = fl->client_id;
> +	inbuf.namelen = init.namelen;
> +	inbuf.pageslen = 0;
>  	if (!fl->cctx->audio_init_mem) {
>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>  						&buf);
> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
>  			fl->cctx->audio_init_mem = true;
> +			inbuf.pageslen = 1;
>  		}
>  	}
>  
> -	inbuf.client_id = fl->client_id;
> -	inbuf.namelen = init.namelen;
> -	inbuf.pageslen = 0;
>  	fl->pd = USER_PD;
>  
>  	args[0].ptr = (u64)(uintptr_t)&inbuf;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] misc: fastrpc: Remove buffer from list prior to unmap operation

[Dmitry Baryshkov]

On Tue, May 13, 2025 at 09:58:24AM +0530, Ekansh Gupta wrote:
> fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
> getting removed from the list after it is unmapped from DSP. This can
> create potential race conditions if any other thread removes the entry
> from list while unmap operation is ongoing. Remove the entry before
> calling unmap operation.
> 
> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index b629e24f00bc..d54368bf8c5c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1868,9 +1868,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>  				      &args[0]);
>  	if (!err) {
>  		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
> -		spin_lock(&fl->lock);
> -		list_del(&buf->node);
> -		spin_unlock(&fl->lock);
>  		fastrpc_buf_free(buf);
>  	} else {
>  		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
> @@ -1884,13 +1881,15 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  	struct fastrpc_buf *buf = NULL, *iter, *b;
>  	struct fastrpc_req_munmap req;
>  	struct device *dev = fl->sctx->dev;
> +	int err;
>  
>  	if (copy_from_user(&req, argp, sizeof(req)))
>  		return -EFAULT;
>  
>  	spin_lock(&fl->lock);
>  	list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
> -		if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
> +		if (iter->raddr == req.vaddrout && iter->size == req.size) {

Cosmetics, please drop.

> +			list_del(&iter->node);
>  			buf = iter;
>  			break;
>  		}
> @@ -1903,7 +1902,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  		return -EINVAL;
>  	}
>  
> -	return fastrpc_req_munmap_impl(fl, buf);
> +	err = fastrpc_req_munmap_impl(fl, buf);
> +	if (err) {
> +		spin_lock(&fl->lock);
> +		list_add_tail(&buf->node, &fl->mmaps);
> +		spin_unlock(&fl->lock);
> +	}
> +
> +	return err;
>  }
>  
>  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> @@ -1997,14 +2003,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  
>  	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>  		err = -EFAULT;
> -		goto err_assign;
> +		goto err_copy;
>  	}
>  
>  	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>  		buf->raddr, buf->size);
>  
>  	return 0;
> -

Please keep the empty line here, it improves readability.

> +err_copy:
> +	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> +		spin_lock_irqsave(&fl->cctx->lock, flags);
> +		list_del(&buf->node);
> +		spin_unlock_irqrestore(&fl->cctx->lock, flags);
> +	} else {
> +		spin_lock(&fl->lock);
> +		list_del(&buf->node);
> +		spin_unlock(&fl->lock);
> +	}

Can we store the spinlock pointer in the struct fastrpc_buf instead?

>  err_assign:
>  	fastrpc_req_munmap_impl(fl, buf);
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/5] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Srinivas Kandagatla]

On 5/19/25 11:09, Dmitry Baryshkov wrote:
> On Mon, May 19, 2025 at 10:25:46AM +0100, Srinivas Kandagatla wrote:
>> On 5/13/25 05:28, Ekansh Gupta wrote:
>>> The fastrpc_buf_free function currently does not handle the case where
>>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>>> dereference, causing a crash or undefined behavior when the function
>>> attempts to access members of the buf structure. Add a NULL check to
>>> ensure safe handling of NULL pointers and prevent potential crashes.
>>>
>> You are mostly defining the code here, but not the root cause of it,
>> What exactly is the call trace for this crash?
>>
>>> Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
>>> Cc: stable@kernel.org
>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>> ---
>>>  drivers/misc/fastrpc.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 7b7a22c91fe4..ca3721365ddc 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -394,6 +394,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>>>  
>>>  static void fastrpc_buf_free(struct fastrpc_buf *buf)
>>>  {
>>> +	if (!buf)
>>> +		return;
>>> +
>> Most of the users of the fastrpc_buf_free() already have the null
>> checks, It will be Interesting to know.
>>
>> If we decide to make this function to do null null check, then the
>> existing checks in the caller are redundant.
> 
> I think it was a primary reason for a change: to eliminate NULL checks
> on the caller side, as we do in a lot of other kernel API.

Lets remove the existing NULL checks at caller side as part of this
patch too.


--Srini

> 
>>
>> --srini
>>>  	dma_free_coherent(buf->dev, buf->size, buf->virt,
>>>  			  FASTRPC_PHYS(buf->phys));
>>>  	kfree(buf);
>>
> 

Re: [PATCH v1 1/5] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Ekansh Gupta]

On 5/19/2025 4:10 PM, Srinivas Kandagatla wrote:
> On 5/19/25 11:09, Dmitry Baryshkov wrote:
>> On Mon, May 19, 2025 at 10:25:46AM +0100, Srinivas Kandagatla wrote:
>>> On 5/13/25 05:28, Ekansh Gupta wrote:
>>>> The fastrpc_buf_free function currently does not handle the case where
>>>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>>>> dereference, causing a crash or undefined behavior when the function
>>>> attempts to access members of the buf structure. Add a NULL check to
>>>> ensure safe handling of NULL pointers and prevent potential crashes.
>>>>
>>> You are mostly defining the code here, but not the root cause of it,
>>> What exactly is the call trace for this crash?
>>>
>>>> Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
>>>> Cc: stable@kernel.org
>>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..ca3721365ddc 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -394,6 +394,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>>>>  
>>>>  static void fastrpc_buf_free(struct fastrpc_buf *buf)
>>>>  {
>>>> +	if (!buf)
>>>> +		return;
>>>> +
>>> Most of the users of the fastrpc_buf_free() already have the null
>>> checks, It will be Interesting to know.
>>>
>>> If we decide to make this function to do null null check, then the
>>> existing checks in the caller are redundant.
>> I think it was a primary reason for a change: to eliminate NULL checks
>> on the caller side, as we do in a lot of other kernel API.
> Lets remove the existing NULL checks at caller side as part of this
> patch too.
Sure, I'll remove the checks in the next spin.
>
>
> --Srini
>
>>> --srini
>>>>  	dma_free_coherent(buf->dev, buf->size, buf->virt,
>>>>  			  FASTRPC_PHYS(buf->phys));
>>>>  	kfree(buf);

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> User request for remote heap allocation is supported using ioctl
> interface but support for unmap is missing. This could result in
> memory leak issues. Add unmap user request support for remote heap.

Can this memory be in use by the remote proc?

> 
> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Ekansh Gupta]

On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote:
> On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote:
>> The initially allocated memory is not properly included in the pool,
>> leading to potential issues with memory management. Set the number
> What is 'properly'? Which issues?

inbuf.pageslen is getting updated to 1 in case buffer is allocated, which only
happens if (!fl->cctx->audio_init_mem).

Till now pageslen is always 0 irrespective of whether the memory is allocated
or not due to which audio PD is never able to use this memory.

I'll update this to the commit in the next spin.

>
>> of pages to one to ensure that the initially allocated memory is
>> correctly added to the Audio PD memory pool.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index d4e38b5e5e6c..b629e24f00bc 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  		goto err;
>>  	}
>>  
>> +	inbuf.client_id = fl->client_id;
>> +	inbuf.namelen = init.namelen;
>> +	inbuf.pageslen = 0;
>>  	if (!fl->cctx->audio_init_mem) {
>>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>>  						&buf);
>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
>>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
>>  			fl->cctx->audio_init_mem = true;
>> +			inbuf.pageslen = 1;
>>  		}
>>  	}
>>  
>> -	inbuf.client_id = fl->client_id;
>> -	inbuf.namelen = init.namelen;
>> -	inbuf.pageslen = 0;
>>  	fl->pd = USER_PD;
>>  
>>  	args[0].ptr = (u64)(uintptr_t)&inbuf;
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 4/5] misc: fastrpc: Remove buffer from list prior to unmap operation

[Ekansh Gupta]

On 5/19/2025 3:50 PM, Dmitry Baryshkov wrote:
> On Tue, May 13, 2025 at 09:58:24AM +0530, Ekansh Gupta wrote:
>> fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
>> getting removed from the list after it is unmapped from DSP. This can
>> create potential race conditions if any other thread removes the entry
>> from list while unmap operation is ongoing. Remove the entry before
>> calling unmap operation.
>>
>> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index b629e24f00bc..d54368bf8c5c 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1868,9 +1868,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>>  				      &args[0]);
>>  	if (!err) {
>>  		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
>> -		spin_lock(&fl->lock);
>> -		list_del(&buf->node);
>> -		spin_unlock(&fl->lock);
>>  		fastrpc_buf_free(buf);
>>  	} else {
>>  		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
>> @@ -1884,13 +1881,15 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>>  	struct fastrpc_buf *buf = NULL, *iter, *b;
>>  	struct fastrpc_req_munmap req;
>>  	struct device *dev = fl->sctx->dev;
>> +	int err;
>>  
>>  	if (copy_from_user(&req, argp, sizeof(req)))
>>  		return -EFAULT;
>>  
>>  	spin_lock(&fl->lock);
>>  	list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
>> -		if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
>> +		if (iter->raddr == req.vaddrout && iter->size == req.size) {
> Cosmetics, please drop.
Ack.
>
>> +			list_del(&iter->node);
>>  			buf = iter;
>>  			break;
>>  		}
>> @@ -1903,7 +1902,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>>  		return -EINVAL;
>>  	}
>>  
>> -	return fastrpc_req_munmap_impl(fl, buf);
>> +	err = fastrpc_req_munmap_impl(fl, buf);
>> +	if (err) {
>> +		spin_lock(&fl->lock);
>> +		list_add_tail(&buf->node, &fl->mmaps);
>> +		spin_unlock(&fl->lock);
>> +	}
>> +
>> +	return err;
>>  }
>>  
>>  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> @@ -1997,14 +2003,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>  
>>  	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>>  		err = -EFAULT;
>> -		goto err_assign;
>> +		goto err_copy;
>>  	}
>>  
>>  	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>>  		buf->raddr, buf->size);
>>  
>>  	return 0;
>> -
> Please keep the empty line here, it improves readability.
Ack.
>
>> +err_copy:
>> +	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
>> +		spin_lock_irqsave(&fl->cctx->lock, flags);
>> +		list_del(&buf->node);
>> +		spin_unlock_irqrestore(&fl->cctx->lock, flags);
>> +	} else {
>> +		spin_lock(&fl->lock);
>> +		list_del(&buf->node);
>> +		spin_unlock(&fl->lock);
>> +	}
> Can we store the spinlock pointer in the struct fastrpc_buf instead?
this spinlock is used for multiple lists(bufs, maps and ctx).
>
>>  err_assign:
>>  	fastrpc_req_munmap_impl(fl, buf);
>>  
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>> User request for remote heap allocation is supported using ioctl
>> interface but support for unmap is missing. This could result in
>> memory leak issues. Add unmap user request support for remote heap.
> Can this memory be in use by the remote proc?
Remote heap allocation request is only intended for audioPD. Other PDs
running on DSP are not intended to use this request.
>
>> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 51 insertions(+), 11 deletions(-)
>>

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Ekansh Gupta]

On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
>> Remote heap allocations are not organized in a maintainable manner,
>> leading to potential issues with memory management. As the remote
> Which issues? I think I have been asking this question previously.
> Please expand the commit message here.
This is mostly related to the memory clean-up and the other patch where
unmap request was added, I'll try to pull more details about the issue
scenario.
>
>> heap allocations are maintained in fl mmaps list, the allocations
>> will go away if the audio daemon process is killed but there are
> What is audio daemon process?
As audio PD on DSP is static, there is HLOS process(audio daemon) required to
attach to audio PD to fulfill it's memory and file operation requirements.

This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
daemon as it is required to take additional information and resources to audio PD
while attaching.

//Ekansh
>
>> chances that audio PD might still be using the memory. Move all
>> remote heap allocations to a dedicated list where the entries are
>> cleaned only for user requests and subsystem shutdown.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++----------
>>  1 file changed, 72 insertions(+), 21 deletions(-)
>>

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Srinivas Kandagatla]

On 5/13/25 05:28, Ekansh Gupta wrote:
> Remote heap allocations are not organized in a maintainable manner,
> leading to potential issues with memory management. As the remote
> heap allocations are maintained in fl mmaps list, the allocations
> will go away if the audio daemon process is killed but there are
> chances that audio PD might still be using the memory. Move all
> remote heap allocations to a dedicated list where the entries are
> cleaned only for user requests and subsystem shutdown.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 72 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index ca3721365ddc..d4e38b5e5e6c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -273,10 +273,12 @@ struct fastrpc_channel_ctx {
>  	struct kref refcount;
>  	/* Flag if dsp attributes are cached */
>  	bool valid_attributes;
> +	/* Flag if audio PD init mem was allocated */
> +	bool audio_init_mem;
>  	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>  	struct fastrpc_device *secure_fdevice;
>  	struct fastrpc_device *fdevice;
> -	struct fastrpc_buf *remote_heap;
> +	struct list_head rhmaps;
Other than Audiopd, do you see any other remote heaps getting added to
this list?

>  	struct list_head invoke_interrupted_mmaps;
>  	bool secure;
>  	bool unsigned_support;
> @@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>  	return false;
>  }
>  
> +static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx)
> +{
> +	struct fastrpc_buf *buf, *b;
> +	struct list_head temp_list;
> +	int err;
> +	unsigned long flags;
> +
> +	INIT_LIST_HEAD(&temp_list);
> +
> +	spin_lock_irqsave(&cctx->lock, flags);
> +	list_splice_init(&cctx->rhmaps, &temp_list);
> +	spin_unlock_irqrestore(&cctx->lock, flags);

Can you explain why do we need to do this?

> +
> +	list_for_each_entry_safe(buf, b, &temp_list, node) {> +		if (cctx->vmcount) {
> +			u64 src_perms = 0;
> +			struct qcom_scm_vmperm dst_perms;
> +			u32 i;
> +
> +			for (i = 0; i < cctx->vmcount; i++)
> +				src_perms |= BIT(cctx->vmperms[i].vmid);
> +
> +			dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> +			dst_perms.perm = QCOM_SCM_PERM_RWX;
> +			err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> +						  &src_perms, &dst_perms, 1);
> +			if (err)
> +				continue;

Memory leak here.

> +		}
> +		fastrpc_buf_free(buf);
> +	}
> +}
> +

--srini

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Srinivas Kandagatla]

On 5/13/25 05:28, Ekansh Gupta wrote:
> The initially allocated memory is not properly included in the pool,
> leading to potential issues with memory management. Set the number
> of pages to one to ensure that the initially allocated memory is
> correctly added to the Audio PD memory pool.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d4e38b5e5e6c..b629e24f00bc 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  		goto err;
>  	}
>  
> +	inbuf.client_id = fl->client_id;
> +	inbuf.namelen = init.namelen;

inbuf is not used till the invoke call, why are we moving these two
lines here?

> +	inbuf.pageslen = 0;



>  	if (!fl->cctx->audio_init_mem) {
>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>  						&buf);
> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
>  			fl->cctx->audio_init_mem = true;
> +			inbuf.pageslen = 1;
>  		}
>  	}
>  
> -	inbuf.client_id = fl->client_id;
> -	inbuf.namelen = init.namelen;
> -	inbuf.pageslen = 0;
>  	fl->pd = USER_PD;
>  
>  	args[0].ptr = (u64)(uintptr_t)&inbuf;

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Dmitry Baryshkov]

On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote:
> 
> 
> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
> > On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
> >> Remote heap allocations are not organized in a maintainable manner,
> >> leading to potential issues with memory management. As the remote
> > Which issues? I think I have been asking this question previously.
> > Please expand the commit message here.
> This is mostly related to the memory clean-up and the other patch where
> unmap request was added, I'll try to pull more details about the issue
> scenario.

Thanks.

> >
> >> heap allocations are maintained in fl mmaps list, the allocations
> >> will go away if the audio daemon process is killed but there are
> > What is audio daemon process?
> As audio PD on DSP is static, there is HLOS process(audio daemon) required to
> attach to audio PD to fulfill it's memory and file operation requirements.
> 
> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
> daemon as it is required to take additional information and resources to audio PD
> while attaching.

I find it a little bit strange to see 'required' here, while we have
working audio setup on all up platforms up to and including SM8750
without any additional daemons. This is the primary reason for my
question: what is it, why is it necessary, when is it necessary, etc.

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Dmitry Baryshkov]

On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote:
> 
> 
> On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote:
> > On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote:
> >> The initially allocated memory is not properly included in the pool,
> >> leading to potential issues with memory management. Set the number
> > What is 'properly'? Which issues?
> 
> inbuf.pageslen is getting updated to 1 in case buffer is allocated,

Is it a flag or some page count?

> which only
> happens if (!fl->cctx->audio_init_mem).

You are describing patch behaviour.

> 
> Till now pageslen is always 0 irrespective of whether the memory is allocated
> or not due to which audio PD is never able to use this memory.

and the is current behaviour. So this answers the first question.
'properly'. Now, the second quesiton. 'Which issues?'

> 
> I'll update this to the commit in the next spin.
> 
> >
> >> of pages to one to ensure that the initially allocated memory is
> >> correctly added to the Audio PD memory pool.
> >>
> >> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> >> Cc: stable@kernel.org
> >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index d4e38b5e5e6c..b629e24f00bc 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  		goto err;
> >>  	}
> >>  
> >> +	inbuf.client_id = fl->client_id;
> >> +	inbuf.namelen = init.namelen;
> >> +	inbuf.pageslen = 0;
> >>  	if (!fl->cctx->audio_init_mem) {
> >>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> >>  						&buf);
> >> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
> >>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
> >>  			fl->cctx->audio_init_mem = true;
> >> +			inbuf.pageslen = 1;
> >>  		}
> >>  	}
> >>  
> >> -	inbuf.client_id = fl->client_id;
> >> -	inbuf.namelen = init.namelen;
> >> -	inbuf.pageslen = 0;
> >>  	fl->pd = USER_PD;
> >>  
> >>  	args[0].ptr = (u64)(uintptr_t)&inbuf;
> >> -- 
> >> 2.34.1
> >>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] misc: fastrpc: Remove buffer from list prior to unmap operation

[Dmitry Baryshkov]

On Mon, May 19, 2025 at 04:26:27PM +0530, Ekansh Gupta wrote:
> 
> 
> On 5/19/2025 3:50 PM, Dmitry Baryshkov wrote:
> > On Tue, May 13, 2025 at 09:58:24AM +0530, Ekansh Gupta wrote:
> >> fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
> >> getting removed from the list after it is unmapped from DSP. This can
> >> create potential race conditions if any other thread removes the entry
> >> from list while unmap operation is ongoing. Remove the entry before
> >> calling unmap operation.
> >>
> >> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
> >> Cc: stable@kernel.org
> >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 29 ++++++++++++++++++++++-------
> >>  1 file changed, 22 insertions(+), 7 deletions(-)
> >>

> >
> >> +err_copy:
> >> +	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> >> +		spin_lock_irqsave(&fl->cctx->lock, flags);
> >> +		list_del(&buf->node);
> >> +		spin_unlock_irqrestore(&fl->cctx->lock, flags);
> >> +	} else {
> >> +		spin_lock(&fl->lock);
> >> +		list_del(&buf->node);
> >> +		spin_unlock(&fl->lock);
> >> +	}
> > Can we store the spinlock pointer in the struct fastrpc_buf instead?
> this spinlock is used for multiple lists(bufs, maps and ctx).

pointer, not a spinlock itself.

> >
> >>  err_assign:
> >>  	fastrpc_req_munmap_impl(fl, buf);
> >>  
> >> -- 
> >> 2.34.1
> >>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
> 
> 
> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> > On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> >> User request for remote heap allocation is supported using ioctl
> >> interface but support for unmap is missing. This could result in
> >> memory leak issues. Add unmap user request support for remote heap.
> > Can this memory be in use by the remote proc?
> Remote heap allocation request is only intended for audioPD. Other PDs
> running on DSP are not intended to use this request.

'Intended'. That's fine. I asked a different question: _can_ it be in
use? What happens if userspace by mistake tries to unmap memory too
early? Or if it happens intentionally, at some specific time during
work.

> >
> >> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool")
> >> Cc: stable@kernel.org
> >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 51 insertions(+), 11 deletions(-)
> >>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Ekansh Gupta]

On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote:
> On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote:
>>
>> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
>>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
>>>> Remote heap allocations are not organized in a maintainable manner,
>>>> leading to potential issues with memory management. As the remote
>>> Which issues? I think I have been asking this question previously.
>>> Please expand the commit message here.
>> This is mostly related to the memory clean-up and the other patch where
>> unmap request was added, I'll try to pull more details about the issue
>> scenario.
> Thanks.
>
>>>> heap allocations are maintained in fl mmaps list, the allocations
>>>> will go away if the audio daemon process is killed but there are
>>> What is audio daemon process?
>> As audio PD on DSP is static, there is HLOS process(audio daemon) required to
>> attach to audio PD to fulfill it's memory and file operation requirements.
>>
>> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
>> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
>> daemon as it is required to take additional information and resources to audio PD
>> while attaching.
> I find it a little bit strange to see 'required' here, while we have
> working audio setup on all up platforms up to and including SM8750
> without any additional daemons. This is the primary reason for my
> question: what is it, why is it necessary, when is it necessary, etc.

This daemon is critical to facilitate dynamic loading and memory
requirement for audio PD(running on DSP for audio processing). Even
for audio testing on SM8750, I believe Alexey was enabling this daemon.

What is it?
- HLOS process to attached to audio PD to fulfill the requirements that
cannot be met by DSP alone(like file operations, memory etc.)

Why is it necessary?
- There are limitation on DSP for which the PD requirements needs to be
taken to HLOS. For example, DSP does not have it's own file system, so
any file operation request it PD(say for dynamic loading) needs to be
taken to HLOS(using listener/reverse calls) and is fulfilled there.
Similarly memory requirement is another example.

When is it necessary?
- When audio PD needs to perform any task that requires HLOS relying
operations like dynamic loading etc.

>

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Ekansh Gupta]

On 5/19/2025 7:01 PM, Dmitry Baryshkov wrote:
> On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote:
>>
>> On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote:
>>> On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote:
>>>> The initially allocated memory is not properly included in the pool,
>>>> leading to potential issues with memory management. Set the number
>>> What is 'properly'? Which issues?
>> inbuf.pageslen is getting updated to 1 in case buffer is allocated,
> Is it a flag or some page count?

This is page count, based on this count, DSP with add memory to audioPD
pool. If it is 0, the memory is not added.

>
>> which only
>> happens if (!fl->cctx->audio_init_mem).
> You are describing patch behaviour.
>
>> Till now pageslen is always 0 irrespective of whether the memory is allocated
>> or not due to which audio PD is never able to use this memory.
> and the is current behaviour. So this answers the first question.
> 'properly'. Now, the second quesiton. 'Which issues?'

The issues is actually memory leak as the initial memory is never
used by audio PD and it will immediately make a remote heap request
as no memory is added to the pool initially.

I'll capture this also in the commit text.

>
>> I'll update this to the commit in the next spin.
>>
>>>> of pages to one to ensure that the initially allocated memory is
>>>> correctly added to the Audio PD memory pool.
>>>>
>>>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>>>> Cc: stable@kernel.org
>>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index d4e38b5e5e6c..b629e24f00bc 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>  		goto err;
>>>>  	}
>>>>  
>>>> +	inbuf.client_id = fl->client_id;
>>>> +	inbuf.namelen = init.namelen;
>>>> +	inbuf.pageslen = 0;
>>>>  	if (!fl->cctx->audio_init_mem) {
>>>>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>>>>  						&buf);
>>>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
>>>>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
>>>>  			fl->cctx->audio_init_mem = true;
>>>> +			inbuf.pageslen = 1;
>>>>  		}
>>>>  	}
>>>>  
>>>> -	inbuf.client_id = fl->client_id;
>>>> -	inbuf.namelen = init.namelen;
>>>> -	inbuf.pageslen = 0;
>>>>  	fl->pd = USER_PD;
>>>>  
>>>>  	args[0].ptr = (u64)(uintptr_t)&inbuf;
>>>> -- 
>>>> 2.34.1
>>>>

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
>>
>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>>>> User request for remote heap allocation is supported using ioctl
>>>> interface but support for unmap is missing. This could result in
>>>> memory leak issues. Add unmap user request support for remote heap.
>>> Can this memory be in use by the remote proc?
>> Remote heap allocation request is only intended for audioPD. Other PDs
>> running on DSP are not intended to use this request.
> 'Intended'. That's fine. I asked a different question: _can_ it be in
> use? What happens if userspace by mistake tries to unmap memory too
> early? Or if it happens intentionally, at some specific time during
> work.

If the unmap is restricted to audio daemon, then the unmap will only
happen if the remoteproc is no longer using this memory.

But without this restriction, yes it possible that some userspace process
calls unmap which tries to move the ownership back to HLOS which the
remoteproc is still using the memory. This might lead to memory access
problems.

>
>>>> Fixes: 532ad70c6d449 ("misc: fastrpc: Add mmap request assigning for static PD pool")
>>>> Cc: stable@kernel.org
>>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c | 62 ++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 51 insertions(+), 11 deletions(-)
>>>>

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Ekansh Gupta]

On 5/19/2025 5:05 PM, Srinivas Kandagatla wrote:
> On 5/13/25 05:28, Ekansh Gupta wrote:
>> Remote heap allocations are not organized in a maintainable manner,
>> leading to potential issues with memory management. As the remote
>> heap allocations are maintained in fl mmaps list, the allocations
>> will go away if the audio daemon process is killed but there are
>> chances that audio PD might still be using the memory. Move all
>> remote heap allocations to a dedicated list where the entries are
>> cleaned only for user requests and subsystem shutdown.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++----------
>>  1 file changed, 72 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index ca3721365ddc..d4e38b5e5e6c 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -273,10 +273,12 @@ struct fastrpc_channel_ctx {
>>  	struct kref refcount;
>>  	/* Flag if dsp attributes are cached */
>>  	bool valid_attributes;
>> +	/* Flag if audio PD init mem was allocated */
>> +	bool audio_init_mem;
>>  	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>>  	struct fastrpc_device *secure_fdevice;
>>  	struct fastrpc_device *fdevice;
>> -	struct fastrpc_buf *remote_heap;
>> +	struct list_head rhmaps;
> Other than Audiopd, do you see any other remote heaps getting added to
> this list?

With current implementation it is possible but that is not correct, I
will include a patch to restrict remote heap map and unmap requests to
audio daemon only.

>
>>  	struct list_head invoke_interrupted_mmaps;
>>  	bool secure;
>>  	bool unsigned_support;
>> @@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>>  	return false;
>>  }
>>  
>> +static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx)
>> +{
>> +	struct fastrpc_buf *buf, *b;
>> +	struct list_head temp_list;
>> +	int err;
>> +	unsigned long flags;
>> +
>> +	INIT_LIST_HEAD(&temp_list);
>> +
>> +	spin_lock_irqsave(&cctx->lock, flags);
>> +	list_splice_init(&cctx->rhmaps, &temp_list);
>> +	spin_unlock_irqrestore(&cctx->lock, flags);
> Can you explain why do we need to do this?

To avoid any locking issue. While clean-up, I'm replacing the rhmaps
list with an empty one under a lock and cleaning up the list without
any more locking.

>
>> +
>> +	list_for_each_entry_safe(buf, b, &temp_list, node) {> +		if (cctx->vmcount) {
>> +			u64 src_perms = 0;
>> +			struct qcom_scm_vmperm dst_perms;
>> +			u32 i;
>> +
>> +			for (i = 0; i < cctx->vmcount; i++)
>> +				src_perms |= BIT(cctx->vmperms[i].vmid);
>> +
>> +			dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> +			dst_perms.perm = QCOM_SCM_PERM_RWX;
>> +			err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> +						  &src_perms, &dst_perms, 1);
>> +			if (err)
>> +				continue;
> Memory leak here.

I don't see any better way to handle the failure here as the clean-up
is called when the channel is going down and there won't be any
way to free this memory if qcom_scm call fails?

Do you see any better way to address this? Or should I add a comment
highlighting this limitation?

>
>> +		}
>> +		fastrpc_buf_free(buf);
>> +	}
>> +}
>> +
> --srini

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Ekansh Gupta]

On 5/19/2025 5:11 PM, Srinivas Kandagatla wrote:
> On 5/13/25 05:28, Ekansh Gupta wrote:
>> The initially allocated memory is not properly included in the pool,
>> leading to potential issues with memory management. Set the number
>> of pages to one to ensure that the initially allocated memory is
>> correctly added to the Audio PD memory pool.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>>  drivers/misc/fastrpc.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index d4e38b5e5e6c..b629e24f00bc 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  		goto err;
>>  	}
>>  
>> +	inbuf.client_id = fl->client_id;
>> +	inbuf.namelen = init.namelen;
> inbuf is not used till the invoke call, why are we moving these two
> lines here?

I just moved it above so that the pagelen is updated properly.

Would you prefer storing pagelen to some local variable and then eventually
assigning it to inbuf.pagelen before making the invoke call?

>
>> +	inbuf.pageslen = 0;
>
>
>>  	if (!fl->cctx->audio_init_mem) {
>>  		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>>  						&buf);
>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  			list_add_tail(&buf->node, &fl->cctx->rhmaps);
>>  			spin_unlock_irqrestore(&fl->cctx->lock, flags);
>>  			fl->cctx->audio_init_mem = true;
>> +			inbuf.pageslen = 1;
>>  		}
>>  	}
>>  
>> -	inbuf.client_id = fl->client_id;
>> -	inbuf.namelen = init.namelen;
>> -	inbuf.pageslen = 0;
>>  	fl->pd = USER_PD;
>>  
>>  	args[0].ptr = (u64)(uintptr_t)&inbuf;

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Dmitry Baryshkov]

On Thu, 22 May 2025 at 07:54, Ekansh Gupta
<ekansh.gupta@oss.qualcomm.com> wrote:
>
>
>
> On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote:
> > On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote:
> >>
> >> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
> >>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
> >>>> Remote heap allocations are not organized in a maintainable manner,
> >>>> leading to potential issues with memory management. As the remote
> >>> Which issues? I think I have been asking this question previously.
> >>> Please expand the commit message here.
> >> This is mostly related to the memory clean-up and the other patch where
> >> unmap request was added, I'll try to pull more details about the issue
> >> scenario.
> > Thanks.
> >
> >>>> heap allocations are maintained in fl mmaps list, the allocations
> >>>> will go away if the audio daemon process is killed but there are
> >>> What is audio daemon process?
> >> As audio PD on DSP is static, there is HLOS process(audio daemon) required to
> >> attach to audio PD to fulfill it's memory and file operation requirements.
> >>
> >> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
> >> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
> >> daemon as it is required to take additional information and resources to audio PD
> >> while attaching.
> > I find it a little bit strange to see 'required' here, while we have
> > working audio setup on all up platforms up to and including SM8750
> > without any additional daemons. This is the primary reason for my
> > question: what is it, why is it necessary, when is it necessary, etc.
>
> This daemon is critical to facilitate dynamic loading and memory
> requirement for audio PD(running on DSP for audio processing). Even
> for audio testing on SM8750, I believe Alexey was enabling this daemon.

Could you please point out the daemon sources?

As far as I remember, we didn't need it on any of the platforms up to
and including SM8650, that's why I'm asking.

>
> What is it?
> - HLOS process to attached to audio PD to fulfill the requirements that
> cannot be met by DSP alone(like file operations, memory etc.)
>
> Why is it necessary?
> - There are limitation on DSP for which the PD requirements needs to be
> taken to HLOS. For example, DSP does not have it's own file system, so
> any file operation request it PD(say for dynamic loading) needs to be
> taken to HLOS(using listener/reverse calls) and is fulfilled there.
> Similarly memory requirement is another example.
>
> When is it necessary?
> - When audio PD needs to perform any task that requires HLOS relying
> operations like dynamic loading etc.

This doesn't exactly answer the question. I can play and capture audio
on most of the platforms that I tested without using extra daemon. So,
when is it necessary?


-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Dmitry Baryshkov]

On Thu, 22 May 2025 at 07:58, Ekansh Gupta
<ekansh.gupta@oss.qualcomm.com> wrote:
>
>
>
> On 5/19/2025 7:01 PM, Dmitry Baryshkov wrote:
> > On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote:
> >>
> >> On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote:
> >>> On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote:
> >>>> The initially allocated memory is not properly included in the pool,
> >>>> leading to potential issues with memory management. Set the number
> >>> What is 'properly'? Which issues?
> >> inbuf.pageslen is getting updated to 1 in case buffer is allocated,
> > Is it a flag or some page count?
>
> This is page count,

If it is page count, then why is it '1' instead of being calculated
based on the init.memlen?

> based on this count, DSP with add memory to audioPD
> pool. If it is 0, the memory is not added.
>
> >
> >> which only
> >> happens if (!fl->cctx->audio_init_mem).
> > You are describing patch behaviour.
> >
> >> Till now pageslen is always 0 irrespective of whether the memory is allocated
> >> or not due to which audio PD is never able to use this memory.
> > and the is current behaviour. So this answers the first question.
> > 'properly'. Now, the second quesiton. 'Which issues?'
>
> The issues is actually memory leak as the initial memory is never
> used by audio PD and it will immediately make a remote heap request
> as no memory is added to the pool initially.
>
> I'll capture this also in the commit text.
>
> >
> >> I'll update this to the commit in the next spin.
> >>
> >>>> of pages to one to ensure that the initially allocated memory is
> >>>> correctly added to the Audio PD memory pool.
> >>>>
> >>>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> >>>> Cc: stable@kernel.org
> >>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> >>>> ---
> >>>>  drivers/misc/fastrpc.c | 7 ++++---
> >>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>> index d4e38b5e5e6c..b629e24f00bc 100644
> >>>> --- a/drivers/misc/fastrpc.c
> >>>> +++ b/drivers/misc/fastrpc.c
> >>>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>>>            goto err;
> >>>>    }
> >>>>
> >>>> +  inbuf.client_id = fl->client_id;
> >>>> +  inbuf.namelen = init.namelen;
> >>>> +  inbuf.pageslen = 0;
> >>>>    if (!fl->cctx->audio_init_mem) {
> >>>>            err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> >>>>                                            &buf);
> >>>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>>>                    list_add_tail(&buf->node, &fl->cctx->rhmaps);
> >>>>                    spin_unlock_irqrestore(&fl->cctx->lock, flags);
> >>>>                    fl->cctx->audio_init_mem = true;
> >>>> +                  inbuf.pageslen = 1;
> >>>>            }
> >>>>    }
> >>>>
> >>>> -  inbuf.client_id = fl->client_id;
> >>>> -  inbuf.namelen = init.namelen;
> >>>> -  inbuf.pageslen = 0;
> >>>>    fl->pd = USER_PD;
> >>>>
> >>>>    args[0].ptr = (u64)(uintptr_t)&inbuf;
> >>>> --
> >>>> 2.34.1
> >>>>
>


-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Thu, 22 May 2025 at 08:01, Ekansh Gupta
<ekansh.gupta@oss.qualcomm.com> wrote:
>
>
>
> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
> > On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
> >>
> >> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> >>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> >>>> User request for remote heap allocation is supported using ioctl
> >>>> interface but support for unmap is missing. This could result in
> >>>> memory leak issues. Add unmap user request support for remote heap.
> >>> Can this memory be in use by the remote proc?
> >> Remote heap allocation request is only intended for audioPD. Other PDs
> >> running on DSP are not intended to use this request.
> > 'Intended'. That's fine. I asked a different question: _can_ it be in
> > use? What happens if userspace by mistake tries to unmap memory too
> > early? Or if it happens intentionally, at some specific time during
> > work.
>
> If the unmap is restricted to audio daemon, then the unmap will only
> happen if the remoteproc is no longer using this memory.
>
> But without this restriction, yes it possible that some userspace process
> calls unmap which tries to move the ownership back to HLOS which the
> remoteproc is still using the memory. This might lead to memory access
> problems.

This needs to be fixed in the driver. We need to track which memory is
being used by the remoteproc and unmap it once remoteproc stops using
it, without additional userspace intervention.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Ekansh Gupta]

On 5/22/2025 5:39 PM, Dmitry Baryshkov wrote:
> On Thu, 22 May 2025 at 07:54, Ekansh Gupta
> <ekansh.gupta@oss.qualcomm.com> wrote:
>>
>>
>> On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote:
>>> On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote:
>>>> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
>>>>>> Remote heap allocations are not organized in a maintainable manner,
>>>>>> leading to potential issues with memory management. As the remote
>>>>> Which issues? I think I have been asking this question previously.
>>>>> Please expand the commit message here.
>>>> This is mostly related to the memory clean-up and the other patch where
>>>> unmap request was added, I'll try to pull more details about the issue
>>>> scenario.
>>> Thanks.
>>>
>>>>>> heap allocations are maintained in fl mmaps list, the allocations
>>>>>> will go away if the audio daemon process is killed but there are
>>>>> What is audio daemon process?
>>>> As audio PD on DSP is static, there is HLOS process(audio daemon) required to
>>>> attach to audio PD to fulfill it's memory and file operation requirements.
>>>>
>>>> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
>>>> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
>>>> daemon as it is required to take additional information and resources to audio PD
>>>> while attaching.
>>> I find it a little bit strange to see 'required' here, while we have
>>> working audio setup on all up platforms up to and including SM8750
>>> without any additional daemons. This is the primary reason for my
>>> question: what is it, why is it necessary, when is it necessary, etc.
>> This daemon is critical to facilitate dynamic loading and memory
>> requirement for audio PD(running on DSP for audio processing). Even
>> for audio testing on SM8750, I believe Alexey was enabling this daemon.
> Could you please point out the daemon sources?
>
> As far as I remember, we didn't need it on any of the platforms up to
> and including SM8650, that's why I'm asking.
This source was used for testing audio use case on SM8750:
https://github.com/quic/fastrpc/blob/development/src/adsprpcd.c

The use case tried by Alexey as per my knowledge is audio playback where dynamic
loading was needed but he can give more details on the use case.

He was observing failures and panic which got resolved after picking this patch series.
>
>> What is it?
>> - HLOS process to attached to audio PD to fulfill the requirements that
>> cannot be met by DSP alone(like file operations, memory etc.)
>>
>> Why is it necessary?
>> - There are limitation on DSP for which the PD requirements needs to be
>> taken to HLOS. For example, DSP does not have it's own file system, so
>> any file operation request it PD(say for dynamic loading) needs to be
>> taken to HLOS(using listener/reverse calls) and is fulfilled there.
>> Similarly memory requirement is another example.
>>
>> When is it necessary?
>> - When audio PD needs to perform any task that requires HLOS relying
>> operations like dynamic loading etc.
> This doesn't exactly answer the question. I can play and capture audio
> on most of the platforms that I tested without using extra daemon. So,
> when is it necessary?
For use case details, I'll let Alexey comment here.

The daemons major requirement is to facilitate any dynamic loading or memory
requirements from DSP audio PD. The daemons are already supported for
different types of static PDs to facilitate these requirements(fops and memory).

>

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
> <ekansh.gupta@oss.qualcomm.com> wrote:
>>
>>
>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>>>>>> User request for remote heap allocation is supported using ioctl
>>>>>> interface but support for unmap is missing. This could result in
>>>>>> memory leak issues. Add unmap user request support for remote heap.
>>>>> Can this memory be in use by the remote proc?
>>>> Remote heap allocation request is only intended for audioPD. Other PDs
>>>> running on DSP are not intended to use this request.
>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
>>> use? What happens if userspace by mistake tries to unmap memory too
>>> early? Or if it happens intentionally, at some specific time during
>>> work.
>> If the unmap is restricted to audio daemon, then the unmap will only
>> happen if the remoteproc is no longer using this memory.
>>
>> But without this restriction, yes it possible that some userspace process
>> calls unmap which tries to move the ownership back to HLOS which the
>> remoteproc is still using the memory. This might lead to memory access
>> problems.
> This needs to be fixed in the driver. We need to track which memory is
> being used by the remoteproc and unmap it once remoteproc stops using
> it, without additional userspace intervention.
If it's the audio daemon which is requesting for unmap then it basically means that
the remoteproc is no longer using the memory. Audio PD can request for both grow
and shrink operations for it's dedicated heap. The case of grow is already supported
from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
memory) is not yet available. This memory is more specific to audio PD rather than
complete remoteproc.

If we have to control this completely from driver then I see a problem in freeing/unmapping
the memory when the PD is no longer using the memory.
>

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
> 
> 
> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
> > On Thu, 22 May 2025 at 08:01, Ekansh Gupta
> > <ekansh.gupta@oss.qualcomm.com> wrote:
> >>
> >>
> >> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
> >>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
> >>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> >>>>>> User request for remote heap allocation is supported using ioctl
> >>>>>> interface but support for unmap is missing. This could result in
> >>>>>> memory leak issues. Add unmap user request support for remote heap.
> >>>>> Can this memory be in use by the remote proc?
> >>>> Remote heap allocation request is only intended for audioPD. Other PDs
> >>>> running on DSP are not intended to use this request.
> >>> 'Intended'. That's fine. I asked a different question: _can_ it be in
> >>> use? What happens if userspace by mistake tries to unmap memory too
> >>> early? Or if it happens intentionally, at some specific time during
> >>> work.
> >> If the unmap is restricted to audio daemon, then the unmap will only
> >> happen if the remoteproc is no longer using this memory.
> >>
> >> But without this restriction, yes it possible that some userspace process
> >> calls unmap which tries to move the ownership back to HLOS which the
> >> remoteproc is still using the memory. This might lead to memory access
> >> problems.
> > This needs to be fixed in the driver. We need to track which memory is
> > being used by the remoteproc and unmap it once remoteproc stops using
> > it, without additional userspace intervention.
> If it's the audio daemon which is requesting for unmap then it basically means that
> the remoteproc is no longer using the memory. Audio PD can request for both grow
> and shrink operations for it's dedicated heap. The case of grow is already supported
> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
> memory) is not yet available. This memory is more specific to audio PD rather than
> complete remoteproc.
> 
> If we have to control this completely from driver then I see a problem in freeing/unmapping
> the memory when the PD is no longer using the memory.

What happens if userspace requests to free the memory that is still in
use by the PD

How does PD signal the memory is no longer in use?

> >
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 6/12/2025 1:35 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
>>
>> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
>>> <ekansh.gupta@oss.qualcomm.com> wrote:
>>>>
>>>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
>>>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>>>>>>>> User request for remote heap allocation is supported using ioctl
>>>>>>>> interface but support for unmap is missing. This could result in
>>>>>>>> memory leak issues. Add unmap user request support for remote heap.
>>>>>>> Can this memory be in use by the remote proc?
>>>>>> Remote heap allocation request is only intended for audioPD. Other PDs
>>>>>> running on DSP are not intended to use this request.
>>>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
>>>>> use? What happens if userspace by mistake tries to unmap memory too
>>>>> early? Or if it happens intentionally, at some specific time during
>>>>> work.
>>>> If the unmap is restricted to audio daemon, then the unmap will only
>>>> happen if the remoteproc is no longer using this memory.
>>>>
>>>> But without this restriction, yes it possible that some userspace process
>>>> calls unmap which tries to move the ownership back to HLOS which the
>>>> remoteproc is still using the memory. This might lead to memory access
>>>> problems.
>>> This needs to be fixed in the driver. We need to track which memory is
>>> being used by the remoteproc and unmap it once remoteproc stops using
>>> it, without additional userspace intervention.
>> If it's the audio daemon which is requesting for unmap then it basically means that
>> the remoteproc is no longer using the memory. Audio PD can request for both grow
>> and shrink operations for it's dedicated heap. The case of grow is already supported
>> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
>> memory) is not yet available. This memory is more specific to audio PD rather than
>> complete remoteproc.
>>
>> If we have to control this completely from driver then I see a problem in freeing/unmapping
>> the memory when the PD is no longer using the memory.
> What happens if userspace requests to free the memory that is still in
> use by the PD
I understand your point, for this I was thinking to limit the unmap functionality to the process
that is already attached to audio PD on DSP, no other process will be able to map/unmap this
memory from userspace.

>
> How does PD signal the memory is no longer in use?
PD makes a reverse fastrpc request[1] to unmap the memory when it is no longer used.

[1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231
>

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Thu, Jun 12, 2025 at 03:02:52PM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/12/2025 1:35 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
> >>
> >> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
> >>> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
> >>> <ekansh.gupta@oss.qualcomm.com> wrote:
> >>>>
> >>>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
> >>>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> >>>>>>>> User request for remote heap allocation is supported using ioctl
> >>>>>>>> interface but support for unmap is missing. This could result in
> >>>>>>>> memory leak issues. Add unmap user request support for remote heap.
> >>>>>>> Can this memory be in use by the remote proc?
> >>>>>> Remote heap allocation request is only intended for audioPD. Other PDs
> >>>>>> running on DSP are not intended to use this request.
> >>>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
> >>>>> use? What happens if userspace by mistake tries to unmap memory too
> >>>>> early? Or if it happens intentionally, at some specific time during
> >>>>> work.
> >>>> If the unmap is restricted to audio daemon, then the unmap will only
> >>>> happen if the remoteproc is no longer using this memory.
> >>>>
> >>>> But without this restriction, yes it possible that some userspace process
> >>>> calls unmap which tries to move the ownership back to HLOS which the
> >>>> remoteproc is still using the memory. This might lead to memory access
> >>>> problems.
> >>> This needs to be fixed in the driver. We need to track which memory is
> >>> being used by the remoteproc and unmap it once remoteproc stops using
> >>> it, without additional userspace intervention.
> >> If it's the audio daemon which is requesting for unmap then it basically means that
> >> the remoteproc is no longer using the memory. Audio PD can request for both grow
> >> and shrink operations for it's dedicated heap. The case of grow is already supported
> >> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
> >> memory) is not yet available. This memory is more specific to audio PD rather than
> >> complete remoteproc.
> >>
> >> If we have to control this completely from driver then I see a problem in freeing/unmapping
> >> the memory when the PD is no longer using the memory.
> > What happens if userspace requests to free the memory that is still in
> > use by the PD
> I understand your point, for this I was thinking to limit the unmap functionality to the process
> that is already attached to audio PD on DSP, no other process will be able to map/unmap this
> memory from userspace.

Ugh... and what if the adsprpcd misbehaves?

> >
> > How does PD signal the memory is no longer in use?
> PD makes a reverse fastrpc request[1] to unmap the memory when it is no longer used.

I don't see how this can be made robust. I fear that the only way would
be to unmap the memory only on audio PD restart / shutdown. Such
requests should never leave the kernel.

Moreover, the payload should not be trusted, however you don't validate
the length that you've got from the remote side.

> 
> [1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231
> >

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/5] misc: fastrpc: Move all remote heap allocations to a new list

[Dmitry Baryshkov]

On 12/06/2025 08:13, Ekansh Gupta wrote:
> 
> 
> On 5/22/2025 5:39 PM, Dmitry Baryshkov wrote:
>> On Thu, 22 May 2025 at 07:54, Ekansh Gupta
>> <ekansh.gupta@oss.qualcomm.com> wrote:
>>>
>>>
>>> On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote:
>>>> On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote:
>>>>> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote:
>>>>>>> Remote heap allocations are not organized in a maintainable manner,
>>>>>>> leading to potential issues with memory management. As the remote
>>>>>> Which issues? I think I have been asking this question previously.
>>>>>> Please expand the commit message here.
>>>>> This is mostly related to the memory clean-up and the other patch where
>>>>> unmap request was added, I'll try to pull more details about the issue
>>>>> scenario.
>>>> Thanks.
>>>>
>>>>>>> heap allocations are maintained in fl mmaps list, the allocations
>>>>>>> will go away if the audio daemon process is killed but there are
>>>>>> What is audio daemon process?
>>>>> As audio PD on DSP is static, there is HLOS process(audio daemon) required to
>>>>> attach to audio PD to fulfill it's memory and file operation requirements.
>>>>>
>>>>> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or
>>>>> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio
>>>>> daemon as it is required to take additional information and resources to audio PD
>>>>> while attaching.
>>>> I find it a little bit strange to see 'required' here, while we have
>>>> working audio setup on all up platforms up to and including SM8750
>>>> without any additional daemons. This is the primary reason for my
>>>> question: what is it, why is it necessary, when is it necessary, etc.
>>> This daemon is critical to facilitate dynamic loading and memory
>>> requirement for audio PD(running on DSP for audio processing). Even
>>> for audio testing on SM8750, I believe Alexey was enabling this daemon.
>> Could you please point out the daemon sources?
>>
>> As far as I remember, we didn't need it on any of the platforms up to
>> and including SM8650, that's why I'm asking.
> This source was used for testing audio use case on SM8750:
> https://github.com/quic/fastrpc/blob/development/src/adsprpcd.c
> 
> The use case tried by Alexey as per my knowledge is audio playback where dynamic
> loading was needed but he can give more details on the use case.

Okay.
You need to be more specific in the commit messages.

- It is a normal adsprpcd.
- It is only required for compressed audio playback.

> 
> He was observing failures and panic which got resolved after picking this patch series.

Which failures? Panic in which driver?

>>
>>> What is it?
>>> - HLOS process to attached to audio PD to fulfill the requirements that
>>> cannot be met by DSP alone(like file operations, memory etc.)
>>>
>>> Why is it necessary?
>>> - There are limitation on DSP for which the PD requirements needs to be
>>> taken to HLOS. For example, DSP does not have it's own file system, so
>>> any file operation request it PD(say for dynamic loading) needs to be
>>> taken to HLOS(using listener/reverse calls) and is fulfilled there.
>>> Similarly memory requirement is another example.
>>>
>>> When is it necessary?
>>> - When audio PD needs to perform any task that requires HLOS relying
>>> operations like dynamic loading etc.
>> This doesn't exactly answer the question. I can play and capture audio
>> on most of the platforms that I tested without using extra daemon. So,
>> when is it necessary?
> For use case details, I'll let Alexey comment here.
> 
> The daemons major requirement is to facilitate any dynamic loading or memory
> requirements from DSP audio PD. The daemons are already supported for
> different types of static PDs to facilitate these requirements(fops and memory).

So... compressed audio only or a normal playback / capture too?

> 
>>
> 


-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 6/12/2025 3:54 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 12, 2025 at 03:02:52PM +0530, Ekansh Gupta wrote:
>>
>> On 6/12/2025 1:35 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
>>>> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
>>>>> <ekansh.gupta@oss.qualcomm.com> wrote:
>>>>>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
>>>>>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>>>>>>>>>> User request for remote heap allocation is supported using ioctl
>>>>>>>>>> interface but support for unmap is missing. This could result in
>>>>>>>>>> memory leak issues. Add unmap user request support for remote heap.
>>>>>>>>> Can this memory be in use by the remote proc?
>>>>>>>> Remote heap allocation request is only intended for audioPD. Other PDs
>>>>>>>> running on DSP are not intended to use this request.
>>>>>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
>>>>>>> use? What happens if userspace by mistake tries to unmap memory too
>>>>>>> early? Or if it happens intentionally, at some specific time during
>>>>>>> work.
>>>>>> If the unmap is restricted to audio daemon, then the unmap will only
>>>>>> happen if the remoteproc is no longer using this memory.
>>>>>>
>>>>>> But without this restriction, yes it possible that some userspace process
>>>>>> calls unmap which tries to move the ownership back to HLOS which the
>>>>>> remoteproc is still using the memory. This might lead to memory access
>>>>>> problems.
>>>>> This needs to be fixed in the driver. We need to track which memory is
>>>>> being used by the remoteproc and unmap it once remoteproc stops using
>>>>> it, without additional userspace intervention.
>>>> If it's the audio daemon which is requesting for unmap then it basically means that
>>>> the remoteproc is no longer using the memory. Audio PD can request for both grow
>>>> and shrink operations for it's dedicated heap. The case of grow is already supported
>>>> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
>>>> memory) is not yet available. This memory is more specific to audio PD rather than
>>>> complete remoteproc.
>>>>
>>>> If we have to control this completely from driver then I see a problem in freeing/unmapping
>>>> the memory when the PD is no longer using the memory.
>>> What happens if userspace requests to free the memory that is still in
>>> use by the PD
>> I understand your point, for this I was thinking to limit the unmap functionality to the process
>> that is already attached to audio PD on DSP, no other process will be able to map/unmap this
>> memory from userspace.
> Ugh... and what if the adsprpcd misbehaves?
>
>>> How does PD signal the memory is no longer in use?
>> PD makes a reverse fastrpc request[1] to unmap the memory when it is no longer used.
> I don't see how this can be made robust. I fear that the only way would
> be to unmap the memory only on audio PD restart / shutdown. Such
> requests should never leave the kernel.
>
> Moreover, the payload should not be trusted, however you don't validate
> the length that you've got from the remote side.
I was thinking of giving the entire reserved memory to audio PD when
init_create_static_process is called. This way, we can avoid any need to
support grow/free request from user process and the audio PD pool on
DSP will have sufficient memory support the use cases.

This way the free can be moved to fastrpc_rpmsg_remove(When DSP
is shutting down) or during Audio PD restart(Static PD restart is not
yet supported, but clean-up can be done when PDR framework is
implemented in the future).

Do you see any drawbacks with this design?

>
>> [1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Dmitry Baryshkov]

On Wed, Jul 09, 2025 at 11:13:49AM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/12/2025 3:54 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 12, 2025 at 03:02:52PM +0530, Ekansh Gupta wrote:
> >>
> >> On 6/12/2025 1:35 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
> >>>> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
> >>>>> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
> >>>>> <ekansh.gupta@oss.qualcomm.com> wrote:
> >>>>>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
> >>>>>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
> >>>>>>>>>> User request for remote heap allocation is supported using ioctl
> >>>>>>>>>> interface but support for unmap is missing. This could result in
> >>>>>>>>>> memory leak issues. Add unmap user request support for remote heap.
> >>>>>>>>> Can this memory be in use by the remote proc?
> >>>>>>>> Remote heap allocation request is only intended for audioPD. Other PDs
> >>>>>>>> running on DSP are not intended to use this request.
> >>>>>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
> >>>>>>> use? What happens if userspace by mistake tries to unmap memory too
> >>>>>>> early? Or if it happens intentionally, at some specific time during
> >>>>>>> work.
> >>>>>> If the unmap is restricted to audio daemon, then the unmap will only
> >>>>>> happen if the remoteproc is no longer using this memory.
> >>>>>>
> >>>>>> But without this restriction, yes it possible that some userspace process
> >>>>>> calls unmap which tries to move the ownership back to HLOS which the
> >>>>>> remoteproc is still using the memory. This might lead to memory access
> >>>>>> problems.
> >>>>> This needs to be fixed in the driver. We need to track which memory is
> >>>>> being used by the remoteproc and unmap it once remoteproc stops using
> >>>>> it, without additional userspace intervention.
> >>>> If it's the audio daemon which is requesting for unmap then it basically means that
> >>>> the remoteproc is no longer using the memory. Audio PD can request for both grow
> >>>> and shrink operations for it's dedicated heap. The case of grow is already supported
> >>>> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
> >>>> memory) is not yet available. This memory is more specific to audio PD rather than
> >>>> complete remoteproc.
> >>>>
> >>>> If we have to control this completely from driver then I see a problem in freeing/unmapping
> >>>> the memory when the PD is no longer using the memory.
> >>> What happens if userspace requests to free the memory that is still in
> >>> use by the PD
> >> I understand your point, for this I was thinking to limit the unmap functionality to the process
> >> that is already attached to audio PD on DSP, no other process will be able to map/unmap this
> >> memory from userspace.
> > Ugh... and what if the adsprpcd misbehaves?
> >
> >>> How does PD signal the memory is no longer in use?
> >> PD makes a reverse fastrpc request[1] to unmap the memory when it is no longer used.
> > I don't see how this can be made robust. I fear that the only way would
> > be to unmap the memory only on audio PD restart / shutdown. Such
> > requests should never leave the kernel.
> >
> > Moreover, the payload should not be trusted, however you don't validate
> > the length that you've got from the remote side.
> I was thinking of giving the entire reserved memory to audio PD when
> init_create_static_process is called. This way, we can avoid any need to
> support grow/free request from user process and the audio PD pool on
> DSP will have sufficient memory support the use cases.
> 
> This way the free can be moved to fastrpc_rpmsg_remove(When DSP
> is shutting down) or during Audio PD restart(Static PD restart is not
> yet supported, but clean-up can be done when PDR framework is
> implemented in the future).
> 
> Do you see any drawbacks with this design?

I'm sorry for the delay in responding to your email.

I think this is a perfect idea. Can we be sure that there will be no
extra requests from the DSP?

> 
> >
> >> [1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping user-requested remote heap

[Ekansh Gupta]

On 7/19/2025 3:14 PM, Dmitry Baryshkov wrote:
> On Wed, Jul 09, 2025 at 11:13:49AM +0530, Ekansh Gupta wrote:
>>
>> On 6/12/2025 3:54 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jun 12, 2025 at 03:02:52PM +0530, Ekansh Gupta wrote:
>>>> On 6/12/2025 1:35 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Jun 12, 2025 at 10:50:10AM +0530, Ekansh Gupta wrote:
>>>>>> On 5/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, 22 May 2025 at 08:01, Ekansh Gupta
>>>>>>> <ekansh.gupta@oss.qualcomm.com> wrote:
>>>>>>>> On 5/19/2025 7:04 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Mon, May 19, 2025 at 04:28:34PM +0530, Ekansh Gupta wrote:
>>>>>>>>>> On 5/19/2025 4:22 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Tue, May 13, 2025 at 09:58:25AM +0530, Ekansh Gupta wrote:
>>>>>>>>>>>> User request for remote heap allocation is supported using ioctl
>>>>>>>>>>>> interface but support for unmap is missing. This could result in
>>>>>>>>>>>> memory leak issues. Add unmap user request support for remote heap.
>>>>>>>>>>> Can this memory be in use by the remote proc?
>>>>>>>>>> Remote heap allocation request is only intended for audioPD. Other PDs
>>>>>>>>>> running on DSP are not intended to use this request.
>>>>>>>>> 'Intended'. That's fine. I asked a different question: _can_ it be in
>>>>>>>>> use? What happens if userspace by mistake tries to unmap memory too
>>>>>>>>> early? Or if it happens intentionally, at some specific time during
>>>>>>>>> work.
>>>>>>>> If the unmap is restricted to audio daemon, then the unmap will only
>>>>>>>> happen if the remoteproc is no longer using this memory.
>>>>>>>>
>>>>>>>> But without this restriction, yes it possible that some userspace process
>>>>>>>> calls unmap which tries to move the ownership back to HLOS which the
>>>>>>>> remoteproc is still using the memory. This might lead to memory access
>>>>>>>> problems.
>>>>>>> This needs to be fixed in the driver. We need to track which memory is
>>>>>>> being used by the remoteproc and unmap it once remoteproc stops using
>>>>>>> it, without additional userspace intervention.
>>>>>> If it's the audio daemon which is requesting for unmap then it basically means that
>>>>>> the remoteproc is no longer using the memory. Audio PD can request for both grow
>>>>>> and shrink operations for it's dedicated heap. The case of grow is already supported
>>>>>> from fastrpc_req_mmap but the case of shrink(when remoteproc is no longer using the
>>>>>> memory) is not yet available. This memory is more specific to audio PD rather than
>>>>>> complete remoteproc.
>>>>>>
>>>>>> If we have to control this completely from driver then I see a problem in freeing/unmapping
>>>>>> the memory when the PD is no longer using the memory.
>>>>> What happens if userspace requests to free the memory that is still in
>>>>> use by the PD
>>>> I understand your point, for this I was thinking to limit the unmap functionality to the process
>>>> that is already attached to audio PD on DSP, no other process will be able to map/unmap this
>>>> memory from userspace.
>>> Ugh... and what if the adsprpcd misbehaves?
>>>
>>>>> How does PD signal the memory is no longer in use?
>>>> PD makes a reverse fastrpc request[1] to unmap the memory when it is no longer used.
>>> I don't see how this can be made robust. I fear that the only way would
>>> be to unmap the memory only on audio PD restart / shutdown. Such
>>> requests should never leave the kernel.
>>>
>>> Moreover, the payload should not be trusted, however you don't validate
>>> the length that you've got from the remote side.
>> I was thinking of giving the entire reserved memory to audio PD when
>> init_create_static_process is called. This way, we can avoid any need to
>> support grow/free request from user process and the audio PD pool on
>> DSP will have sufficient memory support the use cases.
>>
>> This way the free can be moved to fastrpc_rpmsg_remove(When DSP
>> is shutting down) or during Audio PD restart(Static PD restart is not
>> yet supported, but clean-up can be done when PDR framework is
>> implemented in the future).
>>
>> Do you see any drawbacks with this design?
> I'm sorry for the delay in responding to your email.
>
> I think this is a perfect idea. Can we be sure that there will be no
> extra requests from the DSP?
Thanks for the review!

With the existing design, maximum available memory from reserved_memory section
can be allocated and after that it the requests are expected to fail.

With the proposed design, as the entire memory is sent to audio PD, usually grow
request is not expected, but even if there is any grow request, it is expected to fail.

I'll send out a patch with this new design for more clear understanding.

//Ekansh
>
>>>> [1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231