Message-ID: <cq3qfx32dallivdcwjwqgq7kggiwucpcyhwqqlbrf6n4efkmuc@htjwnigojag2>
Date: Thu, 12 Jun 2025 13:24:15 +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 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.

> 
> [1] https://github.com/quic/fastrpc/blob/development/src/apps_mem_imp.c#L231
> >

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists