On 7/2/2026 4:37 AM, Srinivas Kandagatla wrote:
On 6/9/26 3:59 AM, 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. Add explicit validation for remote_heap presence and size before sending the memory to DSP, and fail early if the reserved-memory region is missing or incomplete. Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd") Cc: [email protected] Signed-off-by: Jianping Li <[email protected]> --- drivers/misc/fastrpc.c | 112 ++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 53 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 517884000331..1942e74535e5 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -276,6 +276,8 @@ 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; @@ -1341,15 +1343,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; + + if (!cctx->remote_heap || !cctx->remote_heap->dma_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) @@ -1373,31 +1384,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; @@ -1409,8 +1395,17 @@ 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; + spin_lock_irqsave(&cctx->lock, flags); + if (!cctx->audio_init_mem) { + pages[0].addr = cctx->remote_heap->dma_addr; + pages[0].size = cctx->remote_heap->size; + cctx->audio_init_mem = true; + inbuf.pageslen = 1; + } else { + pages[0].addr = 0; + pages[0].size = 0;What is the expected behavoiur in this case?
You're right that this behaviour is subtle and deserves an in-code
comment. I'll add one in v9, right above the spin_lock, along the
lines of:
/*
* 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_unlock_irqrestore(&cctx->lock, flags);args[2].ptr = (u64)(uintptr_t) pages;args[2].length = sizeof(*pages); @@ -1428,27 +1423,7 @@ 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); - } -err_map: - fastrpc_buf_free(fl->cctx->remote_heap); - fl->cctx->remote_heap = NULL; -err_name: + cctx->audio_init_mem = false;this can race.
Thanks for the review. I'd like to clarify the intent here — please
let me know if I'm missing something.
The lock is not really there to protect 'audio_init_mem' itself,
but to make sure the following is only sent to the DSP once:
pages[0].addr = fl->cctx->remote_heap->phys;
pages[0].size = fl->cctx->remote_heap->size;
The flow I had in mind is:
- Thread A enters the critical section, sees audio_init_mem == false,
sets it to true, and proceeds to send the pages to the DSP.
- If Thread A's invoke fails, we reset audio_init_mem back to false
in err_invoke so that a later attempt can retry sending the region.
- Once any thread successfully sends the region, audio_init_mem stays
true forever and the false branch is never taken again.
So the reset on failure is only meant to allow a retry, not to be
racy with a concurrent successful path (there can't be one, since
only one thread is inside the critical section at a time). Do you
see a specific interleaving I've overlooked? If so, I'm happy to
switch to a different scheme.
kfree(name); err: kfree(args); @@ -2415,12 +2390,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) } }- if (domain_id == SDSP_DOMAIN_ID) {+ if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) { struct resource res; u64 src_perms;err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);if (!err) { + if (domain_id == ADSP_DOMAIN_ID) { + data->remote_heap = + kzalloc_obj(*data->remote_heap, GFP_KERNEL); + if (!data->remote_heap) { + err = -ENOMEM; + goto err_free_data; + } + + data->remote_heap->dma_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,@@ -2428,7 +2414,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) if (err) goto err_free_data; } - }secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));@@ -2487,6 +2472,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) misc_deregister(&data->secure_fdevice->miscdev);err_free_data:+ kfree(data->remote_heap); kfree(data); return err; } @@ -2509,6 +2495,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) struct fastrpc_buf *buf, *b; struct fastrpc_user *user; unsigned long flags; + int err;/* No invocations past this point */spin_lock_irqsave(&cctx->lock, flags); @@ -2526,8 +2513,27 @@ 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 && cctx->vmcount) { + u64 src_perms = 0; + struct qcom_scm_vmperm dst_perms; + + for (u32 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->dma_addr, + cctx->remote_heap->size, &src_perms, + &dst_perms, 1); + if (err) + dev_err(&rpdev->dev, + "Failed to assign memory back to HLOS: dma_addr %pad size %#llx err %d\n", + &cctx->remote_heap->dma_addr, cctx->remote_heap->size, err); + } + + kfree(cctx->remote_heap); + cctx->remote_heap = NULL;of_platform_depopulate(&rpdev->dev);
