[Qemu-devel] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Qemu-devel] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
from exceeding these limits, by capping the maximum transfer length to
that value in the block limits VPD page.

[1] Oh well, at least it was easier to follow the kernel source knowing
    it had to end as ENOMEM...

Cc: qemu-stable@nongnu.org
Cc: qemu-block@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-generic.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index d82b462be4..4134d8389b 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -131,6 +131,17 @@ static int execute_command(BlockBackend *blk,
     return 0;
 }
 
+/*
+ * Linux places a hard limit on SG_IO transfers equal to UIO_MAXIOV
+ * pages, which we need to factor in the block limits we return.
+ */
+static uint32_t sg_max_transfer(SCSIDevice *s)
+{
+    uint32_t max_transfer = blk_get_max_transfer(s->conf.blk);
+    max_transfer = MIN_NON_ZERO(max_transfer, UIO_MAXIOV * qemu_real_host_page_size);
+    return max_transfer / s->blocksize;
+}
+
 static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
 {
     uint8_t page, page_idx;
@@ -162,10 +173,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
     if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) {
         page = r->req.cmd.buf[2];
         if (page == 0xb0) {
-            uint32_t max_transfer =
-                blk_get_max_transfer(s->conf.blk) / s->blocksize;
+            uint32_t max_transfer = sg_max_transfer(s);
 
-            assert(max_transfer);
             stl_be_p(&r->buf[8], max_transfer);
             /* Also take care of the opt xfer len. */
             stl_be_p(&r->buf[12],
@@ -206,7 +215,7 @@ static int scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice *s)
     uint8_t buf[64];
 
     SCSIBlockLimits bl = {
-        .max_io_sectors = blk_get_max_transfer(s->conf.blk) / s->blocksize
+        .max_io_sectors = sg_max_transfer(s)
     };
 
     memset(r->buf, 0, r->buflen);
-- 
2.21.0

[Qemu-devel] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
from exceeding these limits, by capping the maximum transfer length to
that value in the block limits VPD page.

[1] Oh well, at least it was easier to follow the kernel source knowing
    it had to end as ENOMEM...

Cc: qemu-stable@nongnu.org
Cc: qemu-block@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-generic.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index d82b462be4..4134d8389b 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -131,6 +131,17 @@ static int execute_command(BlockBackend *blk,
     return 0;
 }
 
+/*
+ * Linux places a hard limit on SG_IO transfers equal to UIO_MAXIOV
+ * pages, which we need to factor in the block limits we return.
+ */
+static uint32_t sg_max_transfer(SCSIDevice *s)
+{
+    uint32_t max_transfer = blk_get_max_transfer(s->conf.blk);
+    max_transfer = MIN_NON_ZERO(max_transfer, UIO_MAXIOV * qemu_real_host_page_size);
+    return max_transfer / s->blocksize;
+}
+
 static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
 {
     uint8_t page, page_idx;
@@ -162,10 +173,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
     if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) {
         page = r->req.cmd.buf[2];
         if (page == 0xb0) {
-            uint32_t max_transfer =
-                blk_get_max_transfer(s->conf.blk) / s->blocksize;
+            uint32_t max_transfer = sg_max_transfer(s);
 
-            assert(max_transfer);
             stl_be_p(&r->buf[8], max_transfer);
             /* Also take care of the opt xfer len. */
             stl_be_p(&r->buf[12],
@@ -206,7 +215,7 @@ static int scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice *s)
     uint8_t buf[64];
 
     SCSIBlockLimits bl = {
-        .max_io_sectors = blk_get_max_transfer(s->conf.blk) / s->blocksize
+        .max_io_sectors = sg_max_transfer(s)
     };
 
     memset(r->buf, 0, r->buflen);
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Stefan Hajnoczi]

On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> from exceeding these limits, by capping the maximum transfer length to
> that value in the block limits VPD page.
>
> [1] Oh well, at least it was easier to follow the kernel source knowing
>     it had to end as ENOMEM...
>
> Cc: qemu-stable@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Stefan Hajnoczi]

On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> from exceeding these limits, by capping the maximum transfer length to
> that value in the block limits VPD page.
>
> [1] Oh well, at least it was easier to follow the kernel source knowing
>     it had to end as ENOMEM...
>
> Cc: qemu-stable@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

On 17/04/19 14:30, Stefan Hajnoczi wrote:
> On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
>> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
>> from exceeding these limits, by capping the maximum transfer length to
>> that value in the block limits VPD page.
>>
>> [1] Oh well, at least it was easier to follow the kernel source knowing
>>     it had to end as ENOMEM...
>>
>> Cc: qemu-stable@nongnu.org
>> Cc: qemu-block@nongnu.org
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

This is acually only an issue with kernels prior to 4.5, so it should be
fixed downstream instead.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

On 17/04/19 14:30, Stefan Hajnoczi wrote:
> On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
>> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
>> from exceeding these limits, by capping the maximum transfer length to
>> that value in the block limits VPD page.
>>
>> [1] Oh well, at least it was easier to follow the kernel source knowing
>>     it had to end as ENOMEM...
>>
>> Cc: qemu-stable@nongnu.org
>> Cc: qemu-block@nongnu.org
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

This is acually only an issue with kernels prior to 4.5, so it should be
fixed downstream instead.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Kevin Wolf]

Am 18.04.2019 um 11:48 hat Paolo Bonzini geschrieben:
> On 17/04/19 14:30, Stefan Hajnoczi wrote:
> > On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> >> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> >> from exceeding these limits, by capping the maximum transfer length to
> >> that value in the block limits VPD page.
> >>
> >> [1] Oh well, at least it was easier to follow the kernel source knowing
> >>     it had to end as ENOMEM...
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Cc: qemu-block@nongnu.org
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> 
> This is acually only an issue with kernels prior to 4.5, so it should be
> fixed downstream instead.

I don't think that upstream QEMU has kernels > 4.5 as an official
requirement, though? We try to be compatible with quite old libs, so we
should stay compatible with old kernels, too.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Kevin Wolf]

Am 18.04.2019 um 11:48 hat Paolo Bonzini geschrieben:
> On 17/04/19 14:30, Stefan Hajnoczi wrote:
> > On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> >> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> >> from exceeding these limits, by capping the maximum transfer length to
> >> that value in the block limits VPD page.
> >>
> >> [1] Oh well, at least it was easier to follow the kernel source knowing
> >>     it had to end as ENOMEM...
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Cc: qemu-block@nongnu.org
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> 
> This is acually only an issue with kernels prior to 4.5, so it should be
> fixed downstream instead.

I don't think that upstream QEMU has kernels > 4.5 as an official
requirement, though? We try to be compatible with quite old libs, so we
should stay compatible with old kernels, too.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

On 18/04/19 12:47, Kevin Wolf wrote:
>> This is acually only an issue with kernels prior to 4.5, so it should be
>> fixed downstream instead.
>
> I don't think that upstream QEMU has kernels > 4.5 as an official
> requirement, though? We try to be compatible with quite old libs, so we
> should stay compatible with old kernels, too.

It is a pretty rare case, where the host must support very large
scatter/gather lists (>=1024 elements) and there is a workaround by
setting max_sectors_kb to 4096 in the host.  It also makes performance
worse.

Having already written and posted the patch, I can of course apply it
too, but I'm not sure it's a good idea.  4.4 is already a pretty old
kernel and whoever is using it likely will not get the fix for free by
using a very new QEMU.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Paolo Bonzini]

On 18/04/19 12:47, Kevin Wolf wrote:
>> This is acually only an issue with kernels prior to 4.5, so it should be
>> fixed downstream instead.
>
> I don't think that upstream QEMU has kernels > 4.5 as an official
> requirement, though? We try to be compatible with quite old libs, so we
> should stay compatible with old kernels, too.

It is a pretty rare case, where the host must support very large
scatter/gather lists (>=1024 elements) and there is a workaround by
setting max_sectors_kb to 4096 in the host.  It also makes performance
worse.

Having already written and posted the patch, I can of course apply it
too, but I'm not sure it's a good idea.  4.4 is already a pretty old
kernel and whoever is using it likely will not get the fix for free by
using a very new QEMU.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Kevin Wolf]

Am 18.04.2019 um 13:01 hat Paolo Bonzini geschrieben:
> On 18/04/19 12:47, Kevin Wolf wrote:
> >> This is acually only an issue with kernels prior to 4.5, so it should be
> >> fixed downstream instead.
> >
> > I don't think that upstream QEMU has kernels > 4.5 as an official
> > requirement, though? We try to be compatible with quite old libs, so we
> > should stay compatible with old kernels, too.
> 
> It is a pretty rare case, where the host must support very large
> scatter/gather lists (>=1024 elements) and there is a workaround by
> setting max_sectors_kb to 4096 in the host.  It also makes performance
> worse.

Fair enough. I didn't consider that it has a performance impact even on
newer kernels that don't need the patch.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Kevin Wolf]

Am 18.04.2019 um 13:01 hat Paolo Bonzini geschrieben:
> On 18/04/19 12:47, Kevin Wolf wrote:
> >> This is acually only an issue with kernels prior to 4.5, so it should be
> >> fixed downstream instead.
> >
> > I don't think that upstream QEMU has kernels > 4.5 as an official
> > requirement, though? We try to be compatible with quite old libs, so we
> > should stay compatible with old kernels, too.
> 
> It is a pretty rare case, where the host must support very large
> scatter/gather lists (>=1024 elements) and there is a workaround by
> setting max_sectors_kb to 4096 in the host.  It also makes performance
> worse.

Fair enough. I didn't consider that it has a performance impact even on
newer kernels that don't need the patch.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Daniel P. Berrangé]

On Thu, Apr 18, 2019 at 12:47:37PM +0200, Kevin Wolf wrote:
> Am 18.04.2019 um 11:48 hat Paolo Bonzini geschrieben:
> > On 17/04/19 14:30, Stefan Hajnoczi wrote:
> > > On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> > >> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> > >> from exceeding these limits, by capping the maximum transfer length to
> > >> that value in the block limits VPD page.
> > >>
> > >> [1] Oh well, at least it was easier to follow the kernel source knowing
> > >>     it had to end as ENOMEM...
> > >>
> > >> Cc: qemu-stable@nongnu.org
> > >> Cc: qemu-block@nongnu.org
> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> ---
> > >>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
> > >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > 
> > 
> > This is acually only an issue with kernels prior to 4.5, so it should be
> > fixed downstream instead.
> 
> I don't think that upstream QEMU has kernels > 4.5 as an official
> requirement, though? We try to be compatible with quite old libs, so we
> should stay compatible with old kernels, too.

We've not explicitly stated or checked a min kernel version, but our
min kernel is defined implicitly by our targetted build platforms:

  https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

The oldest Linux distro under those rules is RHEL-7 (3.10.0), since
Debian Jessie actually updates its kernel version so is already on
version 4.5


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH] scsi-generic: prevent guest from exceeding SG_IO limits

[Daniel P. Berrangé]

On Thu, Apr 18, 2019 at 12:47:37PM +0200, Kevin Wolf wrote:
> Am 18.04.2019 um 11:48 hat Paolo Bonzini geschrieben:
> > On 17/04/19 14:30, Stefan Hajnoczi wrote:
> > > On Wed, Apr 17, 2019 at 12:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> Linux places a limit of UIO_MAXIOV pages on SG_IO ioctls (and if the limit
> > >> is exceeded, a confusing ENOMEM error is returned[1]).  Prevent the guest
> > >> from exceeding these limits, by capping the maximum transfer length to
> > >> that value in the block limits VPD page.
> > >>
> > >> [1] Oh well, at least it was easier to follow the kernel source knowing
> > >>     it had to end as ENOMEM...
> > >>
> > >> Cc: qemu-stable@nongnu.org
> > >> Cc: qemu-block@nongnu.org
> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> ---
> > >>  hw/scsi/scsi-generic.c | 17 +++++++++++++----
> > >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > 
> > 
> > This is acually only an issue with kernels prior to 4.5, so it should be
> > fixed downstream instead.
> 
> I don't think that upstream QEMU has kernels > 4.5 as an official
> requirement, though? We try to be compatible with quite old libs, so we
> should stay compatible with old kernels, too.

We've not explicitly stated or checked a min kernel version, but our
min kernel is defined implicitly by our targetted build platforms:

  https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

The oldest Linux distro under those rules is RHEL-7 (3.10.0), since
Debian Jessie actually updates its kernel version so is already on
version 4.5


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|