Re: [PATCH v2 2/3] scsi: Make scsi_forget_host() wait for request queue removal

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

 



On 7/1/22 09:25, Mike Christie wrote:
On 6/30/22 4:37 PM, Bart Van Assche wrote:
  	struct scsi_device *sdev;
@@ -1970,8 +1980,21 @@ void scsi_forget_host(struct Scsi_Host *shost)
   restart:
  	spin_lock_irq(shost->host_lock);
  	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
+		if (sdev->sdev_state == SDEV_DEL &&
+		    blk_queue_dead(sdev->request_queue)) {
  			continue;
+		}
+		if (sdev->sdev_state == SDEV_DEL) {
+			get_device(&sdev->sdev_gendev);
+			spin_unlock_irq(shost->host_lock);
+
+			while (!blk_queue_dead(sdev->request_queue))
+				msleep(10);
+
+			spin_lock_irq(shost->host_lock);
+			put_device(&sdev->sdev_gendev);
+			goto restart;
+		}
  		spin_unlock_irq(shost->host_lock);
  		__scsi_remove_device(sdev);
  		goto restart;

Are there 2 ways to hit your issue?

1. Normal case. srp_remove_one frees srp_device. Then all refs
to host are dropped and we call srp_exit_cmd_priv which accesses
the freed srp_device?

You don't need the above patch for this case right.

2. Are you hitting a race? Something did a scsi_remove_device. It
set the device state to SDEV_DEL. It's stuck in blk_cleanup_queue
blk_freeze_queue. Now we do the above code. Without your patch
we just move on by that device. Commands could still be accessed.
With your patch we wait for for that other thread to complete the
device destruction.


Could you also solve both issues by adding a scsi_host_template
scsi_host release function that's called from scsi_host_dev_release. A
new srp_host_release would free structs like the srp device from there.
Or would we still have an issue for #2 where we just don't know how
far blk_freeze_queue has got so commands could still be hitting our
queue_rq function when we are doing scsi_host_dev_release?

I like how your patch makes it so we know after scsi_remove_host
has returned that the device is really gone. Could we have a similar
race as in #2 with someone doing a scsi_remove_device and transport
doing scsi_remove_target?

We would not hit the same use after free from the tag set exit_cmd_priv
being called. But, for example, if we wanted to free some transport level
resources that running commands reference after scsi_target_remove could
we hit a use after free? If so maybe we want to move this wait/check to
__scsi_remove_device?

Hi Mike,

Although I'm not sure that I completely understood the above: my goal with this patch is to make sure that all activity on the host tag set has stopped by the time scsi_forget_host() returns. I do not only want to cover the case where scsi_remove_host() is called by the ib_srp driver but also the case where a SCSI device that was created by the ib_srp driver is removed before scsi_remove_host() is called. Users can trigger this scenario by writing into /sys/class/scsi_device/*/*/delete.

Adding a new callback into scsi_host_dev_release() would not help because the scsi_host_dev_release() call may happen long after scsi_forget_host() returned.

Does this answer your question?

Bart.




[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