[Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Eric Blake]

When qemu uses iscsi devices in sg mode, iscsilun->block_size
is left at 0.  Prior to commits cf081fca and similar, when
block limits were tracked in sectors, this did not matter:
various block limits were just left at 0.  But when we started
scaling by block size, this caused SIGFPE.

One possible solution for SG mode is to just blindly skip ALL
of iscsi_refresh_limits(), since we already short circuit so
many other things in sg mode.  But this patch takes a slightly
more conservative approach, and merely guarantees that scaling
will succeed (for SG devices, the scaling is done to the block
layer default of 512 bytes, since we don't know of any iscsi
devices with a smaller block size), while still using multiples
of the original size.  Resulting limits may still be zero in SG
mode (that is, we only fix block_size used as a denominator, not
all uses).

Reported-by: Holger Schranz <holger@fam-schranz.de>
Signed-off-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
---

I would really appreciate Holger testing this patch. We could
also go with the much shorter patch that just does
if (bs->sg) { return; }
at the top of iscsi_refresh_limits(), but I'm not sure if that
would break anything else in the block layer (we had several,
but not all, limits that were provably left alone at 0 for
sg mode).

 block/iscsi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 95ce9e1..ff84cd1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1813,6 +1813,10 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)

     IscsiLun *iscsilun = bs->opaque;
     uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
+    unsigned int block_size = MIN_NON_ZERO(BDRV_SECTOR_SIZE,
+                                           iscsilun->block_size);
+
+    assert(iscsilun->block_size >= BDRV_SECTOR_SIZE || bs->sg);

     bs->bl.request_alignment = iscsilun->block_size;

@@ -1820,12 +1824,12 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
         max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
     }

-    if (max_xfer_len * iscsilun->block_size < INT_MAX) {
+    if (max_xfer_len * block_size < INT_MAX) {
         bs->bl.max_transfer = max_xfer_len * iscsilun->block_size;
     }

     if (iscsilun->lbp.lbpu) {
-        if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) {
+        if (iscsilun->bl.max_unmap < 0xffffffff / block_size) {
             bs->bl.max_pdiscard =
                 iscsilun->bl.max_unmap * iscsilun->block_size;
         }
@@ -1835,7 +1839,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.pdiscard_alignment = iscsilun->block_size;
     }

-    if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) {
+    if (iscsilun->bl.max_ws_len < 0xffffffff / block_size) {
         bs->bl.max_pwrite_zeroes =
             iscsilun->bl.max_ws_len * iscsilun->block_size;
     }
@@ -1846,7 +1850,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
     }
     if (iscsilun->bl.opt_xfer_len &&
-        iscsilun->bl.opt_xfer_len < INT_MAX / iscsilun->block_size) {
+        iscsilun->bl.opt_xfer_len < INT_MAX / block_size) {
         bs->bl.opt_transfer = pow2floor(iscsilun->bl.opt_xfer_len *
                                         iscsilun->block_size);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Holger Schranz]

Hi Eric,

I will test this patch within tomorrow. Is this o.k. for you?

Beste regards

Holger

Am 06.09.2016 um 21:04 schrieb Eric Blake:
> When qemu uses iscsi devices in sg mode, iscsilun->block_size
> is left at 0.  Prior to commits cf081fca and similar, when
> block limits were tracked in sectors, this did not matter:
> various block limits were just left at 0.  But when we started
> scaling by block size, this caused SIGFPE.
>
> One possible solution for SG mode is to just blindly skip ALL
> of iscsi_refresh_limits(), since we already short circuit so
> many other things in sg mode.  But this patch takes a slightly
> more conservative approach, and merely guarantees that scaling
> will succeed (for SG devices, the scaling is done to the block
> layer default of 512 bytes, since we don't know of any iscsi
> devices with a smaller block size), while still using multiples
> of the original size.  Resulting limits may still be zero in SG
> mode (that is, we only fix block_size used as a denominator, not
> all uses).
>
> Reported-by: Holger Schranz <holger@fam-schranz.de>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> CC: qemu-stable@nongnu.org
> ---
>
> I would really appreciate Holger testing this patch. We could
> also go with the much shorter patch that just does
> if (bs->sg) { return; }
> at the top of iscsi_refresh_limits(), but I'm not sure if that
> would break anything else in the block layer (we had several,
> but not all, limits that were provably left alone at 0 for
> sg mode).
>
>   block/iscsi.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 95ce9e1..ff84cd1 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1813,6 +1813,10 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>
>       IscsiLun *iscsilun = bs->opaque;
>       uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
> +    unsigned int block_size = MIN_NON_ZERO(BDRV_SECTOR_SIZE,
> +                                           iscsilun->block_size);
> +
> +    assert(iscsilun->block_size >= BDRV_SECTOR_SIZE || bs->sg);
>
>       bs->bl.request_alignment = iscsilun->block_size;
>
> @@ -1820,12 +1824,12 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>           max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
>       }
>
> -    if (max_xfer_len * iscsilun->block_size < INT_MAX) {
> +    if (max_xfer_len * block_size < INT_MAX) {
>           bs->bl.max_transfer = max_xfer_len * iscsilun->block_size;
>       }
>
>       if (iscsilun->lbp.lbpu) {
> -        if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) {
> +        if (iscsilun->bl.max_unmap < 0xffffffff / block_size) {
>               bs->bl.max_pdiscard =
>                   iscsilun->bl.max_unmap * iscsilun->block_size;
>           }
> @@ -1835,7 +1839,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>           bs->bl.pdiscard_alignment = iscsilun->block_size;
>       }
>
> -    if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) {
> +    if (iscsilun->bl.max_ws_len < 0xffffffff / block_size) {
>           bs->bl.max_pwrite_zeroes =
>               iscsilun->bl.max_ws_len * iscsilun->block_size;
>       }
> @@ -1846,7 +1850,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>           bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
>       }
>       if (iscsilun->bl.opt_xfer_len &&
> -        iscsilun->bl.opt_xfer_len < INT_MAX / iscsilun->block_size) {
> +        iscsilun->bl.opt_xfer_len < INT_MAX / block_size) {
>           bs->bl.opt_transfer = pow2floor(iscsilun->bl.opt_xfer_len *
>                                           iscsilun->block_size);
>       }


---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Re: [Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Kevin Wolf]

Am 06.09.2016 um 21:04 hat Eric Blake geschrieben:
> When qemu uses iscsi devices in sg mode, iscsilun->block_size
> is left at 0.  Prior to commits cf081fca and similar, when
> block limits were tracked in sectors, this did not matter:
> various block limits were just left at 0.  But when we started
> scaling by block size, this caused SIGFPE.
> 
> One possible solution for SG mode is to just blindly skip ALL
> of iscsi_refresh_limits(), since we already short circuit so
> many other things in sg mode.  But this patch takes a slightly
> more conservative approach, and merely guarantees that scaling
> will succeed (for SG devices, the scaling is done to the block
> layer default of 512 bytes, since we don't know of any iscsi
> devices with a smaller block size), while still using multiples
> of the original size.  Resulting limits may still be zero in SG
> mode (that is, we only fix block_size used as a denominator, not
> all uses).
> 
> Reported-by: Holger Schranz <holger@fam-schranz.de>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> CC: qemu-stable@nongnu.org
> ---
> 
> I would really appreciate Holger testing this patch. We could
> also go with the much shorter patch that just does
> if (bs->sg) { return; }
> at the top of iscsi_refresh_limits(), but I'm not sure if that
> would break anything else in the block layer (we had several,
> but not all, limits that were provably left alone at 0 for
> sg mode).

Hm, originally I thought that we could even just return for bs->sg in
bdrv_refresh_limits() like below because for SG devices the limits
should never be used, but with scsi-block they might actually be.

Paolo, what do you think?

Anyway, let's go with the above patch first, it's a conservative one
that qemu-stable and possibly downstream can safely backport. If we're
going to add anything else, that would be for 2.8 only.

Kevin

diff --git a/block/io.c b/block/io.c
index fdf7080..144ff65 100644
--- a/block/io.c
+++ b/block/io.c
@@ -84,7 +84,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
 
     memset(&bs->bl, 0, sizeof(bs->bl));
 
-    if (!drv) {
+    if (!drv || bs->sg) {
         return;
     }

Re: [Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Holger Schranz]

Hello Eric, Kevin,

unfortunately it was not possible for me to get the git tree at this
time. I have insert the changes by hand, which was into this mail below
in block/iscsi.c in the distribution from qemu.org for 2.7 (was this o.k.)?

After installing and start with virsh create, the following problem occur:

internal error: process exited while connecting to monitor:
qemu-system-x86_64: block.c:1022:
bdrv_open_common: Assertion `is_power_of_2(bs->bl.request_alignment)'
failed.

We will send you the backtrace later on.

Best regards

Holger

Am 07.09.2016 um 10:31 schrieb Kevin Wolf:
> Am 06.09.2016 um 21:04 hat Eric Blake geschrieben:
>> When qemu uses iscsi devices in sg mode, iscsilun->block_size
>> is left at 0.  Prior to commits cf081fca and similar, when
>> block limits were tracked in sectors, this did not matter:
>> various block limits were just left at 0.  But when we started
>> scaling by block size, this caused SIGFPE.
>>
>> One possible solution for SG mode is to just blindly skip ALL
>> of iscsi_refresh_limits(), since we already short circuit so
>> many other things in sg mode.  But this patch takes a slightly
>> more conservative approach, and merely guarantees that scaling
>> will succeed (for SG devices, the scaling is done to the block
>> layer default of 512 bytes, since we don't know of any iscsi
>> devices with a smaller block size), while still using multiples
>> of the original size.  Resulting limits may still be zero in SG
>> mode (that is, we only fix block_size used as a denominator, not
>> all uses).
>>
>> Reported-by: Holger Schranz <holger@fam-schranz.de>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> CC: qemu-stable@nongnu.org
>> ---
>>
>> I would really appreciate Holger testing this patch. We could
>> also go with the much shorter patch that just does
>> if (bs->sg) { return; }
>> at the top of iscsi_refresh_limits(), but I'm not sure if that
>> would break anything else in the block layer (we had several,
>> but not all, limits that were provably left alone at 0 for
>> sg mode).
> Hm, originally I thought that we could even just return for bs->sg in
> bdrv_refresh_limits() like below because for SG devices the limits
> should never be used, but with scsi-block they might actually be.
>
> Paolo, what do you think?
>
> Anyway, let's go with the above patch first, it's a conservative one
> that qemu-stable and possibly downstream can safely backport. If we're
> going to add anything else, that would be for 2.8 only.
>
> Kevin
>
> diff --git a/block/io.c b/block/io.c
> index fdf7080..144ff65 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -84,7 +84,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>
>       memset(&bs->bl, 0, sizeof(bs->bl));
>
> -    if (!drv) {
> +    if (!drv || bs->sg) {
>           return;
>       }
>


---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Re: [Qemu-devel] [PATCH] iscsi: Fix divide-by-zero regression on raw SG devices

[Eric Blake]

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

On 09/07/2016 06:48 AM, Holger Schranz wrote:
> Hello Eric, Kevin,
> 
> unfortunately it was not possible for me to get the git tree at this
> time. I have insert the changes by hand, which was into this mail below
> in block/iscsi.c in the distribution from qemu.org for 2.7 (was this o.k.)?

Probably good enough, as it at least got you further (doesn't feel quite
as clean as testing git directly, but since the patch should be
backportable as-is to the 2.7 code, it matches what downstream distros
will be doing).

> 
> After installing and start with virsh create, the following problem occur:
> 
> internal error: process exited while connecting to monitor:
> qemu-system-x86_64: block.c:1022:
> bdrv_open_common: Assertion `is_power_of_2(bs->bl.request_alignment)'
> failed.
> 
> We will send you the backtrace later on.

Ah, I already know where the backtrace would point to. The patch is
setting bl.request_alignment to 0, which is not a power of two, so the
assertion in block.c should also exempt SG devices from reporting a
preferred request_alignment. v2 coming up.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]