Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 03, 2021 at 04:45:51PM +0000, Stefan Hajnoczi wrote:
On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:
> On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
> > +static void vdpasim_blk_work(struct work_struct *work)
> > +{
> > +	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > +	u8 status = VIRTIO_BLK_S_OK;
> > +	int i;
> > +
> > +	spin_lock(&vdpasim->lock);
> > +
> > +	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > +		goto out;
> > +
> > +	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > +		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> > +
> > +		if (!vq->ready)
> > +			continue;
> > +
> > +		while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
> > +					    &vq->in_iov, &vq->head,
> > +					    GFP_ATOMIC) > 0) {
> > +			int write;
> > +
> > +			vq->in_iov.i = vq->in_iov.used - 1;
> > +			write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
> > +						      &status, 1);
> > +			if (write <= 0)
> > +				break;
>
> This code looks fragile:
>
> 1. Relying on unsigned underflow and the while loop in
>   vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
>   risky and could break.
>
> 2. Does this assume that the last in_iov element has size 1? For
>   example, the guest driver may send a single "in" iovec with size 513
>   when reading 512 bytes (with an extra byte for the request status).
>
> Please validate inputs fully, even in test/development code, because
> it's likely to be copied by others when writing production code (or
> deployed in production by unsuspecting users) :).

Perfectly agree on that, so I addressed these things, also following your
review on the previous version, on the next patch of this series:
"vdpa_sim_blk: implement ramdisk behaviour".

Do you think should I move these checks in this patch?

I did this to leave Max credit for this patch and add more code to emulate a
ramdisk in later patches.

You could update the commit description so it's clear that input
validation is missing and will be added in the next commit.

I'll do it.

Thanks,
Stefano




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux