Re: [PATCH v2 1/3] scsi: protect req->aiocb with AioContext lock

[Kevin Wolf <kwolf@redhat.com>]

Am 10.02.2023 um 15:32 hat Stefan Hajnoczi geschrieben:
> If requests are being processed in the IOThread when a SCSIDevice is
> unplugged, scsi_device_purge_requests() -> scsi_req_cancel_async() races
> with I/O completion callbacks. Both threads load and store req->aiocb.
> This can lead to assert(r->req.aiocb == NULL) failures and undefined
> behavior.
> 
> Protect r->req.aiocb with the AioContext lock to prevent the race.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I tried to check that all accesses of .aiocb are actually protected by
the AioContext lock. I stopped at scsi_read_data(), which asserts that
it is non-NULL, but it is only called as a SCSIReqOps function. I
couldn't find any information on what the locking rules are with
SCSIReqOps functions and didn't feel like reverse engineering scsi-bus
etc. without just asking first.

The same question applies to:
- scsi_read_data
- scsi_write_data
- scsi_disk_emulate_write_data
- scsi_disk_emulate_command

Since these are not callbacks scheduled in the AioContext by scsi-disk
itself, I expect that they are indeed covered. The acquire/release pair
in virtio_scsi_handle_cmd() might actually indirectly cover all of them,
but I haven't checked that.

Either way, the changes that you're making look good:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>