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

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

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