[PATCH 0/2] staging: tidspbridge: memory leak fixes

[PATCH 0/2] staging: tidspbridge: memory leak fixes

[Omar Ramirez Luna]

These patches solve two lingering memory leak scenarios introduced
in tidspbridge code.

1. Due to missing balanced calls during bridge_release deallocation, and
   a potential leak in bridge_open because of missing cleanup path.
2. Leaks because of a misuse of an already freed pointer, which triggered
   error paths during module removal leaking some memory.

When accepted they must be propagated to stable kernel releases.

Omar Ramirez Luna (2):
  staging: tidspbridge: fix bridge_open memory leaks
  staging: tidspbridge: fix incorrect free to drv_datap

 drivers/staging/tidspbridge/core/tiomap3430.c    |    2 -
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   58 +++++++++++++---------
 2 files changed, 35 insertions(+), 25 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] staging: tidspbridge: fix bridge_open memory leaks

[Omar Ramirez Luna]

There are two members of pr_ctxt allocated during bridge_open that
are never freed resulting in memory leaks, these are stream_id and
node_id, they are now freed on release of the handle (bridge_release)
right before freeing pr_ctxt.

Error path for bridge_open was also fixed since the same variables
could result in memory leaking due to missing handling of failure
scenarios. While at it, the indentation changes were introduced to
avoid interleaved goto statements inside big if blocks.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   55 +++++++++++++---------
 1 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 76cfc6e..8bac511 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -500,35 +500,42 @@ static int bridge_open(struct inode *ip, struct file *filp)
 	}
 #endif
 	pr_ctxt = kzalloc(sizeof(struct process_context), GFP_KERNEL);
-	if (pr_ctxt) {
-		pr_ctxt->res_state = PROC_RES_ALLOCATED;
-		spin_lock_init(&pr_ctxt->dmm_map_lock);
-		INIT_LIST_HEAD(&pr_ctxt->dmm_map_list);
-		spin_lock_init(&pr_ctxt->dmm_rsv_lock);
-		INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list);
-
-		pr_ctxt->node_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
-		if (pr_ctxt->node_id) {
-			idr_init(pr_ctxt->node_id);
-		} else {
-			status = -ENOMEM;
-			goto err;
-		}
+	if (!pr_ctxt)
+		return -ENOMEM;
+
+	pr_ctxt->res_state = PROC_RES_ALLOCATED;
+	spin_lock_init(&pr_ctxt->dmm_map_lock);
+	INIT_LIST_HEAD(&pr_ctxt->dmm_map_list);
+	spin_lock_init(&pr_ctxt->dmm_rsv_lock);
+	INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list);
 
-		pr_ctxt->stream_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
-		if (pr_ctxt->stream_id)
-			idr_init(pr_ctxt->stream_id);
-		else
-			status = -ENOMEM;
-	} else {
+	pr_ctxt->node_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
+	if (!pr_ctxt->node_id) {
 		status = -ENOMEM;
+		goto err1;
 	}
-err:
+
+	idr_init(pr_ctxt->node_id);
+
+	pr_ctxt->stream_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
+	if (!pr_ctxt->stream_id) {
+		status = -ENOMEM;
+		goto err2;
+	}
+
+	idr_init(pr_ctxt->stream_id);
+
 	filp->private_data = pr_ctxt;
+
 #ifdef CONFIG_TIDSPBRIDGE_RECOVERY
-	if (!status)
-		atomic_inc(&bridge_cref);
+	atomic_inc(&bridge_cref);
 #endif
+	return 0;
+
+err2:
+	kfree(pr_ctxt->node_id);
+err1:
+	kfree(pr_ctxt);
 	return status;
 }
 
@@ -550,6 +557,8 @@ static int bridge_release(struct inode *ip, struct file *filp)
 	flush_signals(current);
 	drv_remove_all_resources(pr_ctxt);
 	proc_detach(pr_ctxt);
+	kfree(pr_ctxt->node_id);
+	kfree(pr_ctxt->stream_id);
 	kfree(pr_ctxt);
 
 	filp->private_data = NULL;
-- 
1.7.4.1

Re: [PATCH 1/2] staging: tidspbridge: fix bridge_open memory leaks

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Mon, Jan 30, 2012 at 07:20:17PM -0600, Omar Ramirez Luna wrote:
> There are two members of pr_ctxt allocated during bridge_open that
> are never freed resulting in memory leaks, these are stream_id and
> node_id, they are now freed on release of the handle (bridge_release)
> right before freeing pr_ctxt.
> 
> Error path for bridge_open was also fixed since the same variables
> could result in memory leaking due to missing handling of failure
> scenarios. While at it, the indentation changes were introduced to
> avoid interleaved goto statements inside big if blocks.
> 

You mentioned in the cover letter email that you wanted these to go
into stable.  I don't think that is needed here.  These are tiny
leaks that we aren't going to hit in real life.

But if we did want to include them in stable then we would add a tag
to the patch next to the Signed-off-by.

Read Documentation/stable_kernel_rules.txt.  It's short.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] staging: tidspbridge: fix bridge_open memory leaks

[Omar Ramirez Luna]

On Tue, Jan 31, 2012 at 2:17 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jan 30, 2012 at 07:20:17PM -0600, Omar Ramirez Luna wrote:
>> There are two members of pr_ctxt allocated during bridge_open that
>> are never freed resulting in memory leaks, these are stream_id and
>> node_id, they are now freed on release of the handle (bridge_release)
>> right before freeing pr_ctxt.
>>
>> Error path for bridge_open was also fixed since the same variables
>> could result in memory leaking due to missing handling of failure
>> scenarios. While at it, the indentation changes were introduced to
>> avoid interleaved goto statements inside big if blocks.
>>
>
> You mentioned in the cover letter email that you wanted these to go
> into stable.  I don't think that is needed here.  These are tiny
> leaks that we aren't going to hit in real life.

They are hit each time an app opens a handle e.g.:

gst-launch <something>

So after 'X' number of playbacks there won't be memory left.

> But if we did want to include them in stable then we would add a tag
> to the patch next to the Signed-off-by.

I thought first wait for inclusion and then send it to stable list,
given that there are other patches touching this file and I don't know
the order they are going to be picked, it is likely that I have to
rebase it given that I might be the last in the queue.

> Read Documentation/stable_kernel_rules.txt.  It's short.

I had read it before.

I don't mind these to sit outside stable tree if they are not required though.

Thanks,

Omar
--
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 1/2] staging: tidspbridge: fix bridge_open memory leaks

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

On Tue, Jan 31, 2012 at 12:09:17PM -0600, Omar Ramirez Luna wrote:
> On Tue, Jan 31, 2012 at 2:17 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Mon, Jan 30, 2012 at 07:20:17PM -0600, Omar Ramirez Luna wrote:
> >> There are two members of pr_ctxt allocated during bridge_open that
> >> are never freed resulting in memory leaks, these are stream_id and
> >> node_id, they are now freed on release of the handle (bridge_release)
> >> right before freeing pr_ctxt.
> >>
> >> Error path for bridge_open was also fixed since the same variables
> >> could result in memory leaking due to missing handling of failure
> >> scenarios. While at it, the indentation changes were introduced to
> >> avoid interleaved goto statements inside big if blocks.
> >>
> >
> > You mentioned in the cover letter email that you wanted these to go
> > into stable.  I don't think that is needed here.  These are tiny
> > leaks that we aren't going to hit in real life.
> 
> They are hit each time an app opens a handle e.g.:
> 
> gst-launch <something>
> 
> So after 'X' number of playbacks there won't be memory left.
> 

Ah.  That sounds worth while then.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Omar Ramirez Luna]

This structure is still used after it has been freed, since it
is being allocated in probe, calls to free it have been moved to
module's remove routine.

This should fix the follwoing messages when attempting to remove the
module:
 drv_get_first_dev_extension: Failed to retrieve the object handle
 drv_get_first_dev_extension: Failed to retrieve the object handle
 drv_destroy: Failed to store DRV object
 mgr_destroy: Failed to store MGR object

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/staging/tidspbridge/core/tiomap3430.c    |    2 --
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index e1c4492..dde559d 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -1046,8 +1046,6 @@ static int bridge_dev_destroy(struct bridge_dev_context *dev_ctxt)
 
 	/* Free the driver's device context: */
 	kfree(drv_datap->base_img);
-	kfree(drv_datap);
-	dev_set_drvdata(bridge, NULL);
 	kfree((void *)dev_ctxt);
 	return status;
 }
diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 8bac511..385740b 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -410,6 +410,9 @@ static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev)
 		DBC_ASSERT(ret == true);
 	}
 
+	kfree(drv_datap);
+	dev_set_drvdata(bridge, NULL);
+
 func_cont:
 	mem_ext_phys_pool_release();
 
-- 
1.7.4.1

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote:
> This structure is still used after it has been freed, since it
> is being allocated in probe, calls to free it have been moved to
> module's remove routine.
> 
> This should fix the follwoing messages when attempting to remove the
> module:
>  drv_get_first_dev_extension: Failed to retrieve the object handle
>  drv_get_first_dev_extension: Failed to retrieve the object handle
>  drv_destroy: Failed to store DRV object
>  mgr_destroy: Failed to store MGR object
> 

So this is only triggered when you do an rmmod to remove the module?

Probably that's not stable material.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Ramirez Luna, Omar]

On Tue, Jan 31, 2012 at 2:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote:
>> This structure is still used after it has been freed, since it
>> is being allocated in probe, calls to free it have been moved to
>> module's remove routine.
>>
>> This should fix the follwoing messages when attempting to remove the
>> module:
>>  drv_get_first_dev_extension: Failed to retrieve the object handle
>>  drv_get_first_dev_extension: Failed to retrieve the object handle
>>  drv_destroy: Failed to store DRV object
>>  mgr_destroy: Failed to store MGR object
>>
>
> So this is only triggered when you do an rmmod to remove the module?

Yes.

> Probably that's not stable material.

The critical issue is that for a small window the freed memory can be
filled with something else and the driver still might dereference that
memory which no longer belongs to it, thus causing a crash.

But I guess that falls into "this can be a problem", it is ok if it is
being left out of stable.

Thanks,

Omar
--
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 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Tue, Jan 31, 2012 at 12:19:00PM -0600, Ramirez Luna, Omar wrote:
> On Tue, Jan 31, 2012 at 2:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote:
> >> This structure is still used after it has been freed, since it
> >> is being allocated in probe, calls to free it have been moved to
> >> module's remove routine.
> >>
> >> This should fix the follwoing messages when attempting to remove the
> >> module:
> >>  drv_get_first_dev_extension: Failed to retrieve the object handle
> >>  drv_get_first_dev_extension: Failed to retrieve the object handle
> >>  drv_destroy: Failed to store DRV object
> >>  mgr_destroy: Failed to store MGR object
> >>
> >
> > So this is only triggered when you do an rmmod to remove the module?
> 
> Yes.
> 

How often do people rmmod things on a production system?  Hopefully,
never right?

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Felipe Contreras]

On Tue, Jan 31, 2012 at 8:43 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> How often do people rmmod things on a production system?  Hopefully,
> never right?

That's right... At least in recent versions of the tidspbrdige, that
have recovery support. Before, we needed a script to detect MMU
faults, and reload the module =/ IIRC this is still the case on the
Nokia N900.

-- 
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 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Tue, Jan 31, 2012 at 09:39:00PM +0200, Felipe Contreras wrote:
> On Tue, Jan 31, 2012 at 8:43 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > How often do people rmmod things on a production system?  Hopefully,
> > never right?
> 
> That's right... At least in recent versions of the tidspbrdige, that
> have recovery support. Before, we needed a script to detect MMU
> faults, and reload the module =/ IIRC this is still the case on the
> Nokia N900.

If you have a script that's doing rmmods then that's different.  I
didn't know about that.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

[Felipe Contreras]

On Wed, Feb 1, 2012 at 8:58 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Jan 31, 2012 at 09:39:00PM +0200, Felipe Contreras wrote:
>> On Tue, Jan 31, 2012 at 8:43 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > How often do people rmmod things on a production system?  Hopefully,
>> > never right?
>>
>> That's right... At least in recent versions of the tidspbrdige, that
>> have recovery support. Before, we needed a script to detect MMU
>> faults, and reload the module =/ IIRC this is still the case on the
>> Nokia N900.
>
> If you have a script that's doing rmmods then that's different.  I
> didn't know about that.

*Had*, there's no need any more.

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