Re: [PATCHv6 01/10] media: add CEC notifier support

2017-04-01 Thread Hans Verkuil 
On 01/04/17 11:39, Russell King - ARM Linux wrote:
> On Sat, Apr 01, 2017 at 11:22:03AM +0200, Hans Verkuil wrote:
>> On 31/03/17 22:46, Russell King - ARM Linux wrote:
>>> On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
 +struct cec_notifier *cec_notifier_get(struct device *dev)
 +{
 +  struct cec_notifier *n;
 +
 +  mutex_lock(_notifiers_lock);
 +  list_for_each_entry(n, _notifiers, head) {
 +  if (n->dev == dev) {
 +  mutex_unlock(_notifiers_lock);
 +  kref_get(>kref);
>>>
>>> Isn't this racy?  What stops one thread trying to get the notifier
>>> while another thread puts the notifier?
>>>
>>
>> Both get and put take the global cec_notifiers_lock mutex.
> 
> No, that doesn't help:
> 
> Thread 0  Thread 1
> mutex_lock()
> list_for_each_entry()
> if()
> mutex_unlock()
>   mutex_lock()
>   kref_put()
>   list_del()
>   kfree()
>   mutex_unlock()
> kref_get()
> 
> So, it's possible that kref_get() can be called on kfree'd memory.
> 

Sorry, you're right. I completely read over the fact that
mutex_unlock(_notifiers_lock) comes too early.

The mutex_unlock now comes after the kref_get. Thanks for reporting this!

Regards,

Hans

Re: [PATCHv6 01/10] media: add CEC notifier support

2017-04-01 Thread Russell King - ARM Linux 
On Sat, Apr 01, 2017 at 11:22:03AM +0200, Hans Verkuil wrote:
> On 31/03/17 22:46, Russell King - ARM Linux wrote:
> > On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
> >> +struct cec_notifier *cec_notifier_get(struct device *dev)
> >> +{
> >> +  struct cec_notifier *n;
> >> +
> >> +  mutex_lock(_notifiers_lock);
> >> +  list_for_each_entry(n, _notifiers, head) {
> >> +  if (n->dev == dev) {
> >> +  mutex_unlock(_notifiers_lock);
> >> +  kref_get(>kref);
> > 
> > Isn't this racy?  What stops one thread trying to get the notifier
> > while another thread puts the notifier?
> > 
> 
> Both get and put take the global cec_notifiers_lock mutex.

No, that doesn't help:

Thread 0Thread 1
mutex_lock()
list_for_each_entry()
if()
mutex_unlock()
mutex_lock()
kref_put()
list_del()
kfree()
mutex_unlock()
kref_get()

So, it's possible that kref_get() can be called on kfree'd memory.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCHv6 01/10] media: add CEC notifier support

2017-04-01 Thread Hans Verkuil 
On 31/03/17 22:46, Russell King - ARM Linux wrote:
> On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
>> +struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> +struct cec_notifier *n;
>> +
>> +mutex_lock(_notifiers_lock);
>> +list_for_each_entry(n, _notifiers, head) {
>> +if (n->dev == dev) {
>> +mutex_unlock(_notifiers_lock);
>> +kref_get(>kref);
> 
> Isn't this racy?  What stops one thread trying to get the notifier
> while another thread puts the notifier?
> 

Both get and put take the global cec_notifiers_lock mutex.

Regards,

Hans

Re: [PATCHv6 01/10] media: add CEC notifier support

2017-03-31 Thread Russell King - ARM Linux 
On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
> +struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + struct cec_notifier *n;
> +
> + mutex_lock(_notifiers_lock);
> + list_for_each_entry(n, _notifiers, head) {
> + if (n->dev == dev) {
> + mutex_unlock(_notifiers_lock);
> + kref_get(>kref);

Isn't this racy?  What stops one thread trying to get the notifier
while another thread puts the notifier?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.