[PATCH] qemu-pr-helper: fix crash in mpath_reconstruct_sense

[PATCH] qemu-pr-helper: fix crash in mpath_reconstruct_sense

[Maxim Levitsky]

The 'r' variable was accidently shadowed, and because of this
we were always passing 0 to mpath_generic_sense, instead of original
return value, which triggers an abort()

This is an attempt to fix the
https://bugzilla.redhat.com/show_bug.cgi?id=1720047
although there might be other places in the code
that trigger qemu-pr-helper crash, and this fix might
not be the root cause.

The crash was reproduced by creating an iscsi target on a test machine,
and passing it twice to the guest like that:

-blockdev node-name=idisk0,driver=iscsi,transport=...,target=...
-device scsi-block,drive=idisk0,bus=scsi0.0,bootindex=-1,scsi-id=1,lun=0,share-rw=on
-device scsi-block,drive=idisk0,bus=scsi0.0,bootindex=-1,scsi-id=1,lun=1,share-rw=on

Then in the guest, both /dev/sda and /dev/sdb were aggregated by multipath to /dev/mpatha,
which was passed to a nested guest like that

-object pr-manager-helper,id=qemu_pr_helper,path=/root/work/vm/testvm/.run/pr_helper.socket
-blockdev node-name=test,driver=host_device,filename=/dev/mapper/mpatha,pr-manager=qemu_pr_helper
-device scsi-block,drive=test,bus=scsi0.0,bootindex=-1,scsi-id=0,lun=0

The nested guest run:

sg_persist --no-inquiry  -v --out --register --param-sark 0x1234 /dev/sda

Strictly speaking this is wrong configuration since qemu is where
the multipath was split, and thus the iscsi target was not aware of
multipath, and thus when libmpathpersist code rightfully tried to register
the PR key on all paths, it failed to do so.

However qemu-pr-helper should not crash in this case.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 scsi/qemu-pr-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index a8a74d1dba..debb18f4aa 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -323,10 +323,10 @@ static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
              */
             uint8_t cdb[6] = { TEST_UNIT_READY };
             int sz = 0;
-            int r = do_sgio(fd, cdb, sense, NULL, &sz, SG_DXFER_NONE);
+            int ret = do_sgio(fd, cdb, sense, NULL, &sz, SG_DXFER_NONE);
 
-            if (r != GOOD) {
-                return r;
+            if (ret != GOOD) {
+                return ret;
             }
             scsi_build_sense(sense, mpath_generic_sense(r));
             return CHECK_CONDITION;
-- 
2.17.2

Re: [PATCH] qemu-pr-helper: fix crash in mpath_reconstruct_sense

[Paolo Bonzini]

On 24/09/19 01:38, Maxim Levitsky wrote:
> The 'r' variable was accidently shadowed, and because of this
> we were always passing 0 to mpath_generic_sense, instead of original
> return value, which triggers an abort()
> 
> This is an attempt to fix the
> https://bugzilla.redhat.com/show_bug.cgi?id=1720047
> although there might be other places in the code
> that trigger qemu-pr-helper crash, and this fix might
> not be the root cause.
> 
> The crash was reproduced by creating an iscsi target on a test machine,
> and passing it twice to the guest like that:
> 
> -blockdev node-name=idisk0,driver=iscsi,transport=...,target=...
> -device scsi-block,drive=idisk0,bus=scsi0.0,bootindex=-1,scsi-id=1,lun=0,share-rw=on
> -device scsi-block,drive=idisk0,bus=scsi0.0,bootindex=-1,scsi-id=1,lun=1,share-rw=on
> 
> Then in the guest, both /dev/sda and /dev/sdb were aggregated by multipath to /dev/mpatha,
> which was passed to a nested guest like that
> 
> -object pr-manager-helper,id=qemu_pr_helper,path=/root/work/vm/testvm/.run/pr_helper.socket
> -blockdev node-name=test,driver=host_device,filename=/dev/mapper/mpatha,pr-manager=qemu_pr_helper
> -device scsi-block,drive=test,bus=scsi0.0,bootindex=-1,scsi-id=0,lun=0
> 
> The nested guest run:
> 
> sg_persist --no-inquiry  -v --out --register --param-sark 0x1234 /dev/sda
> 
> Strictly speaking this is wrong configuration since qemu is where
> the multipath was split, and thus the iscsi target was not aware of
> multipath, and thus when libmpathpersist code rightfully tried to register
> the PR key on all paths, it failed to do so.
> 
> However qemu-pr-helper should not crash in this case.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  scsi/qemu-pr-helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index a8a74d1dba..debb18f4aa 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -323,10 +323,10 @@ static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
>               */
>              uint8_t cdb[6] = { TEST_UNIT_READY };
>              int sz = 0;
> -            int r = do_sgio(fd, cdb, sense, NULL, &sz, SG_DXFER_NONE);
> +            int ret = do_sgio(fd, cdb, sense, NULL, &sz, SG_DXFER_NONE);
>  
> -            if (r != GOOD) {
> -                return r;
> +            if (ret != GOOD) {
> +                return ret;
>              }
>              scsi_build_sense(sense, mpath_generic_sense(r));
>              return CHECK_CONDITION;
> 

Nice catch, thanks!  Patch queued.

Paolo