From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAey-0001ze-KW for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:52:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTAev-0005qO-Dt for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:52:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAev-0005qG-64 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:52:25 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24EqNNn010136 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:52:24 -0500 Message-ID: <54F71C25.1060605@redhat.com> Date: Wed, 04 Mar 2015 09:52:21 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423501897-30410-1-git-send-email-mreitz@redhat.com> <1423501897-30410-5-git-send-email-mreitz@redhat.com> <20150304140242.GQ3465@noname.str.redhat.com> <54F711AB.1060703@redhat.com> <20150304142037.GT3465@noname.str.redhat.com> <54F71590.3030502@redhat.com> <20150304143920.GU3465@noname.str.redhat.com> <54F719B3.9020203@redhat.com> In-Reply-To: <54F719B3.9020203@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 04/37] hw/usb-storage: Check whether BB is inserted List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: John Snow , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster On 2015-03-04 at 09:41, Max Reitz wrote: > On 2015-03-04 at 09:39, Kevin Wolf wrote: >> Am 04.03.2015 um 15:24 hat Max Reitz geschrieben: >>> On 2015-03-04 at 09:20, Kevin Wolf wrote: >>>> Am 04.03.2015 um 15:07 hat Max Reitz geschrieben: >>>>> On 2015-03-04 at 09:02, Kevin Wolf wrote: >>>>>> Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >>>>>>> Only call bdrv_key_required() on the BlockDriverState if the >>>>>>> BlockBackend has an inserted medium. >>>>>>> >>>>>>> Signed-off-by: Max Reitz >>>>>>> Reviewed-by: Eric Blake >>>>>>> --- >>>>>>> hw/usb/dev-storage.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c >>>>>>> index 4539733..3123baf 100644 >>>>>>> --- a/hw/usb/dev-storage.c >>>>>>> +++ b/hw/usb/dev-storage.c >>>>>>> @@ -638,7 +638,7 @@ static void >>>>>>> usb_msd_realize_storage(USBDevice *dev, Error **errp) >>>>>>> usb_msd_handle_reset(dev); >>>>>>> s->scsi_dev = scsi_dev; >>>>>>> - if (bdrv_key_required(blk_bs(blk))) { >>>>>>> + if (blk_is_inserted(blk) && bdrv_key_required(blk_bs(blk))) { >>>>>>> if (cur_mon) { >>>>>>> monitor_read_bdrv_key_start(cur_mon, blk_bs(blk), >>>>>>> usb_msd_password_cb, s); >>>>>> Why would bdrv_key_required() ever return true when no medium is >>>>>> inserted? Sounds like a bug to me, like not resetting state >>>>>> correctly on >>>>>> bdrv_close() of an encrypted image. >>>>> The point is that blk_bs(blk) might be NULL. >>>> This is not what blk_is_inserted() is checking. It happens to protect >>>> you against segfaults because it's robust against using NULL, but with >>>> an existing BDS, checking whether there is a medium inserted (in the >>>> physical device for passthrough drivers) doesn't make sense. >>> Not right now it's not. See patch 6. >> Patch 6 looks unrelated, at least in v2. But if you're trying to say >> that I looked at the wrong version, you're right: It doesn't protect you >> against segfaults at this point yet (which is okay, because blk->bs >> can't be NULL yet), it only performs the misguided inserted check. > > Oops, yes, I meant patch 7. > >> Doesn't answer my initial question or make that check any better. > > The answer to your initial question is: bdrv_key_required() assumes a > non-NULL BDS pointer is passed (which is reasonable). Therefore, it > crashes when "no medium is inserted" in the sense of !blk_bs(blk). > > How it makes the check better: I'll move it after patch 7. And after realizing that patch 8 breaks this (because you might have a non-inserted host CD-ROM backing an encrypted qcow2 file), I'll use blk_bs(blk) instead of blk_is_inserted(blk). Max