On 11-11-21, 17:04, 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.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@xxxxxxxxx>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
> ---
> 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);
I thought we decided on making this in insanely high value instead ?
--
viresh
