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 <mlevi...@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
