[PATCH 0/2] lgdt3306a: fix bugs in usb disconnect/reconnect

[PATCH 0/2] lgdt3306a: fix bugs in usb disconnect/reconnect

[Brad Love]

There are a couple problems currently in this driver, when used as
an i2c_device. This patch set addresses the issues one at a time.

First during process of dvb frontend detach release is called, then
if CONFIG_MEDIA_ATTACH is enabled, the usage count is decremented.
Remove is then called, further decrementing the usage count, to negative
if a single device was attached. Patch one uses dvb_attach to keep the
usage count in sync on removal. I'm not sure if there is a less
'hacky' way to handle this. On a previous attempt I just NULL'd out
release in probe, which just hid the issue. Another way of sorting
out was doing a symbol_get on _attach, but the included patch seems
most consistent behaviour.

Next, there is a double kfree of state which can cause oops/GPF/etc
randomly on removal. In the process of dvb frontend detach release
is called before remove. The problem is _release kfree's state, then
right after _remove cleans up members of state, before kfree'ing
state itself. Patch 2 does not kfree state in _release if the
driver was probed and therefore _remove will be called.


Brad Love (2):
  lgdt3306a: Fix module count mismatch on usb unplug
  lgdt3306a: Fix a double kfree on i2c device remove

 drivers/media/dvb-frontends/lgdt3306a.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/2] lgdt3306a: Fix module count mismatch on usb unplug

[Brad Love]

When used as an i2c device there is a module usage count mismatch on
removal, preventing the driver from being used thereafter. dvb_attach
increments the usage count so it is properly balanced on removal.

On disconnect of Hauppauge SoloHD/DualHD before:

lsmod | grep lgdt3306a
lgdt3306a              28672  -1
i2c_mux                16384  1 lgdt3306a

On disconnect of Hauppauge SoloHD/DualHD after:

lsmod | grep lgdt3306a
lgdt3306a              28672  0
i2c_mux                16384  1 lgdt3306a

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index 6356815..d370671 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -2169,7 +2169,7 @@ static int lgdt3306a_probe(struct i2c_client *client,
 			sizeof(struct lgdt3306a_config));
 
 	config->i2c_addr = client->addr;
-	fe = lgdt3306a_attach(config, client->adapter);
+	fe = dvb_attach(lgdt3306a_attach, config, client->adapter);
 	if (fe == NULL) {
 		ret = -ENODEV;
 		goto err_fe;
-- 
2.7.4

[PATCH 2/2] lgdt3306a: Fix a double kfree on i2c device remove

[Brad Love]

Both lgdt33606a_release and lgdt3306a_remove kfree state, but _release is
called first, then _remove operates on states members before kfree'ing it.
This can lead to random oops/GPF/etc on USB disconnect.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index d370671..3642e6e 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -1768,7 +1768,13 @@ static void lgdt3306a_release(struct dvb_frontend *fe)
 	struct lgdt3306a_state *state = fe->demodulator_priv;
 
 	dbg_info("\n");
-	kfree(state);
+
+	/*
+	 * If state->muxc is not NULL, then we are an i2c device
+	 * and lgdt3306a_remove will clean up state
+	 */
+	if (!state->muxc)
+		kfree(state);
 }
 
 static const struct dvb_frontend_ops lgdt3306a_ops;
-- 
2.7.4

Re: [PATCH 2/2] lgdt3306a: Fix a double kfree on i2c device remove

[Matthias Schwarzott]

Am 05.01.2018 um 15:57 schrieb Brad Love:
> Both lgdt33606a_release and lgdt3306a_remove kfree state, but _release is
> called first, then _remove operates on states members before kfree'ing it.
> This can lead to random oops/GPF/etc on USB disconnect.
> 
lgdt3306a_release does nothing but the kfree. So the exact same effect
can be archived by setting state->frontend.ops.release to NULL. This
need to be done already at probe time I think.
lgdt3306a_remove does this, but too late (after the call to release).

Regards
Matthias

> Signed-off-by: Brad Love <brad@nextdimension.cc>
> ---
>  drivers/media/dvb-frontends/lgdt3306a.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
> index d370671..3642e6e 100644
> --- a/drivers/media/dvb-frontends/lgdt3306a.c
> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
> @@ -1768,7 +1768,13 @@ static void lgdt3306a_release(struct dvb_frontend *fe)
>  	struct lgdt3306a_state *state = fe->demodulator_priv;
>  
>  	dbg_info("\n");
> -	kfree(state);
> +
> +	/*
> +	 * If state->muxc is not NULL, then we are an i2c device
> +	 * and lgdt3306a_remove will clean up state
> +	 */
> +	if (!state->muxc)
> +		kfree(state);
>  }
>  
>  static const struct dvb_frontend_ops lgdt3306a_ops;
> 

Re: [PATCH 2/2] lgdt3306a: Fix a double kfree on i2c device remove

[Brad Love]

On 2018-01-08 14:34, Matthias Schwarzott wrote:
> Am 05.01.2018 um 15:57 schrieb Brad Love:
>> Both lgdt33606a_release and lgdt3306a_remove kfree state, but _release is
>> called first, then _remove operates on states members before kfree'ing it.
>> This can lead to random oops/GPF/etc on USB disconnect.
>>
> lgdt3306a_release does nothing but the kfree. So the exact same effect
> can be archived by setting state->frontend.ops.release to NULL. This
> need to be done already at probe time I think.
> lgdt3306a_remove does this, but too late (after the call to release).
>
> Regards
> Matthias

Hi Matthias,

I agree. This was my rationale in the previous patch:

/patch/46328

Both methods handle the issue. I thought the previous
attempt was fairly clean, but it did not pass review, so
I provided this solution.

Cheers,

Brad







>> Signed-off-by: Brad Love <brad@nextdimension.cc>
>> ---
>>  drivers/media/dvb-frontends/lgdt3306a.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
>> index d370671..3642e6e 100644
>> --- a/drivers/media/dvb-frontends/lgdt3306a.c
>> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
>> @@ -1768,7 +1768,13 @@ static void lgdt3306a_release(struct dvb_frontend *fe)
>>  	struct lgdt3306a_state *state = fe->demodulator_priv;
>>  
>>  	dbg_info("\n");
>> -	kfree(state);
>> +
>> +	/*
>> +	 * If state->muxc is not NULL, then we are an i2c device
>> +	 * and lgdt3306a_remove will clean up state
>> +	 */
>> +	if (!state->muxc)
>> +		kfree(state);
>>  }
>>  
>>  static const struct dvb_frontend_ops lgdt3306a_ops;
>>