[PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Ameya Palande]

Background: bridge_close() has the responsibility to cleanup all the
resources allocated by user space process. One of those resources is
DMMRes which is used for tracking DMM resource allocation done in
PROC_Map() and PROC_UnMap().

DRV_ProcFreeDMMRes() function was used for cleaning up DMMRes has following
problems which are fixed by this patch:

1. It access and modifies pDMMRes pointer when it is already freed by
PROC_UnMap()
2. Instead of passing ulDSPAddr it passes ulDSPResAddr to PROC_UnMap() which
will not retrive correct DMMRes element.

CC: Omar Ramirez Luna <omar.ramirez@ti.com>
CC: Nishanth Menon <nm@ti.com>
CC: Deepak Chitriki Rudramuni <deepak.chitriki@ti.com>
CC: Felipe Contreras <felipe.contreras@nokia.com>
CC: Phil Carmody <ext-phil.2.carmody@nokia.com>

Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 arch/arm/plat-omap/include/dspbridge/drv.h         |   14 ----
 .../plat-omap/include/dspbridge/resourcecleanup.h  |    4 +-
 drivers/dsp/bridge/rmgr/drv.c                      |   63 ++++++++------------
 drivers/dsp/bridge/rmgr/drv_interface.c            |    7 +-
 4 files changed, 30 insertions(+), 58 deletions(-)

diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat-omap/include/dspbridge/drv.h
index b6a5fd2..ba11359 100644
--- a/arch/arm/plat-omap/include/dspbridge/drv.h
+++ b/arch/arm/plat-omap/include/dspbridge/drv.h
@@ -384,18 +384,4 @@ struct PROCESS_CONTEXT{
  */
 	extern DSP_STATUS DRV_ReleaseResources(IN u32 dwContext,
 					       struct DRV_OBJECT *hDrvObject);
-
-/*
- *  ======== DRV_ProcFreeDMMRes ========
- *  Purpose:
- *       Actual DMM De-Allocation.
- *  Parameters:
- *      hPCtxt:      Path to the driver Registry Key.
- *  Returns:
- *      DSP_SOK if success;
- */
-
-
-	extern DSP_STATUS  DRV_ProcFreeDMMRes(HANDLE hPCtxt);
-
 #endif				/* DRV_ */
diff --git a/arch/arm/plat-omap/include/dspbridge/resourcecleanup.h b/arch/arm/plat-omap/include/dspbridge/resourcecleanup.h
index ffbcf5e..d399a2e 100644
--- a/arch/arm/plat-omap/include/dspbridge/resourcecleanup.h
+++ b/arch/arm/plat-omap/include/dspbridge/resourcecleanup.h
@@ -25,13 +25,13 @@ extern DSP_STATUS DRV_GetProcCtxtList(struct PROCESS_CONTEXT **pPctxt,
 extern DSP_STATUS DRV_InsertProcContext(struct DRV_OBJECT *hDrVObject,
 					HANDLE hPCtxt);
 
-extern DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE pCtxt);
+extern DSP_STATUS DRV_Remove_All_DMM_Res_Elements(HANDLE hPCtxt);
 
 extern DSP_STATUS DRV_RemoveAllNodeResElements(HANDLE pCtxt);
 
 extern DSP_STATUS DRV_ProcSetPID(HANDLE pCtxt, s32 hProcess);
 
-extern DSP_STATUS DRV_RemoveAllResources(HANDLE pPctxt);
+extern void DRV_RemoveAllResources(HANDLE pPctxt);
 
 extern DSP_STATUS DRV_RemoveProcContext(struct DRV_OBJECT *hDRVObject,
 				     HANDLE pr_ctxt);
diff --git a/drivers/dsp/bridge/rmgr/drv.c b/drivers/dsp/bridge/rmgr/drv.c
index eca46ca..570f8a4 100644
--- a/drivers/dsp/bridge/rmgr/drv.c
+++ b/drivers/dsp/bridge/rmgr/drv.c
@@ -259,52 +259,39 @@ DSP_STATUS DRV_UpdateDMMResElement(HANDLE hDMMRes, u32 pMpuAddr, u32 ulSize,
 	return status;
 }
 
-/* Actual DMM De-Allocation */
-DSP_STATUS  DRV_ProcFreeDMMRes(HANDLE hPCtxt)
+/* Release all DMM resources and its context */
+DSP_STATUS DRV_Remove_All_DMM_Res_Elements(HANDLE hPCtxt)
 {
-	struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt;
+	struct PROCESS_CONTEXT *p_ctxt = (struct PROCESS_CONTEXT *)hPCtxt;
 	DSP_STATUS status = DSP_SOK;
-	struct DMM_RES_OBJECT *pDMMList = pCtxt->pDMMList;
-	struct DMM_RES_OBJECT *pDMMRes = NULL;
-
-	GT_0trace(curTrace, GT_ENTER, "\nDRV_ProcFreeDMMRes: 1\n");
-	while (pDMMList != NULL) {
-		pDMMRes = pDMMList;
-		pDMMList = pDMMList->next;
-		if (pDMMRes->dmmAllocated) {
-			status = PROC_UnMap(pDMMRes->hProcessor,
-				 (void *)pDMMRes->ulDSPResAddr, pCtxt);
+	struct DMM_RES_OBJECT *p_dmm_list = p_ctxt->pDMMList;
+	struct DMM_RES_OBJECT *p_cur_res;
+
+	while (p_dmm_list) {
+		p_cur_res = p_dmm_list;
+		p_dmm_list = p_dmm_list->next;
+		if (p_cur_res->dmmAllocated) {
+			/*
+			 * PROC_UnMap will free memory allocated to p_cur_res
+			 * pointer. So we need to save following data for the
+			 * execution of PROC_UnReserveMemory()
+			 */
+			void *h_processor = p_cur_res->hProcessor;
+			u32 dsp_res_addr = p_cur_res->ulDSPResAddr;
+
+			status = PROC_UnMap(p_cur_res->hProcessor,
+				 (void *)p_cur_res->ulDSPAddr, p_ctxt);
 			if (DSP_FAILED(status))
 				pr_debug("%s: PROC_UnMap failed! status ="
-						" 0x%xn", __func__, status);
-			status = PROC_UnReserveMemory(pDMMRes->hProcessor,
-				 (void *)pDMMRes->ulDSPResAddr);
+						" 0x%x\n", __func__, status);
+			status = PROC_UnReserveMemory(h_processor,
+				 (void *)dsp_res_addr);
 			if (DSP_FAILED(status))
 				pr_debug("%s: PROC_UnReserveMemory failed!"
-					" status = 0x%xn", __func__, status);
-			pDMMRes->dmmAllocated = 0;
+					" status = 0x%x\n", __func__, status);
 		}
 	}
-	return status;
-}
-
-/* Release all DMM resources and its context
-* This is called from .bridge_release. */
-DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt)
-{
-	struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt;
-	DSP_STATUS status = DSP_SOK;
-	struct DMM_RES_OBJECT *pTempDMMRes2 = NULL;
-	struct DMM_RES_OBJECT *pTempDMMRes = NULL;
-
-	DRV_ProcFreeDMMRes(pCtxt);
-	pTempDMMRes = pCtxt->pDMMList;
-	while (pTempDMMRes != NULL) {
-		pTempDMMRes2 = pTempDMMRes;
-		pTempDMMRes = pTempDMMRes->next;
-		MEM_Free(pTempDMMRes2);
-	}
-	pCtxt->pDMMList = NULL;
+	p_ctxt->pDMMList = NULL;
 	return status;
 }
 
diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index 625f88e..2e56704 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -604,15 +604,14 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
 
 /* To remove all process resources before removing the process from the
  * process context list*/
-DSP_STATUS DRV_RemoveAllResources(HANDLE hPCtxt)
+void DRV_RemoveAllResources(HANDLE hPCtxt)
 {
-	DSP_STATUS status = DSP_SOK;
 	struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt;
 	DRV_RemoveAllSTRMResElements(pCtxt);
 	DRV_RemoveAllNodeResElements(pCtxt);
-	DRV_RemoveAllDMMResElements(pCtxt);
+	DRV_Remove_All_DMM_Res_Elements(pCtxt);
 	pCtxt->resState = PROC_RES_FREED;
-	return status;
+	return;
 }
 
 /* Bridge driver initialization and de-initialization functions */
-- 
1.6.3.3

Re: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Felipe Contreras]

On Mon, Feb 8, 2010 at 10:25 PM, Ameya Palande <ameya.palande@nokia.com> wrote:
> Background: bridge_close() has the responsibility to cleanup all the
> resources allocated by user space process. One of those resources is
> DMMRes which is used for tracking DMM resource allocation done in
> PROC_Map() and PROC_UnMap().
>
> DRV_ProcFreeDMMRes() function was used for cleaning up DMMRes has following
> problems which are fixed by this patch:
>
> 1. It access and modifies pDMMRes pointer when it is already freed by
> PROC_UnMap()
> 2. Instead of passing ulDSPAddr it passes ulDSPResAddr to PROC_UnMap() which
> will not retrive correct DMMRes element.

This patch is doing the right thing to me, however, it's a bit
difficult to review because it's doing many changes at once.

Personally I would
 1) Fix the wrong dereferences in DRV_ProcFreeDMMRes
[at this point the bug should be fixed]
[the leaks would be freed by the outer loop]

 2) Fix the wrong PROC_UnMap address passed in DRV_ProcFreeDMMRes
 3) Remove the unnecessary outer loop in DRV_RemoveAllDMMResElements
[at this point the code would actually make sense]

 4) Merge DRV_ProcFreeDMMRes into DRV_RemoveAllDMMResElements
 5) Improve coding style

The end result would be the same, but easier to see what's going on :)

Cheers.

-- 
Felipe Contreras

RE: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Guzman Lugo, Fernando]

Hi,

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner@vger.kernel.org] On Behalf Of Felipe Contreras
>Sent: Tuesday, February 09, 2010 6:30 AM
>To: Ameya Palande
>Cc: linux-omap@vger.kernel.org; Ramirez Luna, Omar; Menon, Nishanth;
>Chitriki Rudramuni, Deepak; felipe.contreras@nokia.com; ext-
>phil.2.carmody@nokia.com
>Subject: Re: [PATCH] DSPBRIDGE: Prevent memory corruption in
>DRV_ProcFreeDMMRes
>
>On Mon, Feb 8, 2010 at 10:25 PM, Ameya Palande <ameya.palande@nokia.com>
>wrote:
>> Background: bridge_close() has the responsibility to cleanup all the
>> resources allocated by user space process. One of those resources is
>> DMMRes which is used for tracking DMM resource allocation done in
>> PROC_Map() and PROC_UnMap().
>>
>> DRV_ProcFreeDMMRes() function was used for cleaning up DMMRes has
>following
>> problems which are fixed by this patch:
>>
>> 1. It access and modifies pDMMRes pointer when it is already freed by
>> PROC_UnMap()
>> 2. Instead of passing ulDSPAddr it passes ulDSPResAddr to PROC_UnMap()
>which
>> will not retrive correct DMMRes element.
>
>This patch is doing the right thing to me, however, it's a bit
>difficult to review because it's doing many changes at once.
>
>Personally I would
> 1) Fix the wrong dereferences in DRV_ProcFreeDMMRes
>[at this point the bug should be fixed]
>[the leaks would be freed by the outer loop]
>
> 2) Fix the wrong PROC_UnMap address passed in DRV_ProcFreeDMMRes
> 3) Remove the unnecessary outer loop in DRV_RemoveAllDMMResElements
>[at this point the code would actually make sense]
>
> 4) Merge DRV_ProcFreeDMMRes into DRV_RemoveAllDMMResElements
> 5) Improve coding style
>
>The end result would be the same, but easier to see what's going on :)
>
>Cheers.
>
>--
>Felipe Contreras

The root problem here is that remove the DMM element should be in PROC_UnMap but in PROC_UnReserveMemory, because apart of the problem your are seeing about memory corruption if the application does a PROC_ReserveMemory and then it finished unexpectedly the resource cleanup wont be able to unreserved the memory because there is no a DMMres element in process context struct, the same applies in the case of application calls  PROC_ReserveMemory, PROC_Map, PROC_UnMap but fails to do the PROC_UnReserveMemory.


So the right solution should be:

1 .- Remove DRV_InsertDMMResElement function from PROC_Map and put it on PROC_ReserveMemory.

2.- Keep DRV_UpdateDMMResElement function in PROC_Map.

3.- Remove DRV_RemoveDMMResElement function from PROC_UnMap and put it on ROC_UnReserveMemory function.

4.- Clear dmmAllocated flag in PROC_UnMap:

pDMMRes->dmmAllocated = 0;

5 .- in the function DRV_ProcFreeDMMRes, call PROC_UnMap "if (pDMMRes->dmmAllocated)" and always call PROC_UnReserveMemory not in this point is not needed the save hProcessor nor ulDSPResAddr addresses.

6 .- cleanup DRV_RemoveAllDMMResElements function (while loop is not needed).

Regards,
Fernando.


>--
>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Felipe Contreras]

On Tue, Feb 9, 2010 at 10:22 PM, Guzman Lugo, Fernando <x0095840@ti.com> wrote:
> The root problem here is that remove the DMM element should be in PROC_UnMap but in PROC_UnReserveMemory, because apart of the problem your are seeing about memory corruption if the application does a PROC_ReserveMemory and then it finished unexpectedly the resource cleanup wont be able to unreserved the memory because there is no a DMMres element in process context struct, the same applies in the case of application calls  PROC_ReserveMemory, PROC_Map, PROC_UnMap but fails to do the PROC_UnReserveMemory.

No, that's a completely different issue, but also valid.

> So the right solution should be:
>
> 1 .- Remove DRV_InsertDMMResElement function from PROC_Map and put it on PROC_ReserveMemory.
>
> 2.- Keep DRV_UpdateDMMResElement function in PROC_Map.
>
> 3.- Remove DRV_RemoveDMMResElement function from PROC_UnMap and put it on ROC_UnReserveMemory function.
>
> 4.- Clear dmmAllocated flag in PROC_UnMap:
>
> pDMMRes->dmmAllocated = 0;

At this point there's an obvious question; what's the point of
reserving a memory region and not mapping it?

I remember the answer from Hari was: some clients prefer to reserve a
big region once, and map parts of it continuously. I have my doubts
that the use-case even works with the current code-base. But assuming
it does work, your proposed changes would break it.

It would be much easier to merge the two functions into one.

Anyway, please re-read Ameya's description carefully.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Guzman Lugo, Fernando]

Hi,

>-----Original Message-----
>From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
>Sent: Tuesday, February 09, 2010 5:48 PM
>To: Guzman Lugo, Fernando
>Cc: Ameya Palande; linux-omap@vger.kernel.org; Ramirez Luna, Omar; Menon,
>Nishanth; Chitriki Rudramuni, Deepak; felipe.contreras@nokia.com; ext-
>phil.2.carmody@nokia.com
>Subject: Re: [PATCH] DSPBRIDGE: Prevent memory corruption in
>DRV_ProcFreeDMMRes
>
>On Tue, Feb 9, 2010 at 10:22 PM, Guzman Lugo, Fernando <x0095840@ti.com>
>wrote:
>> The root problem here is that remove the DMM element should be in
>PROC_UnMap but in PROC_UnReserveMemory, because apart of the problem your
>are seeing about memory corruption if the application does a
>PROC_ReserveMemory and then it finished unexpectedly the resource cleanup
>wont be able to unreserved the memory because there is no a DMMres element
>in process context struct, the same applies in the case of application
>calls  PROC_ReserveMemory, PROC_Map, PROC_UnMap but fails to do the
>PROC_UnReserveMemory.
>
>No, that's a completely different issue, but also valid.
>
>> So the right solution should be:
>>
>> 1 .- Remove DRV_InsertDMMResElement function from PROC_Map and put it on
>PROC_ReserveMemory.
>>
>> 2.- Keep DRV_UpdateDMMResElement function in PROC_Map.
>>
>> 3.- Remove DRV_RemoveDMMResElement function from PROC_UnMap and put it on
>ROC_UnReserveMemory function.
>>
>> 4.- Clear dmmAllocated flag in PROC_UnMap:
>>
>> pDMMRes->dmmAllocated = 0;
>
>At this point there's an obvious question; what's the point of
>reserving a memory region and not mapping it?
>
>I remember the answer from Hari was: some clients prefer to reserve a
>big region once, and map parts of it continuously. I have my doubts
>that the use-case even works with the current code-base. But assuming
>it does work, your proposed changes would break it.

Resource cleanup does not support that even without my proposed changes. I just proposed a solution which fixes two issues in one patch. Moreover if this change is merged when the second issue be fixed this patch will not needed anymore, so why don't merge the patch which fixes both errors at this moment?

Anyway, if you want to include this patch, the only comments we have is:

>+			 * PROC_UnMap will free memory allocated to p_cur_res
>+			 * pointer. So we need to save following data for the
>+			 * execution of PROC_UnReserveMemory()
>+			 */
>+			void *h_processor = p_cur_res->hProcessor;

You could use hProcessor store in p_ctxt instead of p_cur_res, so that you don't need to save hProcessor.

>+			u32 dsp_res_addr = p_cur_res->ulDSPResAddr;
>+
>+			status = PROC_UnMap(p_cur_res->hProcessor,
>+				 (void *)p_cur_res->ulDSPAddr, p_ctxt);
>
>It would be much easier to merge the two functions into one.

Yes, I am agreed.
>
>Anyway, please re-read Ameya's description carefully.
>
>--
>Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Felipe Contreras]

On Wed, Feb 10, 2010 at 05:06:26AM +0100, ext Guzman Lugo, Fernando wrote:
> >At this point there's an obvious question; what's the point of
> >reserving a memory region and not mapping it?
> >
> >I remember the answer from Hari was: some clients prefer to reserve a
> >big region once, and map parts of it continuously. I have my doubts
> >that the use-case even works with the current code-base. But assuming
> >it does work, your proposed changes would break it.
> 
> Resource cleanup does not support that even without my proposed changes.

Aha! I suspected it :P

> I just proposed a solution which fixes two issues in one patch.
> Moreover if this change is merged when the second issue be fixed this
> patch will not needed anymore, so why don't merge the patch which
> fixes both errors at this moment?

simple patches > complicated patches

Personally I think your patches should be a continuation to the patches
I just proposed.

If nobody wants to split these patches, I'll gladly do so.

> >+			u32 dsp_res_addr = p_cur_res->ulDSPResAddr;
> >+
> >+			status = PROC_UnMap(p_cur_res->hProcessor,
> >+				 (void *)p_cur_res->ulDSPAddr, p_ctxt);
> >
> >It would be much easier to merge the two functions into one.
> 
> Yes, I am agreed.

Good. Perhaps we can start moving reserve/unreserve functionality to
map/unmap, and eventually depreate the former.

Cheers.

-- 
Felipe Contreras

RE: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Guzman Lugo, Fernando]

>-----Original Message-----
>From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
>Sent: Wednesday, February 10, 2010 7:08 AM
>To: Guzman Lugo, Fernando
>Cc: Ameya Palande; linux-omap; Ramirez Luna, Omar; Menon, Nishanth;
>Chitriki Rudramuni, Deepak; Phil Carmody
>Subject: Re: [PATCH] DSPBRIDGE: Prevent memory corruption in
>DRV_ProcFreeDMMRes
>
>On Wed, Feb 10, 2010 at 05:06:26AM +0100, ext Guzman Lugo, Fernando wrote:
>> >At this point there's an obvious question; what's the point of
>> >reserving a memory region and not mapping it?
>> >
>> >I remember the answer from Hari was: some clients prefer to reserve a
>> >big region once, and map parts of it continuously. I have my doubts
>> >that the use-case even works with the current code-base. But assuming
>> >it does work, your proposed changes would break it.
>>
>> Resource cleanup does not support that even without my proposed changes.
>
>Aha! I suspected it :P
>
Resource cleanup does not support that because it tries to unreserved every chunk mapped. However it does not means the dspbridge does not support reserving a big chunk of memory and mapping small parts of that memory, but I have not tested it to confirm if it works.

>> I just proposed a solution which fixes two issues in one patch.
>> Moreover if this change is merged when the second issue be fixed this
>> patch will not needed anymore, so why don't merge the patch which
>> fixes both errors at this moment?
>
>simple patches > complicated patches
The patch is more simple than it looks.

>
>Personally I think your patches should be a continuation to the patches
>I just proposed.
Yes, I think it would be better.

Regards,
Fernando.

>
>If nobody wants to split these patches, I'll gladly do so.
>
>> >+			u32 dsp_res_addr = p_cur_res->ulDSPResAddr;
>> >+
>> >+			status = PROC_UnMap(p_cur_res->hProcessor,
>> >+				 (void *)p_cur_res->ulDSPAddr, p_ctxt);
>> >
>> >It would be much easier to merge the two functions into one.
>>
>> Yes, I am agreed.
>
>Good. Perhaps we can start moving reserve/unreserve functionality to
>map/unmap, and eventually depreate the former.
>
>Cheers.
>
>--
>Felipe Contreras

Re: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Felipe Contreras]

On Wed, Feb 10, 2010 at 06:31:35PM +0100, ext Guzman Lugo, Fernando wrote:
> >-----Original Message-----
> >From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> >> Resource cleanup does not support that even without my proposed changes.
> >
> >Aha! I suspected it :P
> >
> Resource cleanup does not support that because it tries to unreserved
> every chunk mapped. However it does not means the dspbridge does not
> support reserving a big chunk of memory and mapping small parts of
> that memory, but I have not tested it to confirm if it works.

I think I did once and it didn't work, but I thought the problem was in
my app. Anyway I think we should either forget about this use-case or
fix the resource cleaning.

> >> I just proposed a solution which fixes two issues in one patch.
> >> Moreover if this change is merged when the second issue be fixed this
> >> patch will not needed anymore, so why don't merge the patch which
> >> fixes both errors at this moment?
> >
> >simple patches > complicated patches
> The patch is more simple than it looks.

Only one way to know :)

> >Personally I think your patches should be a continuation to the patches
> >I just proposed.
> Yes, I think it would be better.

Cool.

-- 
Felipe Contreras

Re: [PATCH] DSPBRIDGE: Prevent memory corruption in DRV_ProcFreeDMMRes

[Ameya Palande]

Hi all,

On Thu, 2010-02-11 at 16:53 +0100, Contreras Felipe (Nokia-D/Helsinki)
wrote:
> On Wed, Feb 10, 2010 at 06:31:35PM +0100, ext Guzman Lugo, Fernando wrote:
> > >-----Original Message-----
> > >From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> > >> Resource cleanup does not support that even without my proposed changes.
> > >
> > >Aha! I suspected it :P
> > >
> > Resource cleanup does not support that because it tries to unreserved
> > every chunk mapped. However it does not means the dspbridge does not
> > support reserving a big chunk of memory and mapping small parts of
> > that memory, but I have not tested it to confirm if it works.
> 
> I think I did once and it didn't work, but I thought the problem was in
> my app. Anyway I think we should either forget about this use-case or
> fix the resource cleaning.
> 
> > >> I just proposed a solution which fixes two issues in one patch.
> > >> Moreover if this change is merged when the second issue be fixed this
> > >> patch will not needed anymore, so why don't merge the patch which
> > >> fixes both errors at this moment?
> > >
> > >simple patches > complicated patches
> > The patch is more simple than it looks.
> 
> Only one way to know :)
> 
> > >Personally I think your patches should be a continuation to the patches
> > >I just proposed.
> > Yes, I think it would be better.
> 
> Cool.
> 

Thanks for reviewing the patch!

After going through your comments I feel the current patch is inadequate
to do proper resource cleanup :(

I am working on something which should take care of map and reserve
both. Basically I am working on separating current DMM resource
accounting for map and reserve, which should fix all the problems. Will
post the patchset soon!

Cheers,
Ameya.