Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Stefan Hajnoczi]

On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@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