Message-ID: <lmytte3p2eq7fsjsbogzrqjyimlw42v2x2zystx32nuvnm62yb@wzqrmhqcrzl3>
Date: Sat, 19 Jul 2025 12:44:55 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Ekansh Gupta <ekansh.gupta@....qualcomm.com>
Cc: srinivas.kandagatla@....qualcomm.com, linux-arm-msm@...r.kernel.org,
        gregkh@...uxfoundation.org, quic_bkumar@...cinc.com,
        linux-kernel@...r.kernel.org, quic_chennak@...cinc.com,
        dri-devel@...ts.freedesktop.org, arnd@...db.de, stable@...nel.org
Subject: Re: [PATCH v1 5/5] misc: fastrpc: Add missing unmapping
 user-requested remote heap

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@....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

Powered by blists - more mailing lists