| Message ID | 4BAAF5CA.2010401@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 25 Mar 2010 04:04:02 pm john cooper wrote: > Return serial string to the guest application via > ioctl driver call. This is quite nice. Minor nits: > + if (cmd == 'VBID') { > + void *usr_data = (void __user *)data; void __user *usr_data; > + char *id_str; > + int err; > + > + if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL))) > + err = -ENOMEM; > + else if ((err = virtblk_get_id(disk, id_str))) > + ; > + else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES)) > + err = -EFAULT; > + if (id_str) > + kfree(id_str); > + return err; > + } We can't put the id_str on the stack? Makes it even simpler :) Cheers, Rusty.
Rusty Russell wrote: > On Thu, 25 Mar 2010 04:04:02 pm john cooper wrote: >> Return serial string to the guest application via >> ioctl driver call. > > This is quite nice. Minor nits: > >> + if (cmd == 'VBID') { >> + void *usr_data = (void __user *)data; > > void __user *usr_data; > >> + char *id_str; >> + int err; >> + >> + if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL))) >> + err = -ENOMEM; >> + else if ((err = virtblk_get_id(disk, id_str))) >> + ; >> + else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES)) >> + err = -EFAULT; >> + if (id_str) >> + kfree(id_str); >> + return err; >> + } > > We can't put the id_str on the stack? Makes it even simpler :) At a VIRTIO_BLK_ID_BYTES of the current 20 bytes it seems safe but there was discussion of extending it so I thought to locate it in safer storage. Note the above was intended as more of an example to illustrate the mechanism. Marc had proposed a /sys style interface to retrieve a virtio id string which is what motivated revisiting this issue. Marc, if you don't foresee tying off that work relatively soon where a /sys interface would be made available, I'll rework the above to be a little more general. The first version of the S/N patch pulled a user buffer size from the caller and limited the copy out to that length. -john
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index cd66806..0954193 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -225,6 +225,21 @@ static int virtblk_ioctl(struct block_device *bdev, fmode_t mode, struct gendisk *disk = bdev->bd_disk; struct virtio_blk *vblk = disk->private_data; + if (cmd == 'VBID') { + void *usr_data = (void __user *)data; + char *id_str; + int err; + + if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL))) + err = -ENOMEM; + else if ((err = virtblk_get_id(disk, id_str))) + ; + else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES)) + err = -EFAULT; + if (id_str) + kfree(id_str); + return err; + } /* * Only allow the generic SCSI ioctls if the host can support it. */
Return serial string to the guest application via ioctl driver call. Note this form of interface to the guest userland was the consensus when the prior version using the ATA_IDENTIFY came under dispute. Signed-off-by: john cooper <john.cooper@redhat.com> ---