linux-kernel - Re: [PATCH] add virtio disk geometry feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4806706E.10701@us.ibm.com>
Date:	Wed, 16 Apr 2008 16:32:30 -0500
From:	Anthony Liguori <aliguori@...ibm.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
CC:	Ryan Harper <ryanh@...ibm.com>,
	virtualization@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org, kvm-devel@...ts.sourceforge.net,
	Hollis Blanchard <hollisb@...ibm.com>
Subject: Re: [PATCH] add virtio disk geometry feature

Rusty Russell wrote:
> On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
>   
>> From: Ryan Harper <ryanh@...ibm.com>
>>
>> Rather than faking up some geometry, allow the backend to push the disk
>> geometry via virtio pci config option.  Keep the old geo code around for
>> compatibility.
>>     
>
> Hi Ryan,
>
>    Looks good! Some brief review below.  Mainly just "how I would have done
> things" stuff.  BTW, does this help in real life?  I assume something in
> userspace wants it?
>   

Boot loaders (like grub) query the geometry from the kernel to figure 
out how to setup the stage1/stage2.  We've seen strange issues with grub 
thinking it has crazy geometries when installed on a virtio disk (as 
opposed to booting from virtio with an existing disk).

Ryan: have you tested a hardy install with your patches?  Does it help 
when installing to virtio?  I could pretty reliably reproduce the 
strangeness with a 20GB disk image FWIW.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
>  /* We provide getgeo only to please some old bootloader/partitioning tools */
>  static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
>  {
> -	/* some standard values, similar to sd */
> -	geo->heads = 1 << 6;
> -	geo->sectors = 1 << 5;
> -	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	struct virtio_blk *vblk = bd->bd_disk->private_data;
> +	struct virtio_blk_geometry vgeo;
> +	int err;
> +
> +	/* see if the host passed in geometry config */
> +	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> +				offsetof(struct virtio_blk_config, geometry),
> +				&vgeo);
> +
> +	if (!err) {
> +		geo->heads = vgeo.heads;
> +		geo->sectors = vgeo.sectors;
> +		geo->cylinders = vgeo.cylinders;
> +	} else {
> +		/* some standard values, similar to sd */
> +		geo->heads = 1 << 6;
> +		geo->sectors = 1 << 5;
> +		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	}
>  	return 0;
>  }
>   

You're probably breaking PPC since the values in the config space are in 
little endian format.  virtio_config_val does automagic endianness 
conversion if the size is 2, 4, or 8.  In this case, the structure size 
is 4 so the endianness conversion will do the wrong thing.

Magic endianness conversion based on read size is looking pretty evil to 
me... Perhaps we need explicit *_val[8,16,32,64]?

Regards,

Anthony Liguori

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/