Re: [Qemu-stable] [Qemu-devel] [BUGFIX][PATCH for 2.2 1/1] hw/ide/core.c: Prevent SIGSEGV during migration

[Markus Armbruster]

Don Slutz <address@hidden> writes:

> The other callers to blk_set_enable_write_cache() in this file
> already check for s->blk == NULL.
>
> Signed-off-by: Don Slutz <address@hidden>
> ---
>
> I think this is a bugfix that should be back ported to stable
> releases.
>
> I also think this should be done in xen's copy of QEMU for 4.5 with
> back port(s) to active stable releases.
>
> Note: In 2.1 and earlier the routine is
> bdrv_set_enable_write_cache(); variable is s->bs.

Got a reproducer?

I'm asking because I believe s->identify_set implies s->blk.
s->identify_set is initialized to zero, and gets set to non-zero exactly
on the first successful IDENTIFY DEVICE or IDENTIFY PACKET DEVICE, in
ide_identify(), ide_atapi_identify() or ide_cfata_identify(),
respectively.  Only called via cmd_identify() / cmd_identify_packet()
via ide_exec_cmd().  The latter immediately fails when !s->blk:

    s = idebus_active_if(bus);
    /* ignore commands to non existent slave */
    if (s != bus->ifs && !s->blk) {
        return;
    }

Even if I'm right, your patch is fine, because it makes this spot more
obviously correct, and consistent with the other uses of
blk_set_enable_write_cache().  The case for stable is weak, though.

>
>  hw/ide/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 00e21cf..d4af5e2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2401,7 +2401,7 @@ static int ide_drive_post_load(void *opaque, int 
> version_id)
>  {
>      IDEState *s = opaque;
>  
> -    if (s->identify_set) {
> +    if (s->blk && s->identify_set) {
>          blk_set_enable_write_cache(s->blk, !!(s->identify_data[85] & (1 << 
> 5)));
>      }
>      return 0;