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 <address@hidden>
> ---
>  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