| Message ID | 20180508074743.13622-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai: > The write operations to "cmnd->result" and "cmnd->scsi_done" > are protected by the lock on line 642-643, but the write operations > to these data on line 634-635 are not protected by the lock. > Thus, there may exist a data race for "cmnd->result" > and "cmnd->scsi_done". No, the write operations need no lock. The low level driver at this point owns the command. We cannot race with abort() for a command within queuecommand(). We take the lock where we take it to protect dev->resetting. I don't see why the scope of the lock would need to be enlarged. Regards Oliver > To fix this data race, the write operations on line 634-635 > should be also protected by the lock. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Nacked-by: Oliver Neukum <oneukum@suse.com>
On 2018/5/8 16:27, Oliver Neukum wrote: > Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai: >> The write operations to "cmnd->result" and "cmnd->scsi_done" >> are protected by the lock on line 642-643, but the write operations >> to these data on line 634-635 are not protected by the lock. >> Thus, there may exist a data race for "cmnd->result" >> and "cmnd->scsi_done". > No, > > the write operations need no lock. The low level driver at this point > owns the command. We cannot race with abort() for a command within > queuecommand(). We take the lock where we take it to protect > dev->resetting. > > I don't see why the scope of the lock would need to be enlarged. Okay, thanks for your reply and explanation. Best wishes, Jia-Ju Bai
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 6034c39b67d1..dde7a43ad491 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -627,17 +627,18 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd, if (cmnd->device->host->host_self_blocked) return SCSI_MLQUEUE_DEVICE_BUSY; + spin_lock_irqsave(&devinfo->lock, flags); + if ((devinfo->flags & US_FL_NO_ATA_1X) && (cmnd->cmnd[0] == ATA_12 || cmnd->cmnd[0] == ATA_16)) { memcpy(cmnd->sense_buffer, usb_stor_sense_invalidCDB, sizeof(usb_stor_sense_invalidCDB)); cmnd->result = SAM_STAT_CHECK_CONDITION; cmnd->scsi_done(cmnd); + spin_unlock_irqrestore(&devinfo->lock, flags); return 0; } - spin_lock_irqsave(&devinfo->lock, flags); - if (devinfo->resetting) { cmnd->result = DID_ERROR << 16; cmnd->scsi_done(cmnd);
The write operations to "cmnd->result" and "cmnd->scsi_done" are protected by the lock on line 642-643, but the write operations to these data on line 634-635 are not protected by the lock. Thus, there may exist a data race for "cmnd->result" and "cmnd->scsi_done". To fix this data race, the write operations on line 634-635 should be also protected by the lock. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/usb/storage/uas.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)