[PATCH] xc5000: fix memory corruption when unplugging device

[PATCH] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

This patch addresses a regression introduced in the following patch:

commit 5264a522a597032c009f9143686ebf0fa4e244fb
Author: Shuah Khan <shuahkh@osg.samsung.com>
Date:   Mon Sep 22 21:30:46 2014 -0300
    [media] media: tuner xc5000 - release firmwware from xc5000_release()

The "priv" struct is actually reference counted, so the xc5000_release()
function gets called multiple times for hybrid devices.  Because
release_firmware() was always being called, it would work fine as expected
on the first call but then the second call would corrupt aribtrary memory.

Set the pointer to NULL after releasing so that we don't call
release_firmware() twice.

This problem was detected in the HVR-950q where plugging/unplugging the
device multiple times would intermittently show panics in completely
unrelated areas of the kernel.

Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/tuners/xc5000.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index 40f9db6..74b2092 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -1314,7 +1314,10 @@ static int xc5000_release(struct dvb_frontend *fe)
 
 	if (priv) {
 		cancel_delayed_work(&priv->timer_sleep);
-		release_firmware(priv->firmware);
+		if (priv->firmware) {
+			release_firmware(priv->firmware);
+			priv->firmware = NULL;
+		}
 		hybrid_tuner_release_state(priv);
 	}
 
-- 
1.9.1

Re: [PATCH] xc5000: fix memory corruption when unplugging device

[Shuah Khan]

On 02/24/2015 10:29 AM, Devin Heitmueller wrote:
> This patch addresses a regression introduced in the following patch:
> 
> commit 5264a522a597032c009f9143686ebf0fa4e244fb
> Author: Shuah Khan <shuahkh@osg.samsung.com>
> Date:   Mon Sep 22 21:30:46 2014 -0300
>     [media] media: tuner xc5000 - release firmwware from xc5000_release()
> 
> The "priv" struct is actually reference counted, so the xc5000_release()
> function gets called multiple times for hybrid devices.  Because
> release_firmware() was always being called, it would work fine as expected
> on the first call but then the second call would corrupt aribtrary memory.
> 
> Set the pointer to NULL after releasing so that we don't call
> release_firmware() twice.
> 
> This problem was detected in the HVR-950q where plugging/unplugging the
> device multiple times would intermittently show panics in completely
> unrelated areas of the kernel.

Thanks for finding and fixing the problem.

> 
> Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/tuners/xc5000.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
> index 40f9db6..74b2092 100644
> --- a/drivers/media/tuners/xc5000.c
> +++ b/drivers/media/tuners/xc5000.c
> @@ -1314,7 +1314,10 @@ static int xc5000_release(struct dvb_frontend *fe)
>  
>  	if (priv) {
>  		cancel_delayed_work(&priv->timer_sleep);
> -		release_firmware(priv->firmware);

I would request you to add a comment here indicating the
hybrid case scenario to avoid any future cleanup type work
deciding there is no need to set priv->firmware to null
since priv gets released in hybrid_tuner_release_state(priv);


> +		if (priv->firmware) {
> +			release_firmware(priv->firmware);
> +			priv->firmware = NULL;
> +		}
>  		hybrid_tuner_release_state(priv);
>  	}
>  
> 

Adding Mauro as will to the thread. This should go into stable
as well.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

> I would request you to add a comment here indicating the
> hybrid case scenario to avoid any future cleanup type work
> deciding there is no need to set priv->firmware to null
> since priv gets released in hybrid_tuner_release_state(priv);

No, I'm not going to rebase my tree and regenerate the patch just to
add a comment explaining how hybrid_tuner_[request/release]_state()
works (which, btw, is how it works in all hybrid tuner drivers).  I
already wasted enough of my time tracking down the source of the
memory corruption and providing a fix for this regression.  If you
want to submit a subsequent patch with a comment, be my guest.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] xc5000: fix memory corruption when unplugging device

[Antti Palosaari]

On 02/25/2015 07:56 PM, Devin Heitmueller wrote:
>> I would request you to add a comment here indicating the
>> hybrid case scenario to avoid any future cleanup type work
>> deciding there is no need to set priv->firmware to null
>> since priv gets released in hybrid_tuner_release_state(priv);
>
> No, I'm not going to rebase my tree and regenerate the patch just to
> add a comment explaining how hybrid_tuner_[request/release]_state()
> works (which, btw, is how it works in all hybrid tuner drivers).  I
> already wasted enough of my time tracking down the source of the
> memory corruption and providing a fix for this regression.  If you
> want to submit a subsequent patch with a comment, be my guest.

These are just the issues I would like to implement drivers as standard 
I2C driver model =) Attaching driver for one chip twice is ugly hack!

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

> These are just the issues I would like to implement drivers as standard I2C
> driver model =) Attaching driver for one chip twice is ugly hack!

While I'm not arguing the merits of using the standard I2C driver
model, it won't actually help in this case since we would still need a
structure representing shared state accessible by both the DVB and
V4L2 subsystems.  And that struct will need to be referenced counted,
which is exactly what hybrid_tuner_request_state() does.

In short, what you're proposing doesn't have any relevance to this
case - it just moves the problem to some other mechanism for sharing
private state between two drivers and having to reference count it.
And at least in this case it's done the same way for all the tuner
drivers (as opposed to different tuners re-inventing their own
mechanism for sharing state between the different consumers).

If you ever get around to implementing support for a hybrid device
(where you actually have to worry about how both digital and analog
share a single tuner), you'll appreciate some of these challenges and
why what was done was not as bad/crazy as you might think.

If anything, this little regression is yet another point of evidence
that innocent refactoring and "cleanup" of existing code without
really understanding what it does and/or performing significant
testing can leave everybody worse off than if the well-intentioned
committer had done nothing at all.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] xc5000: fix memory corruption when unplugging device

[Mauro Carvalho Chehab]

Em Wed, 25 Feb 2015 13:37:07 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> > These are just the issues I would like to implement drivers as standard I2C
> > driver model =) Attaching driver for one chip twice is ugly hack!
> 
> While I'm not arguing the merits of using the standard I2C driver
> model, it won't actually help in this case since we would still need a
> structure representing shared state accessible by both the DVB and
> V4L2 subsystems.  And that struct will need to be referenced counted,
> which is exactly what hybrid_tuner_request_state() does.
...
> If you ever get around to implementing support for a hybrid device
> (where you actually have to worry about how both digital and analog
> share a single tuner), you'll appreciate some of these challenges and
> why what was done was not as bad/crazy as you might think.

The I2C model that Antti is proposing may work, but, for that,
we'll very likely need to re-write the tuner core.

Currently, the binding model is:

for analog:

	tuner driver -> tuner-core module <==> V4L2 driver

The interface between V4L2 driver and tuner core is the I2C high
level API.

for digital

	tuner driver <==> DVB driver

The interface between the tuner driver and the DVB driver is the
I2C low level API.

Antti's proposal makes the DVB driver to use the high level I2C API,
with makes the DVB driver a little closer to what V4L2 does.

Yet, for V4L2, the tuner-core module is required.

The binding code at the tuner-core is very complex, as it needs to
talk both V4L2 and DVB "dialogs". This should be simplified.

In other words, If we want to use the same model for all tuners, 
we'll need to rewrite the binding schema to avoid the need of a 
tuner core module, or to replace it by something better/simpler.

For locking the tuner between analog/digital usage, the best
approach seems to be to use the media controller.

Regards,
Mauro