* [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove @ 2020-11-20 7:48 Qinglang Miao 2020-11-25 17:06 ` Benjamin Block 0 siblings, 1 reply; 9+ messages in thread From: Qinglang Miao @ 2020-11-20 7:48 UTC (permalink / raw) To: Steffen Maier, Benjamin Block, Heiko Carstens, Vasily Gorbik, Christian Borntraeger Cc: linux-s390, linux-kernel, Qinglang Miao kfree(port) is called in put_device(&port->dev) so that following use would cause use-after-free bug. The former put_device is redundant for device_unregister contains put_device already. So just remove it to fix this. Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> --- drivers/s390/scsi/zfcp_unit.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c index e67bf7388..664b77853 100644 --- a/drivers/s390/scsi/zfcp_unit.c +++ b/drivers/s390/scsi/zfcp_unit.c @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) scsi_device_put(sdev); } - put_device(&unit->dev); - device_unregister(&unit->dev); return 0; -- 2.23.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-20 7:48 [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove Qinglang Miao @ 2020-11-25 17:06 ` Benjamin Block 2020-11-26 1:27 ` Qinglang Miao 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Block @ 2020-11-25 17:06 UTC (permalink / raw) To: Qinglang Miao Cc: Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: > kfree(port) is called in put_device(&port->dev) so that following > use would cause use-after-free bug. > > The former put_device is redundant for device_unregister contains > put_device already. So just remove it to fix this. > > Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> > --- > drivers/s390/scsi/zfcp_unit.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c > index e67bf7388..664b77853 100644 > --- a/drivers/s390/scsi/zfcp_unit.c > +++ b/drivers/s390/scsi/zfcp_unit.c > @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) > scsi_device_put(sdev); > } > > - put_device(&unit->dev); > - > device_unregister(&unit->dev); > > return 0; Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put that away again. -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-25 17:06 ` Benjamin Block @ 2020-11-26 1:27 ` Qinglang Miao 2020-11-26 8:13 ` Cornelia Huck 0 siblings, 1 reply; 9+ messages in thread From: Qinglang Miao @ 2020-11-26 1:27 UTC (permalink / raw) To: Benjamin Block Cc: Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi 在 2020/11/26 1:06, Benjamin Block 写道: > On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: >> kfree(port) is called in put_device(&port->dev) so that following >> use would cause use-after-free bug. >> >> The former put_device is redundant for device_unregister contains >> put_device already. So just remove it to fix this. >> >> Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> >> --- >> drivers/s390/scsi/zfcp_unit.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c >> index e67bf7388..664b77853 100644 >> --- a/drivers/s390/scsi/zfcp_unit.c >> +++ b/drivers/s390/scsi/zfcp_unit.c >> @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) >> scsi_device_put(sdev); >> } >> >> - put_device(&unit->dev); >> - >> device_unregister(&unit->dev); >> >> return 0; > > Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We > explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put > that away again. > Sorry, Benjamin, I don't think so, because device_unregister calls put_device inside. It seem's that another put_device before or after device_unregister is useless and even might cause an use-after-free. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-26 1:27 ` Qinglang Miao @ 2020-11-26 8:13 ` Cornelia Huck 2020-11-26 9:42 ` Benjamin Block 0 siblings, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2020-11-26 8:13 UTC (permalink / raw) To: Qinglang Miao Cc: Benjamin Block, Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi On Thu, 26 Nov 2020 09:27:41 +0800 Qinglang Miao <miaoqinglang@huawei.com> wrote: > 在 2020/11/26 1:06, Benjamin Block 写道: > > On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: > >> kfree(port) is called in put_device(&port->dev) so that following > >> use would cause use-after-free bug. > >> > >> The former put_device is redundant for device_unregister contains > >> put_device already. So just remove it to fix this. > >> > >> Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") > >> Reported-by: Hulk Robot <hulkci@huawei.com> > >> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> > >> --- > >> drivers/s390/scsi/zfcp_unit.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c > >> index e67bf7388..664b77853 100644 > >> --- a/drivers/s390/scsi/zfcp_unit.c > >> +++ b/drivers/s390/scsi/zfcp_unit.c > >> @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) > >> scsi_device_put(sdev); > >> } > >> > >> - put_device(&unit->dev); > >> - > >> device_unregister(&unit->dev); > >> >> return 0; > > > > Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We > > explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put > > that away again. > > > Sorry, Benjamin, I don't think so, because device_unregister calls > put_device inside. > > It seem's that another put_device before or after device_unregister is > useless and even might cause an use-after-free. The issue here (and in the other patches that I had commented on) is that the references have different origins. device_register() acquires a reference, and that reference is given up when you call device_unregister(). However, the code here grabs an extra reference, and it of course has to give it up again when it no longer needs it. This is something that is not that easy to spot by an automated check, I guess? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-26 8:13 ` Cornelia Huck @ 2020-11-26 9:42 ` Benjamin Block 2020-11-26 12:07 ` Qinglang Miao 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Block @ 2020-11-26 9:42 UTC (permalink / raw) To: Qinglang Miao Cc: Cornelia Huck, Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote: > On Thu, 26 Nov 2020 09:27:41 +0800 > Qinglang Miao <miaoqinglang@huawei.com> wrote: > > > 在 2020/11/26 1:06, Benjamin Block 写道: > > > On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: > > >> kfree(port) is called in put_device(&port->dev) so that following > > >> use would cause use-after-free bug. > > >> > > >> The former put_device is redundant for device_unregister contains > > >> put_device already. So just remove it to fix this. > > >> > > >> Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") > > >> Reported-by: Hulk Robot <hulkci@huawei.com> > > >> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> > > >> --- > > >> drivers/s390/scsi/zfcp_unit.c | 2 -- > > >> 1 file changed, 2 deletions(-) > > >> > > >> diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c > > >> index e67bf7388..664b77853 100644 > > >> --- a/drivers/s390/scsi/zfcp_unit.c > > >> +++ b/drivers/s390/scsi/zfcp_unit.c > > >> @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) > > >> scsi_device_put(sdev); > > >> } > > >> > > >> - put_device(&unit->dev); > > >> - > > >> device_unregister(&unit->dev); > > >> >> return 0; > > > > > > Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We > > > explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put > > > that away again. > > > > > Sorry, Benjamin, I don't think so, because device_unregister calls > > put_device inside. > > > > It seem's that another put_device before or after device_unregister is > > useless and even might cause an use-after-free. > > The issue here (and in the other patches that I had commented on) is > that the references have different origins. device_register() acquires > a reference, and that reference is given up when you call > device_unregister(). However, the code here grabs an extra reference, > and it of course has to give it up again when it no longer needs it. > > This is something that is not that easy to spot by an automated check, > I guess? > Indeed. I do think the two patches for zfcp have merit, but not by simply removing the put_device(), but by moving it. For this patch in particular, I'd think the "proper logic" would be to move the `put_device()` to after the `device_unregister()`: device_unregister(&unit->dev); put_device(&unit->dev); return 0; As Cornelia pointed out, the extra `get_device()` we do in `_zfcp_unit_find()` needs to be reversed, otherwise we have a dangling reference and probably some sort of memory-/resource-leak. Let's go by example. If we assume the reference count of `unit->dev` is R, and the function starts with R = 1 (otherwise the deivce would've been freed already), we get: int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) { struct zfcp_unit *unit; struct scsi_device *sdev; write_lock_irq(&port->unit_list_lock); // unit->dev (R = 1) unit = _zfcp_unit_find(port, fcp_lun); // get_device(&unit->dev) // unit->dev (R = 2) if (unit) list_del(&unit->list); write_unlock_irq(&port->unit_list_lock); if (!unit) return -EINVAL; sdev = zfcp_unit_sdev(unit); if (sdev) { scsi_remove_device(sdev); scsi_device_put(sdev); } // unit->dev (R = 2) put_device(&unit->dev); // unit->dev (R = 1) device_unregister(&unit->dev); // unit->dev (R = 0) return 0; } If we now apply this patch, we'd end up with R = 1 after `device_unregister()`, and the device would not be properly removed. If you still think that's wrong, then you'll need to better explain why. -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-26 9:42 ` Benjamin Block @ 2020-11-26 12:07 ` Qinglang Miao 2020-11-26 15:12 ` Benjamin Block 0 siblings, 1 reply; 9+ messages in thread From: Qinglang Miao @ 2020-11-26 12:07 UTC (permalink / raw) To: Benjamin Block, Cornelia Huck Cc: Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi 在 2020/11/26 17:42, Benjamin Block 写道: > On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote: >> On Thu, 26 Nov 2020 09:27:41 +0800 >> Qinglang Miao <miaoqinglang@huawei.com> wrote: >> >>> 在 2020/11/26 1:06, Benjamin Block 写道: >>>> On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: >>>>> kfree(port) is called in put_device(&port->dev) so that following >>>>> use would cause use-after-free bug. >>>>> >>>>> The former put_device is redundant for device_unregister contains >>>>> put_device already. So just remove it to fix this. >>>>> >>>>> Fixes: 86bdf218a717 ("[SCSI] zfcp: cleanup unit sysfs attribute usage") >>>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>>> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> >>>>> --- >>>>> drivers/s390/scsi/zfcp_unit.c | 2 -- >>>>> 1 file changed, 2 deletions(-) >>>>> >>>>> diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c >>>>> index e67bf7388..664b77853 100644 >>>>> --- a/drivers/s390/scsi/zfcp_unit.c >>>>> +++ b/drivers/s390/scsi/zfcp_unit.c >>>>> @@ -255,8 +255,6 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) >>>>> scsi_device_put(sdev); >>>>> } >>>>> >>>>> - put_device(&unit->dev); >>>>> - >>>>> device_unregister(&unit->dev); >>>>> >> return 0; >>>> >>>> Same as in the other mail for `zfcp_sysfs_port_remove_store()`. We >>>> explicitly get a new ref in `_zfcp_unit_find()`, so we also need to put >>>> that away again. >>>> >>> Sorry, Benjamin, I don't think so, because device_unregister calls >>> put_device inside. >>> >>> It seem's that another put_device before or after device_unregister is >>> useless and even might cause an use-after-free. >> >> The issue here (and in the other patches that I had commented on) is >> that the references have different origins. device_register() acquires >> a reference, and that reference is given up when you call >> device_unregister(). However, the code here grabs an extra reference, >> and it of course has to give it up again when it no longer needs it. >> >> This is something that is not that easy to spot by an automated check, >> I guess? >> > > Indeed. > > I do think the two patches for zfcp have merit, but not by simply > removing the put_device(), but by moving it. > > For this patch in particular, I'd think the "proper logic" would be to > move the `put_device()` to after the `device_unregister()`: > > device_unregister(&unit->dev); > put_device(&unit->dev); > > return 0; > > As Cornelia pointed out, the extra `get_device()` we do in > `_zfcp_unit_find()` needs to be reversed, otherwise we have a dangling > reference and probably some sort of memory-/resource-leak. > > Let's go by example. If we assume the reference count of `unit->dev` is > R, and the function starts with R = 1 (otherwise the deivce would've > been freed already), we get: > > int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) > { > struct zfcp_unit *unit; > struct scsi_device *sdev; > > write_lock_irq(&port->unit_list_lock); > // unit->dev (R = 1) > unit = _zfcp_unit_find(port, fcp_lun); > // get_device(&unit->dev) > // unit->dev (R = 2) > if (unit) > list_del(&unit->list); > write_unlock_irq(&port->unit_list_lock); > > if (!unit) > return -EINVAL; > > sdev = zfcp_unit_sdev(unit); > if (sdev) { > scsi_remove_device(sdev); > scsi_device_put(sdev); > } > > // unit->dev (R = 2) > put_device(&unit->dev); > // unit->dev (R = 1) > device_unregister(&unit->dev); > // unit->dev (R = 0) > > return 0; > } > > If we now apply this patch, we'd end up with R = 1 after > `device_unregister()`, and the device would not be properly removed. > > If you still think that's wrong, then you'll need to better explain why. > Hi Banjamin and Cornelia, Your replies make me reliaze that I've been holding a mistake understanding of put_device() as well as reference count. Thanks for you two's patient explanation !! BTW, should I send a v2 on these two patches to move the position of put_device()? > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-26 12:07 ` Qinglang Miao @ 2020-11-26 15:12 ` Benjamin Block 2020-11-27 9:21 ` Steffen Maier 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Block @ 2020-11-26 15:12 UTC (permalink / raw) To: Qinglang Miao Cc: Cornelia Huck, Steffen Maier, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote: > 在 2020/11/26 17:42, Benjamin Block 写道: > > On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote: > > > On Thu, 26 Nov 2020 09:27:41 +0800 > > > Qinglang Miao <miaoqinglang@huawei.com> wrote: > > > > 在 2020/11/26 1:06, Benjamin Block 写道: > > > > > On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: .... > > Let's go by example. If we assume the reference count of `unit->dev` is > > R, and the function starts with R = 1 (otherwise the deivce would've > > been freed already), we get: > > > > int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) > > { > > struct zfcp_unit *unit; > > struct scsi_device *sdev; > > write_lock_irq(&port->unit_list_lock); > > // unit->dev (R = 1) > > unit = _zfcp_unit_find(port, fcp_lun); > > // get_device(&unit->dev) > > // unit->dev (R = 2) > > if (unit) > > list_del(&unit->list); > > write_unlock_irq(&port->unit_list_lock); > > if (!unit) > > return -EINVAL; > > sdev = zfcp_unit_sdev(unit); > > if (sdev) { > > scsi_remove_device(sdev); > > scsi_device_put(sdev); > > } > > // unit->dev (R = 2) > > put_device(&unit->dev); > > // unit->dev (R = 1) > > device_unregister(&unit->dev); > > // unit->dev (R = 0) > > return 0; > > } > > > > If we now apply this patch, we'd end up with R = 1 after > > `device_unregister()`, and the device would not be properly removed. > > > > If you still think that's wrong, then you'll need to better explain why. > > > Hi Banjamin and Cornelia, > > Your replies make me reliaze that I've been holding a mistake understanding > of put_device() as well as reference count. > > Thanks for you two's patient explanation !! > > BTW, should I send a v2 on these two patches to move the position of > put_device()? Feel free to do so. I think having the `put_device()` call after `device_unregister()` in both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more natural, because it ought to be the last time we touch the object in both functions. -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-26 15:12 ` Benjamin Block @ 2020-11-27 9:21 ` Steffen Maier 2020-12-01 2:46 ` Qinglang Miao 0 siblings, 1 reply; 9+ messages in thread From: Steffen Maier @ 2020-11-27 9:21 UTC (permalink / raw) To: Benjamin Block, Qinglang Miao Cc: Cornelia Huck, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi On 11/26/20 4:12 PM, Benjamin Block wrote: > On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote: >> 在 2020/11/26 17:42, Benjamin Block 写道: >>> On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote: >>>> On Thu, 26 Nov 2020 09:27:41 +0800 >>>> Qinglang Miao <miaoqinglang@huawei.com> wrote: >>>>> 在 2020/11/26 1:06, Benjamin Block 写道: >>>>>> On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: > .... >>> Let's go by example. If we assume the reference count of `unit->dev` is >>> R, and the function starts with R = 1 (otherwise the deivce would've >>> been freed already), we get: >>> >>> int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) >>> { >>> struct zfcp_unit *unit; >>> struct scsi_device *sdev; >>> write_lock_irq(&port->unit_list_lock); >>> // unit->dev (R = 1) >>> unit = _zfcp_unit_find(port, fcp_lun); >>> // get_device(&unit->dev) >>> // unit->dev (R = 2) >>> if (unit) >>> list_del(&unit->list); >>> write_unlock_irq(&port->unit_list_lock); >>> if (!unit) >>> return -EINVAL; >>> sdev = zfcp_unit_sdev(unit); >>> if (sdev) { >>> scsi_remove_device(sdev); >>> scsi_device_put(sdev); >>> } >>> // unit->dev (R = 2) >>> put_device(&unit->dev); >>> // unit->dev (R = 1) >>> device_unregister(&unit->dev); >>> // unit->dev (R = 0) >>> return 0; >>> } >>> >>> If we now apply this patch, we'd end up with R = 1 after >>> `device_unregister()`, and the device would not be properly removed. >>> >>> If you still think that's wrong, then you'll need to better explain why. >>> >> Hi Banjamin and Cornelia, >> >> Your replies make me reliaze that I've been holding a mistake understanding >> of put_device() as well as reference count. >> >> Thanks for you two's patient explanation !! >> >> BTW, should I send a v2 on these two patches to move the position of >> put_device()? > > Feel free to do so. > > I think having the `put_device()` call after `device_unregister()` in > both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more > natural, because it ought to be the last time we touch the object in > both functions. If you move put_device(), you could add a comment like we did here to explain which (hidden) get_device is undone: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca ("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only sdevs)") So in this patch it could be: put_device(&unit->dev); /* undo _zfcp_unit_find() */ And in the other patch it could be: put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */ Then it would be clearer next time somebody looks at the code. Especially for the other patch on zfcp_sysfs_port_remove_store() moving the put_device(&port->dev) to at least *after* the call of zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner to me. Along the idead of passing the port to zfcp_erp_port_shutdown with the reference we got from zfcp_get_port_by_wwpn(). That said, the current code is of course still correct as we currently have the port ref of the earlier device_register so passing the port to zfcp_erp_port_shutdown() is safe. If we wanted to make the gets and puts nicely nested, then we could move the puts to just before the device_unregister, but that's bike shedding: device_register() --+ get_device() --+ | put_device() --+ | device_unregister() --+ Benjamin's suggested move location works for me, too. After all, the kdoc of device_unregister explicitly mentions the possibility that other refs might continue to exist after device_unregister was called: device_register() --+ get_device() ---------|--+ device_unregister() --+ | put_device() ------------+ -- Mit freundlichen Gruessen / Kind regards Steffen Maier Linux on IBM Z Development https://www.ibm.com/privacy/us/en/ IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Matthias Hartmann Geschaeftsfuehrung: Dirk Wittkopp Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove 2020-11-27 9:21 ` Steffen Maier @ 2020-12-01 2:46 ` Qinglang Miao 0 siblings, 0 replies; 9+ messages in thread From: Qinglang Miao @ 2020-12-01 2:46 UTC (permalink / raw) To: Steffen Maier Cc: Benjamin Block, Cornelia Huck, Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390, linux-kernel, linux-scsi 在 2020/11/27 17:21, Steffen Maier 写道: > On 11/26/20 4:12 PM, Benjamin Block wrote: >> On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote: >>> 在 2020/11/26 17:42, Benjamin Block 写道: >>>> On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote: >>>>> On Thu, 26 Nov 2020 09:27:41 +0800 >>>>> Qinglang Miao <miaoqinglang@huawei.com> wrote: >>>>>> 在 2020/11/26 1:06, Benjamin Block 写道: >>>>>>> On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote: >> .... >>>> Let's go by example. If we assume the reference count of `unit->dev` is >>>> R, and the function starts with R = 1 (otherwise the deivce would've >>>> been freed already), we get: >>>> >>>> int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun) >>>> { >>>> struct zfcp_unit *unit; >>>> struct scsi_device *sdev; >>>> write_lock_irq(&port->unit_list_lock); >>>> // unit->dev (R = 1) >>>> unit = _zfcp_unit_find(port, fcp_lun); >>>> // get_device(&unit->dev) >>>> // unit->dev (R = 2) >>>> if (unit) >>>> list_del(&unit->list); >>>> write_unlock_irq(&port->unit_list_lock); >>>> if (!unit) >>>> return -EINVAL; >>>> sdev = zfcp_unit_sdev(unit); >>>> if (sdev) { >>>> scsi_remove_device(sdev); >>>> scsi_device_put(sdev); >>>> } >>>> // unit->dev (R = 2) >>>> put_device(&unit->dev); >>>> // unit->dev (R = 1) >>>> device_unregister(&unit->dev); >>>> // unit->dev (R = 0) >>>> return 0; >>>> } >>>> >>>> If we now apply this patch, we'd end up with R = 1 after >>>> `device_unregister()`, and the device would not be properly removed. >>>> >>>> If you still think that's wrong, then you'll need to better explain >>>> why. >>>> >>> Hi Banjamin and Cornelia, >>> >>> Your replies make me reliaze that I've been holding a mistake >>> understanding >>> of put_device() as well as reference count. >>> >>> Thanks for you two's patient explanation !! >>> >>> BTW, should I send a v2 on these two patches to move the position of >>> put_device()? >> >> Feel free to do so. >> >> I think having the `put_device()` call after `device_unregister()` in >> both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more >> natural, because it ought to be the last time we touch the object in >> both functions. > > If you move put_device(), you could add a comment like we did here to > explain which (hidden) get_device is undone: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca > > ("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only > sdevs)") > So in this patch it could be: > put_device(&unit->dev); /* undo _zfcp_unit_find() */ > And in the other patch it could be: > put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */ > Then it would be clearer next time somebody looks at the code. > Hi, Steffen Sorry I didn't notice this mail when I sent a patch to move put_device, you suggestion sounds resonable to me, so I send a v2 to add comments. Thanks. > Especially for the other patch on zfcp_sysfs_port_remove_store() moving > the put_device(&port->dev) to at least *after* the call of > zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner > to me. Along the idead of passing the port to zfcp_erp_port_shutdown > with the reference we got from zfcp_get_port_by_wwpn(). That said, the > current code is of course still correct as we currently have the port > ref of the earlier device_register so passing the port to > zfcp_erp_port_shutdown() is safe. > > If we wanted to make the gets and puts nicely nested, then we could move > the puts to just before the device_unregister, but that's bike shedding: > device_register() --+ > get_device() --+ | > put_device() --+ | > device_unregister() --+ > > Benjamin's suggested move location works for me, too. After all, the > kdoc of device_unregister explicitly mentions the possibility that other > refs might continue to exist after device_unregister was called: > device_register() --+ > get_device() ---------|--+ > device_unregister() --+ | > put_device() ------------+ Glad to know your opinions, I'd like to take this one on my patch. > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-01 2:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 7:48 [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove Qinglang Miao 2020-11-25 17:06 ` Benjamin Block 2020-11-26 1:27 ` Qinglang Miao 2020-11-26 8:13 ` Cornelia Huck 2020-11-26 9:42 ` Benjamin Block 2020-11-26 12:07 ` Qinglang Miao 2020-11-26 15:12 ` Benjamin Block 2020-11-27 9:21 ` Steffen Maier 2020-12-01 2:46 ` Qinglang Miao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).