This fixes a couple of bugs in the buffer handling in virtio-i2c which can
result in incorrect data on the I2C bus or memory corruption in the guest.
I tested this on UML (virtio-uml needs a bug fix too, I will sent that out
later) with the device implementation in rust-vmm/vhost-device.
Vincent Whitchurch (2):
i2c: virtio: disable timeout handling
i2c: virtio: fix completion handling
drivers/i2c/busses/i2c-virtio.c | 46 ++++++++++++++-------------------
1 file changed, 19 insertions(+), 27 deletions(-)
--
2.28.0
The driver currently assumes that the notify callback is only received
when the device is done with all the queued buffers.
However, this is not true, since the notify callback could be called
without any of the queued buffers being completed (for example, with
virtio-pci and shared interrupts) or with only some of the buffers being
completed (since the driver makes them available to the device in
multiple separate virtqueue_add_sgs() calls).
This can lead to incorrect data on the I2C bus or memory corruption in
the guest if the device operates on buffers which are have been freed by
the driver. (The WARN_ON in the driver is also triggered.)
BUG kmalloc-128 (Tainted: G W ): Poison overwritten
First byte 0x0 instead of 0x6b
Allocated in i2cdev_ioctl_rdwr+0x9d/0x1de age=243 cpu=0 pid=28
memdup_user+0x2e/0xbd
i2cdev_ioctl_rdwr+0x9d/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
Freed in i2cdev_ioctl_rdwr+0x1bb/0x1de age=68 cpu=0 pid=28
kfree+0x1bd/0x1cc
i2cdev_ioctl_rdwr+0x1bb/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
Fix this by calling virtio_get_buf() from the notify handler like other
virtio drivers and by actually waiting for all the buffers to be
completed.
Signed-off-by: Vincent Whitchurch <[email protected]>
---
drivers/i2c/busses/i2c-virtio.c | 34 +++++++++++++++------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 7b2474e6876f..2d3ae8e238ec 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -22,24 +22,24 @@
/**
* struct virtio_i2c - virtio I2C data
* @vdev: virtio device for this controller
- * @completion: completion of virtio I2C message
* @adap: I2C adapter for this controller
* @vq: the virtio virtqueue for communication
*/
struct virtio_i2c {
struct virtio_device *vdev;
- struct completion completion;
struct i2c_adapter adap;
struct virtqueue *vq;
};
/**
* struct virtio_i2c_req - the virtio I2C request structure
+ * @completion: completion of virtio I2C message
* @out_hdr: the OUT header of the virtio I2C message
* @buf: the buffer into which data is read, or from which it's written
* @in_hdr: the IN header of the virtio I2C message
*/
struct virtio_i2c_req {
+ struct completion completion;
struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
uint8_t *buf ____cacheline_aligned;
struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
@@ -47,9 +47,11 @@ struct virtio_i2c_req {
static void virtio_i2c_msg_done(struct virtqueue *vq)
{
- struct virtio_i2c *vi = vq->vdev->priv;
+ struct virtio_i2c_req *req;
+ unsigned int len;
- complete(&vi->completion);
+ while ((req = virtqueue_get_buf(vq, &len)))
+ complete(&req->completion);
}
static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
@@ -69,6 +71,8 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
if (!msgs[i].len)
break;
+ init_completion(&reqs[i].completion);
+
/*
* Only 7-bit mode supported for this moment. For the address
* format, Please check the Virtio I2C Specification.
@@ -108,21 +112,13 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
struct virtio_i2c_req *reqs,
struct i2c_msg *msgs, int num)
{
- struct virtio_i2c_req *req;
bool failed = false;
- unsigned int len;
int i, j = 0;
for (i = 0; i < num; i++) {
- /* Detach the ith request from the vq */
- req = virtqueue_get_buf(vq, &len);
+ struct virtio_i2c_req *req = &reqs[i];
- /*
- * Condition req == &reqs[i] should always meet since we have
- * total num requests in the vq. reqs[i] can never be NULL here.
- */
- if (!failed && (WARN_ON(req != &reqs[i]) ||
- req->in_hdr.status != VIRTIO_I2C_MSG_OK))
+ if (!failed && req->in_hdr.status != VIRTIO_I2C_MSG_OK)
failed = true;
i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
@@ -158,11 +154,13 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
* remote here to clear the virtqueue, so we can try another set of
* messages later on.
*/
-
- reinit_completion(&vi->completion);
virtqueue_kick(vq);
- wait_for_completion(&vi->completion);
+ /*
+ * We only need to wait for the last one since the device is required
+ * to complete requests in order.
+ */
+ wait_for_completion(&reqs[count - 1].completion);
count = virtio_i2c_complete_reqs(vq, reqs, msgs, count);
@@ -211,8 +209,6 @@ static int virtio_i2c_probe(struct virtio_device *vdev)
vdev->priv = vi;
vi->vdev = vdev;
- init_completion(&vi->completion);
-
ret = virtio_i2c_setup_vqs(vi);
if (ret)
return ret;
--
2.28.0
If a timeout is hit, it can result is incorrect data on the I2C bus
and/or memory corruptions in the guest since the device can still be
operating on the buffers it was given while the guest has freed them.
Here is, for example, the start of a slub_debug splat which was
triggered on the next transfer after one transfer was forced to timeout
by setting a breakpoint in the backend (rust-vmm/vhost-device):
BUG kmalloc-1k (Not tainted): Poison overwritten
First byte 0x1 instead of 0x6b
Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
__kmalloc+0xc2/0x1c9
virtio_i2c_xfer+0x65/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
kfree+0x1bd/0x1cc
virtio_i2c_xfer+0x32e/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
There is no simple fix for this (the driver would have to always create
bounce buffers and hold on to them until the device eventually returns
the buffers), so just disable the timeout support for now.
Signed-off-by: Vincent Whitchurch <[email protected]>
---
drivers/i2c/busses/i2c-virtio.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index f10a603b13fb..7b2474e6876f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
static int virtio_i2c_complete_reqs(struct virtqueue *vq,
struct virtio_i2c_req *reqs,
- struct i2c_msg *msgs, int num,
- bool timedout)
+ struct i2c_msg *msgs, int num)
{
struct virtio_i2c_req *req;
- bool failed = timedout;
+ bool failed = false;
unsigned int len;
int i, j = 0;
@@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
j++;
}
- return timedout ? -ETIMEDOUT : j;
+ return j;
}
static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
@@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
struct virtio_i2c *vi = i2c_get_adapdata(adap);
struct virtqueue *vq = vi->vq;
struct virtio_i2c_req *reqs;
- unsigned long time_left;
int count;
reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
@@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
reinit_completion(&vi->completion);
virtqueue_kick(vq);
- time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
- if (!time_left)
- dev_err(&adap->dev, "virtio i2c backend timeout.\n");
+ wait_for_completion(&vi->completion);
- count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left);
+ count = virtio_i2c_complete_reqs(vq, reqs, msgs, count);
err_free:
kfree(reqs);
--
2.28.0
+Greg.
On 19-10-21, 09:46, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
That is a very valid problem, and I have faced it too when my QEMU
setup is very slow :)
> Signed-off-by: Vincent Whitchurch <[email protected]>
> ---
> drivers/i2c/busses/i2c-virtio.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index f10a603b13fb..7b2474e6876f 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
>
> static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> struct virtio_i2c_req *reqs,
> - struct i2c_msg *msgs, int num,
> - bool timedout)
> + struct i2c_msg *msgs, int num)
> {
> struct virtio_i2c_req *req;
> - bool failed = timedout;
> + bool failed = false;
> unsigned int len;
> int i, j = 0;
>
> @@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> j++;
> }
>
> - return timedout ? -ETIMEDOUT : j;
> + return j;
> }
>
> static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> struct virtio_i2c *vi = i2c_get_adapdata(adap);
> struct virtqueue *vq = vi->vq;
> struct virtio_i2c_req *reqs;
> - unsigned long time_left;
> int count;
>
> reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> reinit_completion(&vi->completion);
> virtqueue_kick(vq);
>
> - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> - if (!time_left)
> - dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> + wait_for_completion(&vi->completion);
Doing this may not be a good thing based on the kernel rules I have
understood until now. Maybe Greg and Wolfram can clarify on this.
We are waiting here for an external entity (Host kernel) or a firmware
that uses virtio for transport. If the other side is hacked, it can
make the kernel hang here for ever. I thought that is something that
the kernel should never do.
--
viresh
On 19-10-21, 09:46, Vincent Whitchurch wrote:
> static void virtio_i2c_msg_done(struct virtqueue *vq)
> {
> - struct virtio_i2c *vi = vq->vdev->priv;
> + struct virtio_i2c_req *req;
> + unsigned int len;
>
> - complete(&vi->completion);
> + while ((req = virtqueue_get_buf(vq, &len)))
> + complete(&req->completion);
Instead of adding a completion for each request and using only the
last one, maybe we can do this instead here:
while ((req = virtqueue_get_buf(vq, &len))) {
if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT))
complete(&vi->completion);
}
Since we already know which is the last one, we can also check at this
point if buffers for all other requests are received or not.
--
viresh
On Tue, Oct 19, 2021 at 01:39:13PM +0530, Viresh Kumar wrote:
> +Greg.
>
> On 19-10-21, 09:46, Vincent Whitchurch wrote:
> > If a timeout is hit, it can result is incorrect data on the I2C bus
> > and/or memory corruptions in the guest since the device can still be
> > operating on the buffers it was given while the guest has freed them.
> >
> > Here is, for example, the start of a slub_debug splat which was
> > triggered on the next transfer after one transfer was forced to timeout
> > by setting a breakpoint in the backend (rust-vmm/vhost-device):
> >
> > BUG kmalloc-1k (Not tainted): Poison overwritten
> > First byte 0x1 instead of 0x6b
> > Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> > __kmalloc+0xc2/0x1c9
> > virtio_i2c_xfer+0x65/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> > Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> > kfree+0x1bd/0x1cc
> > virtio_i2c_xfer+0x32e/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> >
> > There is no simple fix for this (the driver would have to always create
> > bounce buffers and hold on to them until the device eventually returns
> > the buffers), so just disable the timeout support for now.
>
> That is a very valid problem, and I have faced it too when my QEMU
> setup is very slow :)
>
> > Signed-off-by: Vincent Whitchurch <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-virtio.c | 14 +++++---------
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> > index f10a603b13fb..7b2474e6876f 100644
> > --- a/drivers/i2c/busses/i2c-virtio.c
> > +++ b/drivers/i2c/busses/i2c-virtio.c
> > @@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> >
> > static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> > struct virtio_i2c_req *reqs,
> > - struct i2c_msg *msgs, int num,
> > - bool timedout)
> > + struct i2c_msg *msgs, int num)
> > {
> > struct virtio_i2c_req *req;
> > - bool failed = timedout;
> > + bool failed = false;
> > unsigned int len;
> > int i, j = 0;
> >
> > @@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> > j++;
> > }
> >
> > - return timedout ? -ETIMEDOUT : j;
> > + return j;
> > }
> >
> > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > struct virtqueue *vq = vi->vq;
> > struct virtio_i2c_req *reqs;
> > - unsigned long time_left;
> > int count;
> >
> > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > reinit_completion(&vi->completion);
> > virtqueue_kick(vq);
> >
> > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> > - if (!time_left)
> > - dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> > + wait_for_completion(&vi->completion);
>
> Doing this may not be a good thing based on the kernel rules I have
> understood until now. Maybe Greg and Wolfram can clarify on this.
>
> We are waiting here for an external entity (Host kernel) or a firmware
> that uses virtio for transport. If the other side is hacked, it can
> make the kernel hang here for ever. I thought that is something that
> the kernel should never do.
What is the "other side" here? Is it something that you trust or not?
Usually we trust the hardware, but if you do not trust the hardware,
then yes, you need to have a timeout here.
thanks,
greg k-h
On 19-10-21, 11:36, Greg KH wrote:
> What is the "other side" here? Is it something that you trust or not?
Other side can be a remote processor (for remoteproc over virtio or
something similar), or traditionally it can be host OS or host
firmware providing virtualisation to a Guest running Linux (this
driver). Or something else..
I would incline towards "we trust the other side" here.
> Usually we trust the hardware, but if you do not trust the hardware,
> then yes, you need to have a timeout here.
The other side is the software that has access to the _Real_ hardware,
and so we should trust it. So we can have a actually have a completion
without timeout here, interesting.
--
viresh
> The other side is the software that has access to the _Real_ hardware,
> and so we should trust it. So we can have a actually have a completion
> without timeout here, interesting.
So, if the other side gets a timeout talking to the HW, then the timeout
error will be propagated? If so, then we may live with plain
wait_for_completion().
On Tue, Oct 19, 2021 at 03:12:03PM +0530, Viresh Kumar wrote:
> On 19-10-21, 11:36, Greg KH wrote:
> > What is the "other side" here? Is it something that you trust or not?
>
> Other side can be a remote processor (for remoteproc over virtio or
> something similar), or traditionally it can be host OS or host
> firmware providing virtualisation to a Guest running Linux (this
> driver). Or something else..
>
> I would incline towards "we trust the other side" here.
That's in contradition with what other people seem to think the virtio
drivers are for, see this crazy thread for details about that:
https://lore.kernel.org/all/20211009003711.1390019-1-sathyanarayanan.kuppuswamy@linux.intel.com/
You can "trust" the hardware, but also handle things when hardware is
broken, which is most often the case in the real world.
So why is having a timeout a problem here? If you have an overloaded
system, you want things to time out so that you can start to recover.
> > Usually we trust the hardware, but if you do not trust the hardware,
> > then yes, you need to have a timeout here.
>
> The other side is the software that has access to the _Real_ hardware,
> and so we should trust it. So we can have a actually have a completion
> without timeout here, interesting.
And if that hardware stops working? Timeouts are good to have, why not
just bump it up a bit if you are running into it in a real-world
situation?
thanks,
greg k-h
On 19-10-21, 13:15, Wolfram Sang wrote:
>
> > The other side is the software that has access to the _Real_ hardware,
> > and so we should trust it. So we can have a actually have a completion
> > without timeout here, interesting.
>
> So, if the other side gets a timeout talking to the HW, then the timeout
> error will be propagated?
It should be, ideally :)
> If so, then we may live with plain wait_for_completion().
--
viresh
On 19-10-21, 13:16, Greg KH wrote:
> On Tue, Oct 19, 2021 at 03:12:03PM +0530, Viresh Kumar wrote:
> > On 19-10-21, 11:36, Greg KH wrote:
> > > What is the "other side" here? Is it something that you trust or not?
> >
> > Other side can be a remote processor (for remoteproc over virtio or
> > something similar), or traditionally it can be host OS or host
> > firmware providing virtualisation to a Guest running Linux (this
> > driver). Or something else..
> >
> > I would incline towards "we trust the other side" here.
>
> That's in contradition with what other people seem to think the virtio
> drivers are for, see this crazy thread for details about that:
> https://lore.kernel.org/all/20211009003711.1390019-1-sathyanarayanan.kuppuswamy@linux.intel.com/
>
> You can "trust" the hardware, but also handle things when hardware is
> broken, which is most often the case in the real world.
That's what I was worried about when I got you in, broken or hacked :)
> So why is having a timeout a problem here? If you have an overloaded
> system, you want things to time out so that you can start to recover.
>
> And if that hardware stops working? Timeouts are good to have, why not
> just bump it up a bit if you are running into it in a real-world
> situation?
I think it is set to HZ currently, though I haven't tried big
transfers but I still get into some issues with Qemu based stuff.
Maybe we can bump it up to few seconds :)
--
viresh
> I think it is set to HZ currently, though I haven't tried big
> transfers but I still get into some issues with Qemu based stuff.
> Maybe we can bump it up to few seconds :)
If you use adapter->timeout, this can even be set at runtime using a
ioctl. So, it can adapt to use cases. Of course, the driver should
initialize it to a sane default if the automatic default (HZ) is not
suitable.
On 2021/10/19 16:09, Viresh Kumar wrote:
> Doing this may not be a good thing based on the kernel rules I have
> understood until now. Maybe Greg and Wolfram can clarify on this.
>
> We are waiting here for an external entity (Host kernel) or a firmware
> that uses virtio for transport. If the other side is hacked, it can
> make the kernel hang here for ever. I thought that is something that
> the kernel should never do.
I'm also worried about this. We may be able to solve it by setting a big
timeout value in the driver.
On 2021/10/20 2:14, Wolfram Sang wrote:
>> I think it is set to HZ currently, though I haven't tried big
>> transfers but I still get into some issues with Qemu based stuff.
>> Maybe we can bump it up to few seconds :)
> If you use adapter->timeout, this can even be set at runtime using a
> ioctl. So, it can adapt to use cases. Of course, the driver should
> initialize it to a sane default if the automatic default (HZ) is not
> suitable.
I think a big value may solve most cases. but the driver never know how
big is enough by static configuration.
Can we make this value to be configurable, just let the other side
provide this value ?
On Wed, Oct 20, 2021 at 12:20:13PM +0800, Jie Deng wrote:
>
> On 2021/10/20 2:14, Wolfram Sang wrote:
> > > I think it is set to HZ currently, though I haven't tried big
> > > transfers but I still get into some issues with Qemu based stuff.
> > > Maybe we can bump it up to few seconds :)
> > If you use adapter->timeout, this can even be set at runtime using a
> > ioctl. So, it can adapt to use cases. Of course, the driver should
> > initialize it to a sane default if the automatic default (HZ) is not
> > suitable.
>
>
> I think a big value may solve most cases. but the driver never know how big
> is enough by static configuration.
>
> Can we make this value to be configurable, just let the other side provide
> this value ?
If an ioctl can change it, that would mean it is configurable, right?
On 2021/10/20 13:36, Greg KH wrote:
> On Wed, Oct 20, 2021 at 12:20:13PM +0800, Jie Deng wrote:
>> On 2021/10/20 2:14, Wolfram Sang wrote:
>>>> I think it is set to HZ currently, though I haven't tried big
>>>> transfers but I still get into some issues with Qemu based stuff.
>>>> Maybe we can bump it up to few seconds :)
>>> If you use adapter->timeout, this can even be set at runtime using a
>>> ioctl. So, it can adapt to use cases. Of course, the driver should
>>> initialize it to a sane default if the automatic default (HZ) is not
>>> suitable.
>>
>> I think a big value may solve most cases. but the driver never know how big
>> is enough by static configuration.
>>
>> Can we make this value to be configurable, just let the other side provide
>> this value ?
> If an ioctl can change it, that would mean it is configurable, right?
Yes, but we need to know what's the best value to be configured for a
specific "other side".
I think the "other side" should be more aware of what value is
reasonable to be used.
On 20-10-21, 14:35, Jie Deng wrote:
> Yes, but we need to know what's the best value to be configured for a
> specific "other side".
>
> I think the "other side" should be more aware of what value is reasonable to
> be used.
If we _really_ need that, then it would require an update to the
specification first.
I am not sure if the other side is the only party in play here. It
depends on the entire setup and not just the hardware device.
Specially with virtualisation things become really slow because of
context switches, etc. It may be better for the guest userspace to
decide on the value.
Since this is specially for virtualisation, the kernel may set the
value to few HZ by default, lets say 10 (Yeah its really high) :).
--
viresh
On 2021/10/20 14:41, Viresh Kumar wrote:
> On 20-10-21, 14:35, Jie Deng wrote:
>> Yes, but we need to know what's the best value to be configured for a
>> specific "other side".
>>
>> I think the "other side" should be more aware of what value is reasonable to
>> be used.
> If we _really_ need that, then it would require an update to the
> specification first.
>
> I am not sure if the other side is the only party in play here. It
> depends on the entire setup and not just the hardware device.
> Specially with virtualisation things become really slow because of
> context switches, etc. It may be better for the guest userspace to
> decide on the value.
>
> Since this is specially for virtualisation, the kernel may set the
> value to few HZ by default, lets say 10 (Yeah its really high) :).
I'm OK with this way for the simplicity.
On 2021/10/19 16:22, Viresh Kumar wrote:
> On 19-10-21, 09:46, Vincent Whitchurch wrote:
>> static void virtio_i2c_msg_done(struct virtqueue *vq)
>> {
>> - struct virtio_i2c *vi = vq->vdev->priv;
>> + struct virtio_i2c_req *req;
>> + unsigned int len;
>>
>> - complete(&vi->completion);
>> + while ((req = virtqueue_get_buf(vq, &len)))
>> + complete(&req->completion);
> Instead of adding a completion for each request and using only the
> last one, maybe we can do this instead here:
>
> while ((req = virtqueue_get_buf(vq, &len))) {
> if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT))
Is this for the last one check ? For the last one, this bit should be
cleared, right ?
> complete(&vi->completion);
> }
>
> Since we already know which is the last one, we can also check at this
> point if buffers for all other requests are received or not.
>
On 20-10-21, 16:54, Jie Deng wrote:
>
> On 2021/10/19 16:22, Viresh Kumar wrote:
> > On 19-10-21, 09:46, Vincent Whitchurch wrote:
> > > static void virtio_i2c_msg_done(struct virtqueue *vq)
> > > {
> > > - struct virtio_i2c *vi = vq->vdev->priv;
> > > + struct virtio_i2c_req *req;
> > > + unsigned int len;
> > > - complete(&vi->completion);
> > > + while ((req = virtqueue_get_buf(vq, &len)))
> > > + complete(&req->completion);
> > Instead of adding a completion for each request and using only the
> > last one, maybe we can do this instead here:
> >
> > while ((req = virtqueue_get_buf(vq, &len))) {
> > if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT))
>
>
> Is this for the last one check ? For the last one, this bit should be
> cleared, right ?
Oops, you are right. This should be `!=` instead. Thanks.
--
viresh
On Wed, Oct 20, 2021 at 11:17:21AM +0200, Viresh Kumar wrote:
> On 20-10-21, 16:54, Jie Deng wrote:
> >
> > On 2021/10/19 16:22, Viresh Kumar wrote:
> > > On 19-10-21, 09:46, Vincent Whitchurch wrote:
> > > > static void virtio_i2c_msg_done(struct virtqueue *vq)
> > > > {
> > > > - struct virtio_i2c *vi = vq->vdev->priv;
> > > > + struct virtio_i2c_req *req;
> > > > + unsigned int len;
> > > > - complete(&vi->completion);
> > > > + while ((req = virtqueue_get_buf(vq, &len)))
> > > > + complete(&req->completion);
> > > Instead of adding a completion for each request and using only the
> > > last one, maybe we can do this instead here:
> > >
> > > while ((req = virtqueue_get_buf(vq, &len))) {
> > > if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT))
> >
> >
> > Is this for the last one check ? For the last one, this bit should be
> > cleared, right ?
>
> Oops, you are right. This should be `!=` instead. Thanks.
I don't quite understand how that would be safe since
virtqueue_add_sgs() can fail after a few iterations and all queued
request buffers can have FAIL_NEXT set. In such a case, we would end up
waiting forever with your proposed change, wouldn't we?
On 20-10-21, 12:38, Vincent Whitchurch wrote:
> I don't quite understand how that would be safe since
> virtqueue_add_sgs() can fail after a few iterations and all queued
> request buffers can have FAIL_NEXT set. In such a case, we would end up
> waiting forever with your proposed change, wouldn't we?
Good point. I didn't think of that earlier.
I think a good simple way of handling this is counting the number of
buffers sent and received. Once they match, we are done. That
shouldn't break anything else I believe.
--
viresh
On Wed, Oct 20, 2021 at 09:04:45AM +0200, Jie Deng wrote:
> On 2021/10/20 14:41, Viresh Kumar wrote:
> > On 20-10-21, 14:35, Jie Deng wrote:
> >> Yes, but we need to know what's the best value to be configured for a
> >> specific "other side".
> >>
> >> I think the "other side" should be more aware of what value is reasonable to
> >> be used.
> > If we _really_ need that, then it would require an update to the
> > specification first.
> >
> > I am not sure if the other side is the only party in play here. It
> > depends on the entire setup and not just the hardware device.
> > Specially with virtualisation things become really slow because of
> > context switches, etc. It may be better for the guest userspace to
> > decide on the value.
> >
> > Since this is specially for virtualisation, the kernel may set the
> > value to few HZ by default, lets say 10 (Yeah its really high) :).
>
> I'm OK with this way for the simplicity.
That would not be safe. Userspace can change this timeout and the end
result with the current implementation of the driver is potentially
kernel memory corruption, which is something userspace of course never
should be able to trigger.
Even if the timeout were hardcoded in the driver and the driver made to
ignore what userspace requests, it would need to be set to a
ridiculously high value so that we can guarantee that the timeout never
ever occurs (since the result is potentially random kernel memory
corruption). Which is basically equivalent to disabling the timeout
entirely as my patch does.
If the timeout cannot be disabled, then the driver should be fixed to
always copy buffers and hold on to them to avoid memory corruption in
the case of timeout, as I mentioned in my commit message. That would be
quite a substantial change to the driver so it's not something I'm
personally comfortable with doing, especially not this late in the -rc
cycle, so I'd leave that to others.
On 20-10-21, 12:55, Vincent Whitchurch wrote:
> If the timeout cannot be disabled, then the driver should be fixed to
> always copy buffers and hold on to them to avoid memory corruption in
> the case of timeout, as I mentioned in my commit message. That would be
> quite a substantial change to the driver so it's not something I'm
> personally comfortable with doing, especially not this late in the -rc
> cycle, so I'd leave that to others.
Or we can avoid clearing up and freeing the buffers here until the
point where the buffers are returned by the host. Until that happens,
we can avoid taking new requests but return to the earlier caller with
timeout failure. That would avoid corruption, by freeing buffers
sooner, and not hanging of the kernel.
--
viresh
On 2021/10/20 19:03, Viresh Kumar wrote:
> On 20-10-21, 12:55, Vincent Whitchurch wrote:
>> If the timeout cannot be disabled, then the driver should be fixed to
>> always copy buffers and hold on to them to avoid memory corruption in
>> the case of timeout, as I mentioned in my commit message. That would be
>> quite a substantial change to the driver so it's not something I'm
>> personally comfortable with doing, especially not this late in the -rc
>> cycle, so I'd leave that to others.
> Or we can avoid clearing up and freeing the buffers here until the
> point where the buffers are returned by the host. Until that happens,
> we can avoid taking new requests but return to the earlier caller with
> timeout failure. That would avoid corruption, by freeing buffers
> sooner, and not hanging of the kernel.
It seems similar to use "wait_for_completion". If the other side is
hacked, the guest may never
get the buffers returned by the host, right ?
For this moment, we can solve the problem by using a hardcoded big value
or disabling the timeout.
Over the long term, I think the backend should provide that timeout
value and guarantee that its processing
time should not exceed that value.
On 2021/10/19 15:46, Vincent Whitchurch wrote:
> The driver currently assumes that the notify callback is only received
> when the device is done with all the queued buffers.
>
> However, this is not true, since the notify callback could be called
> without any of the queued buffers being completed (for example, with
> virtio-pci and shared interrupts) or with only some of the buffers being
> completed (since the driver makes them available to the device in
> multiple separate virtqueue_add_sgs() calls).
Can the backend driver control the time point of interrupt injection ? I
can't think of
why the backend has to send an early interrupt. This operation should be
avoided
in the backend driver if possible. However, this change make sense if
early interrupt
can't be avoid.
>
> This can lead to incorrect data on the I2C bus or memory corruption in
> the guest if the device operates on buffers which are have been freed by
> the driver. (The WARN_ON in the driver is also triggered.)
>
On 21-10-21, 13:55, Jie Deng wrote:
> Can the backend driver control the time point of interrupt injection ? I
> can't think of
>
> why the backend has to send an early interrupt. This operation should be
> avoided
>
> in the backend driver if possible. However, this change make sense if early
> interrupt
>
> can't be avoid.
The backend driver probably won't send an event, but the notification
event, if it comes, shouldn't have side effects like what it currently
have (where we finish the ongoing transfer by calling complete()).
--
viresh
On Wed, Oct 20, 2021 at 12:47:09PM +0200, Viresh Kumar wrote:
> On 20-10-21, 12:38, Vincent Whitchurch wrote:
> > I don't quite understand how that would be safe since
> > virtqueue_add_sgs() can fail after a few iterations and all queued
> > request buffers can have FAIL_NEXT set. In such a case, we would end up
> > waiting forever with your proposed change, wouldn't we?
>
> Good point. I didn't think of that earlier.
>
> I think a good simple way of handling this is counting the number of
> buffers sent and received. Once they match, we are done. That
> shouldn't break anything else I believe.
That could work, but it's not so straightforward since you would have to
introduce locking to prevent races since the final count is only known
after virtio_i2c_prepare_reqs() completes, while the callback could be
called before that. Please do not hesitate to send out a patch to fix
it that way if that is what you prefer.
On Thu, Oct 21, 2021 at 05:30:28AM +0200, Jie Deng wrote:
> On 2021/10/20 19:03, Viresh Kumar wrote:
> > On 20-10-21, 12:55, Vincent Whitchurch wrote:
> >> If the timeout cannot be disabled, then the driver should be fixed to
> >> always copy buffers and hold on to them to avoid memory corruption in
> >> the case of timeout, as I mentioned in my commit message. That would be
> >> quite a substantial change to the driver so it's not something I'm
> >> personally comfortable with doing, especially not this late in the -rc
> >> cycle, so I'd leave that to others.
> > Or we can avoid clearing up and freeing the buffers here until the
> > point where the buffers are returned by the host. Until that happens,
> > we can avoid taking new requests but return to the earlier caller with
> > timeout failure. That would avoid corruption, by freeing buffers
> > sooner, and not hanging of the kernel.
>
> It seems similar to use "wait_for_completion". If the other side is
> hacked, the guest may never get the buffers returned by the host,
> right ?
Note that it is trivial for the host to DoS the guest. All the host has
to do is stop responding to I/O requests (I2C or others), then the guest
will not be able to perform its intended functions, regardless of
whether this particular driver waits forever or not. Even TDX (which
Greg mentioned) does not prevent that, see:
https://lore.kernel.org/virtualization/?q=tdx+dos
> For this moment, we can solve the problem by using a hardcoded big
> value or disabling the timeout.
Is that an Acked-by on this patch which does the latter?
> Over the long term, I think the backend should provide that timeout
> value and guarantee that its processing time should not exceed that
> value.
If you mean that the spec should be changed to allow the virtio driver
to be able to program a certain timeout for I2C transactions in the
virtio device, yes, that does sound reasonable.
On 2021/10/29 20:24, Vincent Whitchurch wrote:
> On Thu, Oct 21, 2021 at 05:30:28AM +0200, Jie Deng wrote:
>> For this moment, we can solve the problem by using a hardcoded big
>> value or disabling the timeout.
> Is that an Acked-by on this patch which does the latter?
Yes, you can add my Acked-by. Let's see if other people still have
different opinions.
>
>> Over the long term, I think the backend should provide that timeout
>> value and guarantee that its processing time should not exceed that
>> value.
> If you mean that the spec should be changed to allow the virtio driver
> to be able to program a certain timeout for I2C transactions in the
> virtio device, yes, that does sound reasonable.
Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui.
She may work on this in the future.
On 19-10-21, 09:46, Vincent Whitchurch wrote:
> The driver currently assumes that the notify callback is only received
> when the device is done with all the queued buffers.
>
> However, this is not true, since the notify callback could be called
> without any of the queued buffers being completed (for example, with
> virtio-pci and shared interrupts) or with only some of the buffers being
> completed (since the driver makes them available to the device in
> multiple separate virtqueue_add_sgs() calls).
>
> This can lead to incorrect data on the I2C bus or memory corruption in
> the guest if the device operates on buffers which are have been freed by
> the driver. (The WARN_ON in the driver is also triggered.)
>
> BUG kmalloc-128 (Tainted: G W ): Poison overwritten
> First byte 0x0 instead of 0x6b
> Allocated in i2cdev_ioctl_rdwr+0x9d/0x1de age=243 cpu=0 pid=28
> memdup_user+0x2e/0xbd
> i2cdev_ioctl_rdwr+0x9d/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in i2cdev_ioctl_rdwr+0x1bb/0x1de age=68 cpu=0 pid=28
> kfree+0x1bd/0x1cc
> i2cdev_ioctl_rdwr+0x1bb/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> Fix this by calling virtio_get_buf() from the notify handler like other
> virtio drivers and by actually waiting for all the buffers to be
> completed.
>
Add a fixes tag here please, so it can get picked to the buggy kernel
version as well.
> Signed-off-by: Vincent Whitchurch <[email protected]>
Acked-by: Viresh Kumar <[email protected]>
--
viresh
>>> Over the long term, I think the backend should provide that timeout
>>> value and guarantee that its processing time should not exceed that
>>> value.
>> If you mean that the spec should be changed to allow the virtio driver
>> to be able to program a certain timeout for I2C transactions in the
>> virtio device, yes, that does sound reasonable.
>
>
>Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui.
>
>She may work on this in the future.
>
I'll try to update the spec first.
- Conghui
On 03-11-21, 06:18, Chen, Conghui wrote:
> >>> Over the long term, I think the backend should provide that timeout
> >>> value and guarantee that its processing time should not exceed that
> >>> value.
> >> If you mean that the spec should be changed to allow the virtio driver
> >> to be able to program a certain timeout for I2C transactions in the
> >> virtio device, yes, that does sound reasonable.
> >
> >
> >Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui.
> >
> >She may work on this in the future.
> >
>
> I'll try to update the spec first.
I don't think the spec should be changed for timeout. Timeout-interval
here isn't the property of just the host firmware/kernel, but the
entire setup plays a role here.
Host have its own timeframe to take care of things (I think HZ should
really be enough for that, since kernel can manage it for busses
normally with just that). Then comes the virtualization, context
switches, guest OS, backend, etc, which add to this delay. All this is
not part of the virtio protocol and so shouldn't be made part of it.
--
viresh
On Wed, Nov 03, 2021 at 07:37:45AM +0100, Viresh Kumar wrote:
> On 03-11-21, 06:18, Chen, Conghui wrote:
> > >>> Over the long term, I think the backend should provide that timeout
> > >>> value and guarantee that its processing time should not exceed that
> > >>> value.
> > >> If you mean that the spec should be changed to allow the virtio driver
> > >> to be able to program a certain timeout for I2C transactions in the
> > >> virtio device, yes, that does sound reasonable.
> > >
> > >
> > >Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui.
> > >
> > >She may work on this in the future.
> > >
> >
> > I'll try to update the spec first.
>
> I don't think the spec should be changed for timeout. Timeout-interval
> here isn't the property of just the host firmware/kernel, but the
> entire setup plays a role here.
>
> Host have its own timeframe to take care of things (I think HZ should
> really be enough for that, since kernel can manage it for busses
> normally with just that). Then comes the virtualization, context
> switches, guest OS, backend, etc, which add to this delay. All this is
> not part of the virtio protocol and so shouldn't be made part of it.
The suggested timeout is not meant to take into account the overhead of
virtualization, but to be used by the virtio device as a timeout for the
transaction on the I2C bus (presumably by programming this value to the
physical I2C controller, if one exists).
Assume that userspace (or an I2C client driver) asks for a timeout of 20
ms for a particular transfer because it, say, knows that the particular
connected I2C peripheral either responds within 10 ms to a particular
register read or never responds, so it doesn't want to waste time
waiting unnecessarily long for the transfer to complete.
If the virtio device end does not have any information on what timeout
is required (as in the current spec), it must assume some high value
which will never cause I2C transactions to spuriously timeout, say 10
seconds.
Even if the virtio driver is fixed to copy and hold all buffers to avoid
memory corruption and to time out and return to the caller after the
requested 20 ms, the next I2C transfer can not be issued until 10
seconds have passed, since the virtio device end will still be waiting
for the hardcoded 10 second timeout and may not respond to new requests
until that transfer has timed out.
On 03-11-21, 15:42, Vincent Whitchurch wrote:
> The suggested timeout is not meant to take into account the overhead of
> virtualization, but to be used by the virtio device as a timeout for the
> transaction on the I2C bus (presumably by programming this value to the
> physical I2C controller, if one exists).
>
> Assume that userspace (or an I2C client driver) asks for a timeout of 20
> ms for a particular transfer because it, say, knows that the particular
> connected I2C peripheral either responds within 10 ms to a particular
> register read or never responds, so it doesn't want to waste time
> waiting unnecessarily long for the transfer to complete.
>
> If the virtio device end does not have any information on what timeout
> is required (as in the current spec), it must assume some high value
> which will never cause I2C transactions to spuriously timeout, say 10
> seconds.
>
> Even if the virtio driver is fixed to copy and hold all buffers to avoid
> memory corruption and to time out and return to the caller after the
> requested 20 ms, the next I2C transfer can not be issued until 10
> seconds have passed, since the virtio device end will still be waiting
> for the hardcoded 10 second timeout and may not respond to new requests
> until that transfer has timed out.
Okay, so this is more about making sure the device times-out before
the driver or lets say in an expected time-frame. That should be okay
I guess.
--
viresh