Re: [BUG] cdev_put() race condition

Boaz Harrosh
2008-12-18T08:26:12Z

Hans Verkuil wrote:
>>>>> delayin
>>>>> delayout
>>>>> delayout
>> OK Now I do not understand something. How is the double release possible?
>> I guess it is something to do with the complicated chrdev_open() in its
>> inode->i_cdev == NULL case. But still isn't the kref inside kobject
>> suppose to protect me from exactly that?
>
> Only if there is also proper locking to prevent a kref_get from being called
> when the release of a kref_put is in progress. And that's missing in cdev.
>
> Typical scenario: a USB device is disconnected, the driver sees that no
> applications are using it, then it calls cdev_del. This calls cdev_put, the
> cdev's refcount goes to 0 and it will call release. BUT, at this moment an
> application can open the device *again* and chrdev_open() will reuse the
> i_cdev pointer and call cdev_get(p) on it, and this increases the refcount
> from 0 to 1 again. And later it calls cdev_put again and release is called
> a second time.

OK I see.

>
>>>>> Note the duplicate 'delayin' messages. Also note that the cdev struct
>>>>> was allocated by sg.c, so the second cdev cleanup will likely poke
>>>>> into already freed memory.
>>>>>

>> I had a similar problem and solved it in a way that I think should be
>> safe. I'll try and run your tests to make sure. Here is a short
>> description of my solution:
>>
>> MyDevice {
>> ...
>> embed_cdev;
>> embed_kref;
>> ...
>> };
>>
>> OnHotPlug() {
>> ...
>> kref_init(embed_kref); // kref==1
>> cdev_add(embed_cdev); // cdev==1
>> get_(embed_cdev); // cdev++==2
>> }
>>
>> OnFileOpen() { // kernel does // cdev++==n > 2
>> kref_get(embed_kref); // kref++==n > 1
>> }
>>
>> OnFileClose() {
>> kref_put(embed_kref, __release); // kref++==n >= 0
>> } // kernel does // cdev--==n >= 1
>>
>> OnHotRemove() {
>> cdev_del(embed_cdev); // cdev--==1
>> kref_put(embed_kref, __release); // kref--==n >= 0
>> }
>>
>> __release() { // by definition kref==0
>> put_(embed_cdev); // cdev--==0
>> }
>>
>> What do you think?
>
> This won't help either. __release calls cdev_put, the cdev refcount goes to
> 0, the cdev release is called, but at this time someone can open the device
> again. Refcounting in the driver simply won't help since chrdev_open is
> always called before the driver has a chance to check anything.
>

No, But at this point cdev_del has already been called before the final put,
so if a chrdev_open is called while in cdev_release it will not find my
device anymore. I have separated the unmapping of the device from it's final put.

> In addition, I think it is nuts to introduce a kref that just shadows the
> cdev's kref. We should be able to rely on cdev for this.
>

I agree, that's why I like Tejun's patch because in theory we can get
rid of the shadow kref. (Without a layering violation)

> I expect that when you test your driver with this you should hit the same
> race condition. Remember that you need to mknod the device. If you rely on
> udev then this will never happen because udev has removed the device node
> before cdev_del is called. (At least, I think that is always true. I'm no
> expert on this.)
>

OK I see, I do use udev in all my fedora images. I will try to disable it
and test. So far I was unable to reproduce the problem with my device.

>> BTW sg does not use kref and I suspect it might be racy by its own.
>
> It might, but it has nothing to do with this bug.
>
> Regards,
>
> Hans
>

Thanks
Boaz