Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Qinglang Miao]

在 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@xxxxxxxxxx> 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.
>