On 2020-06-25 21:25, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 8:14 PM
> ...
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 387503e11b..9e3002ddaf 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest
>> *req)
>> }
>> }
>>
>> +typedef struct GetLbaStatusCBData {
>> + uint32_t num_blocks;
>> + uint32_t is_deallocated;
>> + SCSIDiskReq *r;
>> +} GetLbaStatusCBData;
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret);
>> +
>> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int
>> ret)
>> +{
>> + SCSIDiskReq *r = data->r;
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> + assert(r->req.aiocb == NULL);
>> +
>> + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>> + s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
>> +
>> + r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
>> + r->req.cmd.lba *
>> s->qdev.blocksize,
>> + s->qdev.blocksize,
>> + scsi_get_lba_status_complete,
>> data);
>> +}
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret)
>> +{
>> + GetLbaStatusCBData *data = opaque;
>> + SCSIDiskReq *r = data->r;
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> + assert(r->req.aiocb != NULL);
>> + r->req.aiocb = NULL;
>> +
>> + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> + if (scsi_disk_req_check_error(r, ret, true)) {
>> + g_free(data);
>> + goto done;
>> + }
>> +
>> + block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>> + scsi_req_unref(&r->req);
>> + g_free(data);
>> +
>> +done:
>> + aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> +}
>> +
>> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t
>> *outbuf)
>> +{
>> + SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> + GetLbaStatusCBData *data;
>> + uint32_t *num_blocks;
>> + uint32_t *is_deallocated;
>> +
>> + data = g_new0(GetLbaStatusCBData, 1);
>> + data->r = r;
>> + num_blocks = &(data->num_blocks);
>> + is_deallocated = &(data->is_deallocated);
>> +
>> + scsi_req_ref(&r->req);
>> + scsi_get_lba_status_complete_noio(data, 0);
>
> scsi_get_lba_status_complete_noio() looks asynchronous. If the
> BlockDriver yields in .bdrv_co_block_status() then the operation has not
> completed yet when scsi_get_lba_status_complete_noio() returns. It is
> not safe to access the GetLbaStatusCBData data until the async operation
> is complete.
>
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
>
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
> the completion function before g_free(data) is called.
Emm, I need to dig into the code for better understanding your suggestion.
I used to think that I already completely make
'scsi_get_lba_status_complete_noio'
asynchronous.
>> +
>> + /*
>> + * 8 + 16 is the length in bytes of response header and
>> + * one LBA status descriptor
>> + */
>> + memset(outbuf, 0, 8 + 16);
>> + outbuf[3] = 20;
>> + outbuf[8] = (req->cmd.lba >> 56) & 0xff;
>> + outbuf[9] = (req->cmd.lba >> 48) & 0xff;
>> + outbuf[10] = (req->cmd.lba >> 40) & 0xff;
>> + outbuf[11] = (req->cmd.lba >> 32) & 0xff;
>> + outbuf[12] = (req->cmd.lba >> 24) & 0xff;
>> + outbuf[13] = (req->cmd.lba >> 16) & 0xff;
>> + outbuf[14] = (req->cmd.lba >> 8) & 0xff;
>> + outbuf[15] = req->cmd.lba & 0xff;
>> + outbuf[16] = (*num_blocks >> 24) & 0xff;
>> + outbuf[17] = (*num_blocks >> 16) & 0xff;
>> + outbuf[18] = (*num_blocks >> 8) & 0xff;
>> + outbuf[19] = *num_blocks & 0xff;
>> + outbuf[20] = *is_deallocated ? 1 : 0;
>
> SCSI defines 3 values and QEMU can represent all of them:
>
> 0 - mapped or unknown
> 1 - deallocated
> 2 - anchored
>
> See the BDRV_BLOCK_* constants in include/block/block.h. The
> is_deallocated boolean is not enough to represent this state, but the
> bdrv_block_status() return value can be used instead.
I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
I'd like to use these combinations to analyze the bdrv_block_status() return
value:
'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | !
BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
Am I right?
>> +}
>> +
>> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>> {
>> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest
>> *req, uint8_t *buf)
>>
>> /* Protection, exponent and lowest lba field left blank. */
>> break;
>> + } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
>> + if (req->cmd.lba > s->qdev.max_lba) {
>> + goto illegal_lba;
>> + }
>> + scsi_disk_emulate_get_lba_status(req, outbuf);
>> + r->iov.iov_len = req->cmd.xfer;
>> + return r->iov.iov_len;
>
> Is there something tricky going on here with iov_len that prevents us
> from using break here and sharing the functions normal return code path?
Using 'break' here and following the normal return code path cause the assertion
'assert(!r->req.aiocb)'.
In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in
function
scsi_disk_emulate_command, It causes the same assertion if no this 'return'
statement.
I borrowed this idea to avoid the assertion.
>> }
>> trace_scsi_disk_emulate_command_SAI_unsupported();
>> goto illegal_request;
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index 878b4c3581..645014fb0b 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -38,6 +38,7 @@ enum BlockAcctType {
>> BLOCK_ACCT_WRITE,
>> BLOCK_ACCT_FLUSH,
>> BLOCK_ACCT_UNMAP,
>> + BLOCK_ACCT_GET_LBA_STATUS,
>> BLOCK_MAX_IOTYPE,
>> };
>>
>> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
>> index 874176019e..b18377b214 100644
>> --- a/include/scsi/constants.h
>> +++ b/include/scsi/constants.h
>> @@ -154,6 +154,7 @@
>> * SERVICE ACTION IN subcodes
>> */
>> #define SAI_READ_CAPACITY_16 0x10
>> +#define SAI_GET_LBA_STATUS 0x12
>>
>> /*
>> * READ POSITION service action codes
>> --
>> 2.26.0
>>
