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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux