Linux-Media Archive on lore.kernel.org
 help / Atom feed
* Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak
       [not found] <HK0PR02MB363461179B3A702DB9CAEC55B26A0@HK0PR02MB3634.apcprd02.prod.outlook.com>
@ 2019-02-11 10:38 ` Hans Verkuil (hansverk)
  2019-02-11 10:57   ` Hans Verkuil (hansverk)
  0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil (hansverk) @ 2019-02-11 10:38 UTC (permalink / raw)
  To: Wen Yang, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, LKML

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/4] media: cec-notifier: fix possible object reference leak
  2019-02-11 10:38 ` [PATCH 1/4] media: cec-notifier: fix possible object reference leak Hans Verkuil (hansverk)
@ 2019-02-11 10:57   ` Hans Verkuil (hansverk)
       [not found]     ` <HK0PR02MB3634D1937109CD28ECBF1654B2650@HK0PR02MB3634.apcprd02.prod.outlook.com>
  0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil (hansverk) @ 2019-02-11 10:57 UTC (permalink / raw)
  To: Wen Yang, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, LKML

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
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: 答复: [PATCH 1/4] media: cec-notifier: fix possible object reference leak
       [not found]     ` <HK0PR02MB3634D1937109CD28ECBF1654B2650@HK0PR02MB3634.apcprd02.prod.outlook.com>
@ 2019-02-12 13:39       ` " Hans Verkuil (hansverk)
  0 siblings, 0 replies; 3+ messages in thread
From: Hans Verkuil (hansverk) @ 2019-02-12 13:39 UTC (permalink / raw)
  To: Wen Yang, Hans Verkuil, Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, LKML

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <HK0PR02MB363461179B3A702DB9CAEC55B26A0@HK0PR02MB3634.apcprd02.prod.outlook.com>
2019-02-11 10:38 ` [PATCH 1/4] media: cec-notifier: fix possible object reference leak Hans Verkuil (hansverk)
2019-02-11 10:57   ` Hans Verkuil (hansverk)
     [not found]     ` <HK0PR02MB3634D1937109CD28ECBF1654B2650@HK0PR02MB3634.apcprd02.prod.outlook.com>
2019-02-12 13:39       ` 答复: " Hans Verkuil (hansverk)

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox