On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote: > Signed-off-by: Lin Ma <l...@suse.com> > --- > hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++ > include/block/accounting.h | 1 + > include/scsi/constants.h | 1 + > 3 files changed, 92 insertions(+) > > 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. > + > + /* > + * 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. > +} > + > 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? > } > 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 >
signature.asc
Description: PGP signature