Re: [PATCH v12] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>]

On 24-08-2026 14:19, Jianping Li wrote:
> 
> On 8/20/2026 1:10 PM, Ekansh Gupta wrote:
>> On 14-08-2026 15:49, Jianping Li wrote:
>>> Allocating and freeing Audio PD memory from userspace is unsafe because
>>> the kernel cannot reliably determine when the DSP has finished using the
>>> memory. Userspace may free buffers while they are still in use by the DSP,
>>> and remote free requests cannot be safely trusted.
>>>
>>> Additionally, the current implementation allows userspace to repeatedly
>>> grow the Audio PD heap, but does not support shrinking it. This can lead
>>> to unbounded memory usage over time, effectively causing a memory leak.
>>>
>>> Fix this by allocating the entire Audio PD reserved-memory region during
>>> rpmsg probe and tying its lifetime to the rpmsg channel. This removes
>>> userspace-controlled alloc/free and ensures that memory is reclaimed only
>>> when the DSP process is torn down.
>>>
>>> The reserved-memory region is now mandatory for the Audio PD domain.
>>> Rather than failing rpmsg probe when it is missing, validate it in
>>> fastrpc_init_create_static_process() and reject only the static-process
>>> creation. This keeps the fastrpc device probing for all other domains
>>> even on a misconfigured device tree.
>>>
>>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>>> Cc: stable@kernel.org
>>> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
>>> ---
>>> Patch [v11]: https://lore.kernel.org/all/20260731093210.473-1-
>>> jianping.li@oss.qualcomm.com/
>>>
>>> Changes in v12:
>>>  - Do not fail rpmsg probe when the reserved-memory region is missing,
>>>    validate the region in fastrpc_init_create_static_process() instead,
>>>    so probe keeps working for all domains.
>>>  - Add fastrpc_domain_has_reserved_heap() / fastrpc_domain_uses_static_heap()
>>>    helpers to replace the open-coded ADSP/SDSP domain checks.
>>>
>>> Changes in v11:
>>>  - Replace the remote_heap fastrpc_buf pointer with dedicated
>>>    remote_heap_addr and remote_heap_size fields in
>>>    fastrpc_channel_ctx to avoid leaving a partially
>>>    initialized fastrpc_buf.
>>>
>>>  - Drop ADSP_MMAP_REMOTE_HEAP_ADDR support from
>>>    fastrpc_req_mmap() since the user process should no longer
>>>    grow or shrink the Audio PD remote heap.
>>>
>>> Changes in v10:
>>>  - Move Audio PD remote heap validation into
>>>    fastrpc_rpmsg_probe().
>>>
>>>  - Treat Audio PD remote heap as a mandatory
>>>    resource and fail probe if the reserved
>>>    memory region is missing.
>>>
>>> Changes in v9:
>>>  - Make sure fastrpc_init_create_static_process()
>>>    only sets audio_init_mem to false when the sent
>>>    address is actually invalid.
>>> ---
>>>  drivers/misc/fastrpc.c | 150 +++++++++++++++++++++--------------------
>>>  1 file changed, 76 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 90fd669636ec..3f14a4673698 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -70,8 +70,6 @@
>>>  #define ADSP_MMAP_HEAP_ADDR  4
>>>  /* MAP static DMA buffer on DSP User PD */
>>>  #define ADSP_MMAP_DMA_BUFFER  6
>>> -/* Add memory to static PD pool protection thru hypervisor */
>>> -#define ADSP_MMAP_REMOTE_HEAP_ADDR  8
>>>  /* Add memory to userPD pool, for user heap */
>>>  #define ADSP_MMAP_ADD_PAGES 0x1000
>>>  /* Add memory to userPD pool, for LLC heap */
>>> @@ -314,10 +312,14 @@ 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;
>>> +	/* Audio PD reserved remote heap region */
>>> +	phys_addr_t remote_heap_addr;
>>> +	u64 remote_heap_size;
>>>  	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>>>  	struct fastrpc_device *secure_fdevice;
>>>  	struct fastrpc_device *fdevice;
>>> -	struct fastrpc_buf *remote_heap;
>>>  	struct list_head invoke_interrupted_mmaps;
>>>  	bool secure;
>>>  	bool unsigned_support;
>>> @@ -1454,15 +1456,24 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>  	struct fastrpc_init_create_static init;
>>>  	struct fastrpc_invoke_args *args;
>>>  	struct fastrpc_phy_page pages[1];
>>> +	struct fastrpc_channel_ctx *cctx = fl->cctx;
>>>  	char *name;
>>>  	int err;
>>> -	bool scm_done = false;
>>>  	struct {
>>>  		int client_id;
>>>  		u32 namelen;
>>>  		u32 pageslen;
>>>  	} inbuf;
>>>  	u32 sc;
>>> +	unsigned long flags;
>>> +	bool sent_heap = false;
>>> +
>>> +	if (!cctx->remote_heap_addr || !cctx->remote_heap_size) {
>>> +		err = -ENOMEM;
>>> +		dev_err(fl->sctx->dev,
>>> +			"remote heap memory region is not added\n");
>>> +		return err;
>>> +	}
>>>  
>>>  	args = kzalloc_objs(*args, FASTRPC_CREATE_STATIC_PROCESS_NARGS);
>>>  	if (!args)
>>> @@ -1486,31 +1497,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>  	inbuf.client_id = fl->client_id;
>>>  	inbuf.namelen = init.namelen;
>>>  	inbuf.pageslen = 0;
>>> -	if (!fl->cctx->remote_heap) {
>>> -		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>>> -						&fl->cctx->remote_heap);
>>> -		if (err)
>>> -			goto err_name;
>>> -
>>> -		/* 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->dma_addr,
>>> -							(u64)fl->cctx->remote_heap->size,
>>> -							&src_perms,
>>> -							fl->cctx->vmperms, fl->cctx->vmcount);
>>> -			if (err) {
>>> -				dev_err(fl->sctx->dev,
>>> -					"Failed to assign memory with dma_addr %pad size 0x%llx err %d\n",
>>> -					&fl->cctx->remote_heap->dma_addr,
>>> -					fl->cctx->remote_heap->size, err);
>>> -				goto err_map;
>>> -			}
>>> -			scm_done = true;
>>> -			inbuf.pageslen = 1;
>>> -		}
>>> -	}
>>>  
>>>  	fl->pd = USER_PD;
>>>  
>>> @@ -1522,8 +1508,25 @@ 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->dma_addr;
>>> -	pages[0].size = fl->cctx->remote_heap->size;
>>> +	/*
>>> +	 * Audio PD is a static PD and retains the remote heap
>>> +	 * information across daemon restarts. Therefore only
>>> +	 * the first attach should provide heap information to
>>> +	 * DSP. Subsequent attaches reuse the previously
>>> +	 * initialized memory pool.
>>> +	 */
>>> +	spin_lock_irqsave(&cctx->lock, flags);
>>> +	if (!cctx->audio_init_mem) {
>>> +		pages[0].addr = cctx->remote_heap_addr;
>>> +		pages[0].size = cctx->remote_heap_size;
>>> +		cctx->audio_init_mem = true;
>>> +		inbuf.pageslen = 1;
>>> +		sent_heap = true;
>>> +	} else {
>>> +		pages[0].addr = 0;
>>> +		pages[0].size = 0;
>>> +	}
>>> +	spin_unlock_irqrestore(&cctx->lock, flags);
>>>  
>>>  	args[2].ptr = (u64)(uintptr_t) pages;
>>>  	args[2].length = sizeof(*pages);
>>> @@ -1541,27 +1544,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>  
>>>  	return 0;
>>>  err_invoke:
>>> -	if (fl->cctx->vmcount && scm_done) {
>>> -		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(fl->cctx->remote_heap->dma_addr,
>>> -						(u64)fl->cctx->remote_heap->size,
>>> -						&src_perms, &dst_perms, 1);
>>> -		if (err)
>>> -			dev_err(fl->sctx->dev, "Failed to assign memory dma_addr %pad size 0x%llx err %d\n",
>>> -				&fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
>>> +	if (sent_heap) {
>>> +		spin_lock_irqsave(&cctx->lock, flags);
>>> +		cctx->audio_init_mem = false;
>>> +		spin_unlock_irqrestore(&cctx->lock, flags);
>>>  	}
>>> -err_map:
>>> -	fastrpc_buf_free(fl->cctx->remote_heap);
>>> -	fl->cctx->remote_heap = NULL;
>>> -err_name:
>>>  	kfree(name);
>>>  err:
>>>  	kfree(args);
>>> @@ -2090,7 +2077,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>>  	if (copy_from_user(&req, argp, sizeof(req)))
>>>  		return -EFAULT;
>>>  
>>> -	if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) {
>>> +	if (req.flags != ADSP_MMAP_ADD_PAGES) {
>>>  		dev_err(dev, "flag not supported 0x%x\n", req.flags); return -EINVAL; @@ -2101,10 +2088,7 @@ static int
>>> fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) return -
>>> EINVAL; } - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) - err =
>>> fastrpc_remote_heap_alloc(fl, dev, req.size, &buf); - else - err =
>>> fastrpc_buf_alloc(fl, dev, req.size, &buf); + err =
>>> fastrpc_buf_alloc(fl, dev, req.size, &buf); if (err) { dev_err(dev, "failed to allocate buffer\n");
>>> @@ -2143,20 +2127,6 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>>  	/* let the client know the address to use */
>>>  	req.vaddrout = rsp_msg.vaddr;
>>>  
>>> -	/* Add memory to static PD pool, protection thru hypervisor */
>>> -	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>>> -		u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>> -
>>> -		err = qcom_scm_assign_mem(buf->dma_addr, (u64)buf->size,
>>> -			&src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>>> -		if (err) {
>>> -			dev_err(fl->sctx->dev,
>>> -				"Failed to assign memory dma_addr %pad size 0x%llx err %d",
>>> -				&buf->dma_addr, buf->size, err);
>>> -			goto err_assign;
>>> -		}
>>> -	}
>>> -
>>>  	spin_lock(&fl->lock);
>>>  	list_add_tail(&buf->node, &fl->mmaps);
>>>  	spin_unlock(&fl->lock);
>>> @@ -2537,6 +2507,16 @@ static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unuse
>>>  	{},
>>>  };
>>>  
>>> +static bool fastrpc_domain_has_reserved_heap(u32 domain_id)
>>> +{
>>> +	return domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID;
>>> +}
>>> +
>>> +static bool fastrpc_domain_uses_static_heap(u32 domain_id)
>>> +{
>>> +	return domain_id == ADSP_DOMAIN_ID;
>>> +}
>> any reason to have functions for one time used checks?> +
> 
> Just to give some context: I added these helpers based on the v11
> review, which requested replacing the open-coded ADSP/SDSP checks
> with functions describing the per-domain capabilities.
> 
> In this patch, they currently have a single caller each.
> Therefore, if you feel it's unnecessary, I can revert
> to the version without helpers.
move the reserved memory initialization block to a new helper function,
something like fastrpc_init_reserved_mem()>
>>>  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>  {
>>>  	struct device *rdev = &rpdev->dev;
>>> @@ -2584,20 +2564,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>  		}
>>>  	}
>>>  
>>> -	if (domain_id == SDSP_DOMAIN_ID) {
>>> +	if (fastrpc_domain_has_reserved_heap(domain_id)) {
>>>  		struct resource res;
>>>  		u64 src_perms;
>>>  
>>>  		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
>>>  		if (!err) {
>>> +			if (fastrpc_domain_uses_static_heap(domain_id)) {
>>> +				data->remote_heap_addr = res.start;
>>> +				data->remote_heap_size = resource_size(&res);
>>> +			}
>>>  			src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>>  
>>>  			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
>>>  				    data->vmperms, data->vmcount);
>> better to check vmcount before calling this>  			if (err)
> 
> Agreed. I'll wrap the qcom_scm_assign_mem() call in if (data->vmcount) in v13.
> 
>>>  				goto err_free_data;
>>> +		} else {
>>> +			err = 0;
>>>  		}
>>> -
>>>  	}
>>>  
>>>  	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>>> @@ -2681,6 +2666,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>>  	struct fastrpc_buf *buf, *b;
>>>  	struct fastrpc_user *user;
>>>  	unsigned long flags;
>>> +	int err, i;
>>>  
>>>  	/* No invocations past this point */
>>>  	spin_lock_irqsave(&cctx->lock, flags);
>>> @@ -2698,8 +2684,24 @@ 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);
>>> +	if (cctx->remote_heap_size && cctx->vmcount) {
>>> +		u64 src_perms = 0;
>>> +		struct qcom_scm_vmperm dst_perms;
>>> +
>>> +		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(cctx->remote_heap_addr,
>>> +					  cctx->remote_heap_size, &src_perms,
>>> +					  &dst_perms, 1);
>>> +		if (err)
>>> +			dev_err(&rpdev->dev,
>>> +				"Failed to assign memory back to HLOS: addr %pa size %#llx err %d\n",
>>> +				&cctx->remote_heap_addr, cctx->remote_heap_size, err);
>>> +	}
>>>  
>>>  	of_platform_depopulate(&rpdev->dev);
>>>