[7/9] lgdt3306a: Set fe ops.release to NULL if probed

  If release is part of frontend ops then it is called in the
course of dvb_frontend_detach. The process also decrements
the module usage count. The problem is if the lgdt3306a
driver is reached via i2c_new_device, then when it is
eventually destroyed remove is called, which further
decrements the module usage count to negative. After this
occurs the driver is in a bad state and no longer works.
Also fixed by NULLing out the release callback is a double
kfree of state, which introduces arbitrary oopses/GPF.
This problem is only currently reachable via the em28xx driver.

On disconnect of Hauppauge SoloHD before:

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

On disconnect of Hauppauge SoloHD 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 | 1 +
 1 file changed, 1 insertion(+)
  

On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
> If release is part of frontend ops then it is called in the
> course of dvb_frontend_detach. The process also decrements
> the module usage count. The problem is if the lgdt3306a
> driver is reached via i2c_new_device, then when it is
> eventually destroyed remove is called, which further
> decrements the module usage count to negative. After this
> occurs the driver is in a bad state and no longer works.
> Also fixed by NULLing out the release callback is a double
> kfree of state, which introduces arbitrary oopses/GPF.
> This problem is only currently reachable via the em28xx driver.
>
> On disconnect of Hauppauge SoloHD before:
>
> lsmod | grep lgdt3306a
> lgdt3306a              28672  -1
> i2c_mux                16384  1 lgdt3306a
>
> On disconnect of Hauppauge SoloHD 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 | 1 +
>  1 file changed, 1 insertion(+)
>

Brad,

We won't be able to apply this one.  The symptom that you're trying to
fix is indicative of some other problem, probably in the em28xx
driver.  NULL'ing the release callback is not the right thing to do.

-Mike Krufky

> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
> index 6356815..d2477ed 100644
> --- a/drivers/media/dvb-frontends/lgdt3306a.c
> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client,
>
>         i2c_set_clientdata(client, fe->demodulator_priv);
>         state = fe->demodulator_priv;
> +       state->frontend.ops.release = NULL;
>
>         /* create mux i2c adapter for tuner */
>         state->muxc = i2c_mux_alloc(client->adapter, &client->dev,
> --
> 2.7.4
>
  

On 2018-01-04 18:19, Michael Ira Krufky wrote:
> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>> If release is part of frontend ops then it is called in the
>> course of dvb_frontend_detach. The process also decrements
>> the module usage count. The problem is if the lgdt3306a
>> driver is reached via i2c_new_device, then when it is
>> eventually destroyed remove is called, which further
>> decrements the module usage count to negative. After this
>> occurs the driver is in a bad state and no longer works.
>> Also fixed by NULLing out the release callback is a double
>> kfree of state, which introduces arbitrary oopses/GPF.
>> This problem is only currently reachable via the em28xx driver.
>>
>> On disconnect of Hauppauge SoloHD before:
>>
>> lsmod | grep lgdt3306a
>> lgdt3306a              28672  -1
>> i2c_mux                16384  1 lgdt3306a
>>
>> On disconnect of Hauppauge SoloHD 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
> Brad,
>
> We won't be able to apply this one.  The symptom that you're trying to
> fix is indicative of some other problem, probably in the em28xx
> driver.  NULL'ing the release callback is not the right thing to do.
>
> -Mike Krufky

Hey Mike,

Redacting this patch to deal with individually. I will separately send
to the list an example of another bridge driver which exhibits the same
issue, when using the lgdt3306a driver similarly. I don't think this is
solely an em28xx issue. I will also send a patch fixing only the double
free, as that seems to be most important.

Cheers,

Brad





>> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
>> index 6356815..d2477ed 100644
>> --- a/drivers/media/dvb-frontends/lgdt3306a.c
>> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
>> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client,
>>
>>         i2c_set_clientdata(client, fe->demodulator_priv);
>>         state = fe->demodulator_priv;
>> +       state->frontend.ops.release = NULL;
>>
>>         /* create mux i2c adapter for tuner */
>>         state->muxc = i2c_mux_alloc(client->adapter, &client->dev,
>> --
>> 2.7.4
>>
  

Am 05.01.2018 um 01:19 schrieb Michael Ira Krufky:
> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>> If release is part of frontend ops then it is called in the
>> course of dvb_frontend_detach. The process also decrements
>> the module usage count. The problem is if the lgdt3306a
>> driver is reached via i2c_new_device, then when it is
>> eventually destroyed remove is called, which further
>> decrements the module usage count to negative. After this
>> occurs the driver is in a bad state and no longer works.
>> Also fixed by NULLing out the release callback is a double
>> kfree of state, which introduces arbitrary oopses/GPF.
>> This problem is only currently reachable via the em28xx driver.
>>
>> On disconnect of Hauppauge SoloHD before:
>>
>> lsmod | grep lgdt3306a
>> lgdt3306a              28672  -1
>> i2c_mux                16384  1 lgdt3306a
>>
>> On disconnect of Hauppauge SoloHD 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
> 
> Brad,
> 
> We won't be able to apply this one.  The symptom that you're trying to
> fix is indicative of some other problem, probably in the em28xx
> driver.  NULL'ing the release callback is not the right thing to do.
> 

Hi Mike,
Why do you nak this perfectly fine patch?
Let me start to explain.

dvb_attach style drivers have an attach function and for unloading a
.release callback.

i2c-style drivers have a probe and a remove function.

Mixed style drivers must be constructed, that either release or remove
is called, never both.
They both do the same thing, but with different signature.


Now checking lgdt3306a driver:

dvb attach style:
In lgdt3306a_attach the release callback is set to lgdt3306a_release and
no remove exists. Fine.

i2c style probe:
struct i2c_driver contains lgdt3306a_probe and lgdt3306a_remove.
lgdt3306a_probe shares code and calls lgdt3306a_attach, but afterwards
the release callback field must be set to NULL.

This is/was done exactly like this in multiple other drivers as long as
they have been multiple style attachable.

Regards
Matthias

> 
>> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
>> index 6356815..d2477ed 100644
>> --- a/drivers/media/dvb-frontends/lgdt3306a.c
>> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
>> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client,
>>
>>         i2c_set_clientdata(client, fe->demodulator_priv);
>>         state = fe->demodulator_priv;
>> +       state->frontend.ops.release = NULL;
>>
>>         /* create mux i2c adapter for tuner */
>>         state->muxc = i2c_mux_alloc(client->adapter, &client->dev,
>> --
>> 2.7.4
>>
>
  

On Tue, Jan 9, 2018 at 12:17 AM, Matthias Schwarzott <zzam@gentoo.org> wrote:
> Am 05.01.2018 um 01:19 schrieb Michael Ira Krufky:
>> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>>> If release is part of frontend ops then it is called in the
>>> course of dvb_frontend_detach. The process also decrements
>>> the module usage count. The problem is if the lgdt3306a
>>> driver is reached via i2c_new_device, then when it is
>>> eventually destroyed remove is called, which further
>>> decrements the module usage count to negative. After this
>>> occurs the driver is in a bad state and no longer works.
>>> Also fixed by NULLing out the release callback is a double
>>> kfree of state, which introduces arbitrary oopses/GPF.
>>> This problem is only currently reachable via the em28xx driver.
>>>
>>> On disconnect of Hauppauge SoloHD before:
>>>
>>> lsmod | grep lgdt3306a
>>> lgdt3306a              28672  -1
>>> i2c_mux                16384  1 lgdt3306a
>>>
>>> On disconnect of Hauppauge SoloHD 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 | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>
>> Brad,
>>
>> We won't be able to apply this one.  The symptom that you're trying to
>> fix is indicative of some other problem, probably in the em28xx
>> driver.  NULL'ing the release callback is not the right thing to do.
>>
>
> Hi Mike,
> Why do you nak this perfectly fine patch?
> Let me start to explain.
>
> dvb_attach style drivers have an attach function and for unloading a
> .release callback.
>
> i2c-style drivers have a probe and a remove function.
>
> Mixed style drivers must be constructed, that either release or remove
> is called, never both.
> They both do the same thing, but with different signature.
>
>
> Now checking lgdt3306a driver:
>
> dvb attach style:
> In lgdt3306a_attach the release callback is set to lgdt3306a_release and
> no remove exists. Fine.
>
> i2c style probe:
> struct i2c_driver contains lgdt3306a_probe and lgdt3306a_remove.
> lgdt3306a_probe shares code and calls lgdt3306a_attach, but afterwards
> the release callback field must be set to NULL.
>
> This is/was done exactly like this in multiple other drivers as long as
> they have been multiple style attachable.
>
> Regards
> Matthias
>
>>
>>> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
>>> index 6356815..d2477ed 100644
>>> --- a/drivers/media/dvb-frontends/lgdt3306a.c
>>> +++ b/drivers/media/dvb-frontends/lgdt3306a.c
>>> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client,
>>>
>>>         i2c_set_clientdata(client, fe->demodulator_priv);
>>>         state = fe->demodulator_priv;
>>> +       state->frontend.ops.release = NULL;
>>>
>>>         /* create mux i2c adapter for tuner */
>>>         state->muxc = i2c_mux_alloc(client->adapter, &client->dev,
>>> --
>>> 2.7.4
>>>
>>
>

Brad & Matthias,

It makes sense.  This is just a result of a transitional period where
there are multiple APIs for attaching the driver.  Hopefully we can
consolidate soon.

I will look at the other drivers when I have time later on, but you're
probably right, and we will likely end up merging this one after all.

I'll try to get a PR out to Mauro by the weekend.

-Mike