[PATCH 0/2] virtio-i2c: Fix buffer handling

[PATCH 0/2] virtio-i2c: Fix buffer handling

[Vincent Whitchurch]

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

[PATCH 0/2] virtio-i2c: Fix buffer handling

[Vincent Whitchurch]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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 <vincent.whitchurch@axis.com>
---
 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

[PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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 <vincent.whitchurch@axis.com>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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 <vincent.whitchurch@axis.com>
---
 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

[PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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 <vincent.whitchurch@axis.com>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

+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 <vincent.whitchurch@axis.com>
> ---
>  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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

+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 <vincent.whitchurch@axis.com>
> ---
>  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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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 <vincent.whitchurch@axis.com>
> > ---
> >  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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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 <vincent.whitchurch@axis.com>
> > ---
> >  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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]


> 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().


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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 ?

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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 ?





_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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?

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Greg KH]

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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Jie Deng]

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.
>

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Jie Deng]

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.
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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?

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Jie Deng]

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.)
>

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Jie Deng]

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.)
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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.

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Vincent Whitchurch]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Jie Deng]

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.


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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 <vincent.whitchurch@axis.com>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 2/2] i2c: virtio: fix completion handling

[Viresh Kumar]

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 <vincent.whitchurch@axis.com>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/2] i2c: virtio: disable timeout handling

[Chen, Conghui]

>>> 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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Vincent Whitchurch]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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

Re: [PATCH 1/2] i2c: virtio: disable timeout handling

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization