Virtualization - Nov 2022 - [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

On Thu, Nov 24, 2022 at 01:42:37AM -0500, Michael S. Tsirkin
wrote:
> On Wed, Nov 23, 2022 at 10:41:46PM +0000, Chaitanya Kulkarni wrote:
> > 
> > > +/* Get lifetime information from device */
> > > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk,
unsigned long arg)
> > > +{
> > > +	struct request_queue *q = vblk->disk->queue;
> > > +	struct request *req = NULL;
> > > +	struct virtblk_req *vbr;
> > > +	struct virtio_blk_lifetime lifetime;
> > > +	int ret;
> > > +
> > > +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> > > +	 * There is no check/decode on values received from the device.
> > > +	 * The data is sent as is to the user.
> > > +	 */
> > 
> > Odd comment style, why not :-
> > 
> > 	/*
> > 	 * The virtio_blk_lifetime struct fields follow virtio spec.
> > 	 * There is no check/decode on values received from the device.
> > 	 * The data is sent as is to the user.
> > 	 */
> 
> most of virtio blk is like this. I don't really care much.
> 
> > > +
> > > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > > +	 * feature is negotiated.
> > > +	 */
> > 
> > above comment is redundant to the below code as following code is
> > self explanatory.,..
> 
> i find it helpful personally - this point is important enough to
> stress at cost of some duplication.
> 
> > > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > > +		return -ENOTTY;
> > > +
> > > +	memset(&lifetime, 0, sizeof(lifetime));
> > > +
> > > +	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> > > +	if (IS_ERR(req))
> > > +		return PTR_ERR(req);
> > > +
> > > +	/* Write the correct type */
> > 
> > same here for above comment...
> 
> it's pretty harmless though
> 
> > > +	vbr = blk_mq_rq_to_pdu(req);
> > > +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev,
VIRTIO_BLK_T_GET_LIFETIME);
> > > +
> > > +	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime),
GFP_KERNEL);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	blk_execute_rq(req, false);
> > > +
> > > +	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	/* Pass the data to the user */
> > > +	if (copy_to_user((void __user *)arg, &lifetime,
sizeof(lifetime))) {
> > > +		ret = -EFAULT;
> > > +		goto out;
> > 
> > there nothing here to "goto out" following is sufficient I
guess :-
> > 
> > if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime)))
> > 	ret = -EFAULT;
> 
> error handling with goto seems cleaner, easier to add code down
> the road.
> 
> > > +	}
> > > +
> > > +out:
> > > +	blk_mq_free_request(req);
> > > +	return ret;
> > > +}
> > > +
> > 
> > [...]
> > 
> >    };
> > >   
> > > +/* Get lifetime information struct for each request */
> > > +struct virtio_blk_lifetime {
> > > +	/*
> > > +	 * specifies the percentage of reserved blocks that are
consumed.
> > > +	 * optional values following virtio spec:
> > > +	 * 0 - undefined
> > > +	 * 1 - normal, < 80% of reserved blocks are consumed
> > > +	 * 2 - warning, 80% of reserved blocks are consumed
> > > +	 * 3 - urgent, 90% of reserved blocks are consumed
> > > +	 */
> > > +	__le16 pre_eol_info;
> > > +	/*
> > > +	 * this field refers to wear of SLC cells and is provided in
increments of 10used,
> > > +	 * and so on, thru to 11 meaning estimated lifetime exceeded.
All values above 11
> > > +	 * are reserved
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_a;
> > > +	/*
> > > +	 * this field refers to wear of MLC cells and is provided with
the same semantics as
> > > +	 * device_lifetime_est_typ_a
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_b;
> > > +};
> > > +
> > 
> > are you sure there won't be any new members ever be added in
future ?
> > to make it is futuresafe I'd add padding to this structure and
keep
> > the reserve bytes to the meaningful size...
> > 
> > -ck
> 
> 
> virtio has feature bits for this kind of thing, we don't do padding.
The struct defined in the VIRTIO spec is being reused in the ioctl
userspace ABI. While feature bits are used by VIRTIO, padding is common
in the Linux UAPI.

Adding padding would require decoupling the VIRTIO spec struct from the
ioctl struct. That's probably a good idea because I noticed endianness
is left to userspace due to the __le fields in the VIRTIO spec struct
and that's likely to cause userspace bugs/confusion.

I suggest defining a separate UAPI struct for this ioctl.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20221124/24695f55/attachment-0001.sig>

Thanks Stefan
> How about naming it VBLK_GET_LIFETIME? It's clearer what the ioctl does
> and it follows the name of virtio-blk request type.
You're right, I'll rename it.
> ENOTTY already has meaning for ioctl(2):
>
>
>   ENOTTY fd is not associated with a character special device.
>
>
>   ENOTTY The specified request does not apply to the kind of object that
the file descriptor fd references.
>
>
> Use ENOTSUP instead?
 ENOTSUP seems like a better fit, I'll change the error code.
> In terms of security, any process with access to the block device node
> is allowed to get the lifetime?
>
>
> Usually only privileged processes have access to block device nodes, but
> I wanted to check whether anyone can think of a scenario where this is
> not okay.
>
>
> For example, a virtio-blk device may have a partition that an untrusted
> process like a database or key-value store accesses. Can the untrusted
> process read the lifetime information of the entire device?

I agree that only a privileged process should be able to read the lifetime.
I could add something like:
if (!capable(CAP_SYS_ADMIN))
        return -EPERM;
> It's unusual for an ioctl to produce a struct that's not in CPU
> endianness. I think the kernel should deal with endianness here.
The endianness was discussed in the first version:
> > After more thought, I think that the driver should handle the
> > virtio_blk_lifetime struct endianness.
> > Something like:
> > ...
> > lifetime.pre_eol_info = __le16_to_cpu(lifetime.pre_eol_info);
> > lifetime. device_lifetime_est_typ_a  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_a);
> > lifetime. device_lifetime_est_typ_b  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_b);
> >
> > if (copy_to_user((void __user *)arg, (void *)&lifetime,
> > ...
> >
> > What do you think?
>
>
> I think if you are going to pass struct virtio_blk_lifetime to
> userspace, better pass it as defined in the spec, in LE format.

I tend to agree that endianness should be taken care of in the kernel.
> We need to check that vblk->vdev is non-NULL before accessing it in
> virtblk_ioctl_lifetime():
>
>
>   if (!vblk->vdev) {
>       mutex_unlock(&vblk->dev_mutex);
>       return -ENXIO;
>   }
>
>
> Without the check I expect virtblk_ioctl_lifetime() to dereference a
> NULL pointer.
Right

Alvaro

> I suggest defining a separate UAPI struct for this ioctl.
Sounds fine to me.
I could use the following struct

struct virtio_blk_lifetime_ioctl {
        u16 pre_eol_info;
        u16 device_lifetime_est_typ_a;
        u16 device_lifetime_est_typ_b;
        u16 padding;
};