From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAUv-0003ym-Ar for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:42:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTAUq-0001jO-KG for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:42:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37900) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAUp-0001iy-EE for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:42:00 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24EfwwI004271 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:41:58 -0500 Message-ID: <54F719B3.9020203@redhat.com> Date: Wed, 04 Mar 2015 09:41:55 -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> In-Reply-To: <20150304143920.GU3465@noname.str.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: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. Max