Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak

Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak

[Hans Verkuil (hansverk)]

On 09/02/2019 03:48, Wen Yang wrote:
> put_device() should be called in cec_notifier_release(),
> since the dev is being passed down to cec_notifier_get_conn(),
> which holds reference. On cec_notifier destruction, it
> should drop the reference to the device.
> 
> Fixes: 6917a7b77413 ("[media] media: add CEC notifier support")
> Signed-off-by: Wen Yang <yellowriver2010@hotmail.com>
> ---
>  drivers/media/cec/cec-notifier.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index dd2078b..621d4ae 100644
> --- a/drivers/media/cec/cec-notifier.c
> +++ b/drivers/media/cec/cec-notifier.c
> @@ -66,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
>  		container_of(kref, struct cec_notifier, kref);
>  
>  	list_del(&n->head);
> +	put_device(n->dev);
>  	kfree(n->conn);
>  	kfree(n);
>  }
> 

Sorry, no. The dev pointer is just a search key that the notifier code looks
for. It is not the notifier's responsibility to take a reference, that would
be the responsibility of the hdmi and cec drivers.

If you can demonstrate that there is an object reference leak, then please
provide the details: it is likely a bug elsewhere and not in the notifier
code.

BTW, your patch series didn't arrive on the linux-media mailinglist for
some reason.

Regards,

	Hans

Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak

[Hans Verkuil (hansverk)]

On 11/02/2019 11:38, Hans Verkuil (hansverk) wrote:
> On 09/02/2019 03:48, Wen Yang wrote:
>> put_device() should be called in cec_notifier_release(),
>> since the dev is being passed down to cec_notifier_get_conn(),
>> which holds reference. On cec_notifier destruction, it
>> should drop the reference to the device.
>>
>> Fixes: 6917a7b77413 ("[media] media: add CEC notifier support")
>> Signed-off-by: Wen Yang <yellowriver2010@hotmail.com>
>> ---
>>  drivers/media/cec/cec-notifier.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
>> index dd2078b..621d4ae 100644
>> --- a/drivers/media/cec/cec-notifier.c
>> +++ b/drivers/media/cec/cec-notifier.c
>> @@ -66,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
>>  		container_of(kref, struct cec_notifier, kref);
>>  
>>  	list_del(&n->head);
>> +	put_device(n->dev);
>>  	kfree(n->conn);
>>  	kfree(n);
>>  }
>>
> 
> Sorry, no. The dev pointer is just a search key that the notifier code looks
> for. It is not the notifier's responsibility to take a reference, that would
> be the responsibility of the hdmi and cec drivers.

Correction: the cec driver should never take a reference of the hdmi device.
It never accesses the HDMI device, it only needs the HDMI device pointer as
a key in the notifier list.

The real problem is that several CEC drivers take a reference of the HDMI device
and never release it. So those drivers need to be fixed.

Regards,

	Hans

> 
> If you can demonstrate that there is an object reference leak, then please
> provide the details: it is likely a bug elsewhere and not in the notifier
> code.
> 
> BTW, your patch series didn't arrive on the linux-media mailinglist for
> some reason.
> 
> Regards,
> 
> 	Hans
> 

Re: 答复: [PATCH 1/4] media: cec-notifier: fix possible object reference leak

[Hans Verkuil (hansverk)]

Hi Wen,

On 2/12/19 2:04 PM, Wen Yang wrote:
> Hi Hans, thank you for your comments.
> I will use my company's mailbox and submit a v2 patch to fix these problems tomorrow.

I made a bunch of patches fixing this yesterday. It's available here:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=cec-refcnt

I haven't posted anything since I had no chance to test it with actual
hardware. But if you have hardware and are able to verify that this
solves the issue, then please let me know so that I can get this merged.

Regards,

	Hans

> 
> Regards,
> 
>         Wen
> 
> ________________________________________
> 发件人: Hans Verkuil (hansverk) <hansverk@cisco.com>
> 发送时间: 2019年2月11日 10:57
> 收件人: Wen Yang; Hans Verkuil; Mauro Carvalho Chehab
> 抄送: Linux Media Mailing List; LKML
> 主题: Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak
> 
> On 11/02/2019 11:38, Hans Verkuil (hansverk) wrote:
>> On 09/02/2019 03:48, Wen Yang wrote:
>>> put_device() should be called in cec_notifier_release(),
>>> since the dev is being passed down to cec_notifier_get_conn(),
>>> which holds reference. On cec_notifier destruction, it
>>> should drop the reference to the device.
>>>
>>> Fixes: 6917a7b77413 ("[media] media: add CEC notifier support")
>>> Signed-off-by: Wen Yang <yellowriver2010@hotmail.com>
>>> ---
>>>  drivers/media/cec/cec-notifier.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
>>> index dd2078b..621d4ae 100644
>>> --- a/drivers/media/cec/cec-notifier.c
>>> +++ b/drivers/media/cec/cec-notifier.c
>>> @@ -66,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
>>>              container_of(kref, struct cec_notifier, kref);
>>>
>>>      list_del(&n->head);
>>> +    put_device(n->dev);
>>>      kfree(n->conn);
>>>      kfree(n);
>>>  }
>>>
>>
>> Sorry, no. The dev pointer is just a search key that the notifier code looks
>> for. It is not the notifier's responsibility to take a reference, that would
>> be the responsibility of the hdmi and cec drivers.
> 
> Correction: the cec driver should never take a reference of the hdmi device.
> It never accesses the HDMI device, it only needs the HDMI device pointer as
> a key in the notifier list.
> 
> The real problem is that several CEC drivers take a reference of the HDMI device
> and never release it. So those drivers need to be fixed.
> 
> Regards,
> 
>         Hans
> 
>>
>> If you can demonstrate that there is an object reference leak, then please
>> provide the details: it is likely a bug elsewhere and not in the notifier
>> code.
>>
>> BTW, your patch series didn't arrive on the linux-media mailinglist for
>> some reason.
>>
>> Regards,
>>
>>       Hans
>>
> 
>