lkml.org 
[lkml]   [2017]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] driver core: Make it safe to use get_device() if the reference count is zero
From
Date

On 2017/12/14 16:56, Greg KH wrote:
> On Thu, Dec 14, 2017 at 04:27:56PM +0800, Jason Yan wrote:
>>
>>
>> On 2017/12/14 16:10, Greg KH wrote:
>>> On Thu, Dec 14, 2017 at 03:56:46PM +0800, Jason Yan wrote:
>>>>
>>>> On 2017/12/14 15:42, Greg KH wrote:
>>>>> On Thu, Dec 14, 2017 at 11:39:36AM +0800, Jason Yan wrote:
>>>>>> Some driviers may have the chance to increase a reference count that
>>>>>> has dropped to zero when using get_device() because of their design.
>>>>> Then those drivers are broken :)
>>>>>
>>>>>> We have met such a issue with scsi:
>>>>>> https://www.spinics.net/lists/linux-scsi/msg115295.html
>>>>>>
>>>>>> The scsi core will keep the scsi device object in the host list after
>>>>>> it has been deleted and the iterator can still find it. All of the
>>>>>> places where need iterating have to check the state of the scsi device
>>>>>> and this makes a lot of code redundancy and complexity.
>>>>>>
>>>>>> Provide a safe mechanism in get_device() by using
>>>>>> kobject_get_unless_zero().
>>>>>>
>>>>>> Suggested-by: Bart Van Assche <bart.vanassche@wdc.com>
>>>>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>>>>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>> CC: Bart Van Assche <bart.vanassche@wdc.com>
>>>>>> CC: Ewan D. Milne <emilne@redhat.com>
>>>>>> CC: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
>>>>>> CC: Christoph Hellwig <hch@lst.de>
>>>>>> ---
>>>>>> drivers/base/core.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>>>> index 12ebd05..cc74810 100644
>>>>>> --- a/drivers/base/core.c
>>>>>> +++ b/drivers/base/core.c
>>>>>> @@ -1916,7 +1916,7 @@ EXPORT_SYMBOL_GPL(device_register);
>>>>>> */
>>>>>> struct device *get_device(struct device *dev)
>>>>>> {
>>>>>> - return dev ? kobj_to_dev(kobject_get(&dev->kobj)) : NULL;
>>>>>> + return dev && kobject_get_unless_zero(&dev->kobj) ? dev : NULL;
>>>>> I really don't want to do this, the bus the device is on should prevent
>>>>> this from happening.
>>>>>
>>>>> Also, once that reference count drops to zero, the memory should be
>>>>> freed, so you really have a stale pointer here, and this code would fail
>>>>> if you had slab debugging enabled anyway.
>>>>
>>>> Actually I don't want this either. But the design of scsi core will leave
>>>> the scsi device on the host list after it is deleted, and it can be
>>>> found later and the refcount have a very big chance to increase from
>>>> zero again. And after a lot of discussion it seems that the scsi layer
>>>> is difficult to change the situation in the near future.
>>>
>>> Keeping a 'struct device' reference counted chunk of memory on a list
>>> that has a different lifetime rule from that device itself, is crazy.
>>>
>>
>> The lifetime rule is the same. That device itself will delete from the
>> host list in the destructor, scsi_device_dev_release_usercontext(), and
>> the memory will be freed after that. That's why this issue came out.
>>
>>> And yes, I remember how all of this came about, but I really don't have
>>> the time to work on it myself...
>>>
>>>>> So I don't even think this fixes the issue you think it fixes :)
>>>>
>>>> This issue is very easy to reproduce on my machine and I have tested the
>>>> patch and it really fixes the issue.
>>>
>>> Even with slab debugging enabled? If so, what is keeping that memory
>>> from being freed once the reference count drops to 0?
>>>
>>
>> As above, the memory is not freed yet when we increasing the refconunt from
>> zero, so it's nothing to do with slab debugging enabled or not. If
>> we want to free it, we have to grab host lock first to delete it from
>> the list, so if we are grabing the host lock, we can increase the
>> refcount safely from 0 to 1.
>
> Wait, what? Once that refcount goes to 0, the release callback will be
> called, and the memory had better be freed. Any pointer you might still
> have to the structure is now invalid. The fact that you are somehow
> still keeping that pointer around is not ok, and slab debugging should
> have caused the memory to be overwritten and garbage would result if you
> tried to make this call again.
>

I don't know why scsi have this design. Anyone who is familiar with the
history of this design can explain this?

> thanks,
>
> greg k-h
>
> .
>

\
 
 \ /
  Last update: 2017-12-14 10:32    [W:0.088 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site