Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751127AbdLNH5F (ORCPT ); Thu, 14 Dec 2017 02:57:05 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:2288 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750720AbdLNH5E (ORCPT ); Thu, 14 Dec 2017 02:57:04 -0500 Subject: Re: [PATCH] driver core: Make it safe to use get_device() if the reference count is zero To: Greg KH References: <20171214033936.6534-1-yanaijie@huawei.com> <20171214074227.GA30120@kroah.com> CC: , , , , Bart Van Assche , "Ewan D . Milne" , "Christoph Hellwig" From: Jason Yan Message-ID: <5A322EBE.9040108@huawei.com> Date: Thu, 14 Dec 2017 15:56:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20171214074227.GA30120@kroah.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.96.203] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2436 Lines: 67 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 >> Signed-off-by: Jason Yan >> CC: Greg Kroah-Hartman >> CC: Bart Van Assche >> CC: Ewan D. Milne >> CC: James E.J. Bottomley >> CC: Christoph Hellwig >> --- >> 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. > 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. > thanks, > > greg k-h > > . >