Re: [PATCH v7 4/5] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Jianping Li <jianping.li@oss.qualcomm.com>]

On 6/2/2026 5:41 PM, Jie Gan wrote:
>
>
> On 6/2/2026 3:17 PM, 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.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
>> ---
>>   drivers/misc/fastrpc.c | 96 +++++++++++++++++++-----------------------
>>   1 file changed, 43 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index f46a8f53970d..33be8bed6a0b 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;
>> @@ -1344,15 +1346,16 @@ 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 (!fl->cctx->remote_heap ||
>>           !fl->cctx->remote_heap->dma_addr ||
>> @@ -1383,31 +1386,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;
>>   @@ -1419,8 +1397,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 (!fl->cctx->audio_init_mem) {
>> +        pages[0].addr = fl->cctx->remote_heap->dma_addr;
>> +        pages[0].size = fl->cctx->remote_heap->size;
>> +        fl->cctx->audio_init_mem = true;
>> +        inbuf.pageslen = 1;
>> +    } 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);
>> @@ -1438,27 +1425,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:
>> +    fl->cctx->audio_init_mem = false;
>>       kfree(name);
>>   err:
>>       kfree(args);
>> @@ -2425,12 +2392,21 @@ 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)
>> +                    return -ENOMEM;
>
> allocated data never free with directly return.
>
> goto err_free_data;
>
> Beside, we also need free data->remote_heap in err_free_data path as 
> you added new memory allocation.

I will adjust goto err_free_data, and add free(data->remote_heap) in 
err_free_data.

err_free_data:
     kfree(data->remote_heap);
     kfree(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,
>> @@ -2438,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"));
>> @@ -2519,6 +2494,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);
>> @@ -2536,8 +2512,22 @@ 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);
>
> after removed the code, the cctx->remote_heap is not freed:
> 1. cctx->vmcount == 0
> 2. if (!err) is false
>
> we should free the cctx->remote_heap unconditionally if it exists.
>
> Thanks,
> Jie

You are right, after changing this to kfree here, the lifecycle should 
not be tied to whether the assignment is successful

so here we should kfree remote_heap every time.

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;

>
>> +    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)
>> +            kfree(cctx->remote_heap);
>> +    }
>>         of_platform_depopulate(&rpdev->dev);
>