[PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio

[PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio

[Kit Westneat]

check_lba_range expects sectors to be expressed in original qdev blocksize, but
scsi_unmap_complete_noio was translating them to 512 block sizes, which was
causing sense errors in the larger LBAs in devices using a 4k block size.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345
Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
---
 hw/scsi/scsi-disk.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 3580e7ee61..e8a547dbb7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1582,6 +1582,7 @@ invalid_field:
     scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }
 
+/* sector_num and nb_sectors expected to be in qdev blocksize */
 static inline bool check_lba_range(SCSIDiskState *s,
                                    uint64_t sector_num, uint32_t nb_sectors)
 {
@@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
     assert(r->req.aiocb == NULL);
 
     if (data->count > 0) {
-        r->sector = ldq_be_p(&data->inbuf[0])
-            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-        r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL)
-            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-        if (!check_lba_range(s, r->sector, r->sector_count)) {
+        uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
+        uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
+        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+
+        if (!check_lba_range(s, sector_num, nb_sectors)) {
             block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
                                BLOCK_ACCT_UNMAP);
             scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
-- 
2.26.3

Re: [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio

[Paolo Bonzini]

On 21/05/21 16:28, Kit Westneat wrote:
> check_lba_range expects sectors to be expressed in original qdev blocksize, but
> scsi_unmap_complete_noio was translating them to 512 block sizes, which was
> causing sense errors in the larger LBAs in devices using a 4k block size.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345
> Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
> ---
>   hw/scsi/scsi-disk.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 3580e7ee61..e8a547dbb7 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1582,6 +1582,7 @@ invalid_field:
>       scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>   }
>   
> +/* sector_num and nb_sectors expected to be in qdev blocksize */
>   static inline bool check_lba_range(SCSIDiskState *s,
>                                      uint64_t sector_num, uint32_t nb_sectors)
>   {
> @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>       assert(r->req.aiocb == NULL);
>   
>       if (data->count > 0) {
> -        r->sector = ldq_be_p(&data->inbuf[0])
> -            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> -        r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL)
> -            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> -        if (!check_lba_range(s, r->sector, r->sector_count)) {
> +        uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
> +        uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> +        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +
> +        if (!check_lba_range(s, sector_num, nb_sectors)) {
>               block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
>                                  BLOCK_ACCT_UNMAP);
>               scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
> 

Queued, thanks!

If you want to produce a testcase for tests/qtest/virtio-scsi-test.c, I 
won't complain.

Paolo

Re: [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio

[Kit Westneat]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

I started looking into adding tests, but it doesn't look like blkdebug
supports changing the blocksize. I saw that it supports changing the memory
alignment to 4k via the align parameter, but it doesn't implement
bdrv_probe_blocksizes so the blocksize stays at 512.

Would it be worth adding a blocksize parameter to blkdebug? (or maybe
log-blksize & phys-blksize?) Or is there another way to set the blocksize
of a qdev?

Thanks,
Kit



On Fri, May 21, 2021 at 11:20 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 21/05/21 16:28, Kit Westneat wrote:
> > check_lba_range expects sectors to be expressed in original qdev
> blocksize, but
> > scsi_unmap_complete_noio was translating them to 512 block sizes, which
> was
> > causing sense errors in the larger LBAs in devices using a 4k block size.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345
> > Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
> > ---
> >   hw/scsi/scsi-disk.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 3580e7ee61..e8a547dbb7 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -1582,6 +1582,7 @@ invalid_field:
> >       scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
> >   }
> >
> > +/* sector_num and nb_sectors expected to be in qdev blocksize */
> >   static inline bool check_lba_range(SCSIDiskState *s,
> >                                      uint64_t sector_num, uint32_t
> nb_sectors)
> >   {
> > @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData
> *data, int ret)
> >       assert(r->req.aiocb == NULL);
> >
> >       if (data->count > 0) {
> > -        r->sector = ldq_be_p(&data->inbuf[0])
> > -            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> > -        r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL)
> > -            * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> > -        if (!check_lba_range(s, r->sector, r->sector_count)) {
> > +        uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
> > +        uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> > +        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> > +        r->sector_count = nb_sectors * (s->qdev.blocksize /
> BDRV_SECTOR_SIZE);
> > +
> > +        if (!check_lba_range(s, sector_num, nb_sectors)) {
> >               block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
> >                                  BLOCK_ACCT_UNMAP);
> >               scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
> >
>
> Queued, thanks!
>
> If you want to produce a testcase for tests/qtest/virtio-scsi-test.c, I
> won't complain.
>
> Paolo
>
>

[-- Attachment #2: Type: text/html, Size: 3760 bytes --]