* [PATCH v2 0/2] virtio-i2c: Fix buffer handling
@ 2021-11-11 16:04 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: conghui.chen, mst, virtualization, linux-i2c, linux-kernel,
kernel, 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.
Changes in v2:
- Added Acked-by and Fixes tags
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 0/2] virtio-i2c: Fix buffer handling
@ 2021-11-11 16:04 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: mst, Vincent Whitchurch, linux-kernel, virtualization, kernel,
linux-i2c, conghui.chen
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.
Changes in v2:
- Added Acked-by and Fixes tags
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-11 16:04 ` Vincent Whitchurch
-1 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: conghui.chen, mst, virtualization, linux-i2c, linux-kernel,
kernel, 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.
Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
Acked-by: Jie Deng <jie.deng@intel.com>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 1/2] i2c: virtio: disable timeout handling
@ 2021-11-11 16:04 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: mst, Vincent Whitchurch, linux-kernel, virtualization, kernel,
linux-i2c, conghui.chen
If a timeout is hit, it can result is incorrect data on the I2C bus
and/or memory corruptions in the guest since the device can still be
operating on the buffers it was given while the guest has freed them.
Here is, for example, the start of a slub_debug splat which was
triggered on the next transfer after one transfer was forced to timeout
by setting a breakpoint in the backend (rust-vmm/vhost-device):
BUG kmalloc-1k (Not tainted): Poison overwritten
First byte 0x1 instead of 0x6b
Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
__kmalloc+0xc2/0x1c9
virtio_i2c_xfer+0x65/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
kfree+0x1bd/0x1cc
virtio_i2c_xfer+0x32e/0x35c
__i2c_transfer+0x429/0x57d
i2c_transfer+0x115/0x134
i2cdev_ioctl_rdwr+0x16a/0x1de
i2cdev_ioctl+0x247/0x2ed
vfs_ioctl+0x21/0x30
sys_ioctl+0xb18/0xb41
There is no simple fix for this (the driver would have to always create
bounce buffers and hold on to them until the device eventually returns
the buffers), so just disable the timeout support for now.
Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
Acked-by: Jie Deng <jie.deng@intel.com>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/2] i2c: virtio: fix completion handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-11 16:04 ` Vincent Whitchurch
-1 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: conghui.chen, mst, virtualization, linux-i2c, linux-kernel,
kernel, 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.
Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/2] i2c: virtio: fix completion handling
@ 2021-11-11 16:04 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 16:04 UTC (permalink / raw)
To: wsa, jie.deng, viresh.kumar
Cc: mst, Vincent Whitchurch, linux-kernel, virtualization, kernel,
linux-i2c, conghui.chen
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.
Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-11 16:46 ` Michael S. Tsirkin
-1 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-11 16:46 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: wsa, jie.deng, viresh.kumar, conghui.chen, virtualization,
linux-i2c, linux-kernel, kernel
On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@intel.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
For an eventual fix, I think you'd have to reset the device,
then you can get free up the outstanding buffers.
This has to be done carefully to make sure it does
not race with interrupts and/or new requests, typically
not easy.
> ---
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
@ 2021-11-11 16:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-11 16:46 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: viresh.kumar, linux-kernel, virtualization, wsa, kernel,
linux-i2c, conghui.chen
On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@intel.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
For an eventual fix, I think you'd have to reset the device,
then you can get free up the outstanding buffers.
This has to be done carefully to make sure it does
not race with interrupts and/or new requests, typically
not easy.
> ---
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] i2c: virtio: fix completion handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-11 16:57 ` Michael S. Tsirkin
-1 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-11 16:57 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: wsa, jie.deng, viresh.kumar, conghui.chen, virtualization,
linux-i2c, linux-kernel, kernel
On Thu, Nov 11, 2021 at 05:04:12PM +0100, 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.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 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.
> + */
Hmm the spec only says:
A device MUST guarantee the requests in the virtqueue being processed in order
if multiple requests are received at a time.
it does not seem to require using the buffers in order.
In any case, just waiting for all of them in a loop
seems cleaner and likely won't take longer ...
> + 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] i2c: virtio: fix completion handling
@ 2021-11-11 16:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-11 16:57 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: viresh.kumar, linux-kernel, virtualization, wsa, kernel,
linux-i2c, conghui.chen
On Thu, Nov 11, 2021 at 05:04:12PM +0100, 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.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 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.
> + */
Hmm the spec only says:
A device MUST guarantee the requests in the virtqueue being processed in order
if multiple requests are received at a time.
it does not seem to require using the buffers in order.
In any case, just waiting for all of them in a loop
seems cleaner and likely won't take longer ...
> + 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-12 2:35 ` Viresh Kumar
-1 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-12 2:35 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: wsa, jie.deng, conghui.chen, mst, virtualization, linux-i2c,
linux-kernel, kernel
On 11-11-21, 17:04, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@intel.com>
> 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);
I thought we decided on making this in insanely high value instead ?
--
viresh
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
@ 2021-11-12 2:35 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-12 2:35 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: mst, linux-kernel, virtualization, wsa, kernel, linux-i2c, conghui.chen
On 11-11-21, 17:04, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@intel.com>
> 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);
I thought we decided on making this in insanely high value instead ?
--
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-12 2:35 ` Viresh Kumar
@ 2021-11-19 15:30 ` Vincent Whitchurch
-1 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-19 15:30 UTC (permalink / raw)
To: Viresh Kumar
Cc: wsa, jie.deng, conghui.chen, mst, virtualization, linux-i2c,
linux-kernel, kernel
On Fri, Nov 12, 2021 at 03:35:29AM +0100, Viresh Kumar wrote:
> On 11-11-21, 17:04, Vincent Whitchurch wrote:
> > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > struct virtqueue *vq = vi->vq;
> > struct virtio_i2c_req *reqs;
> > - unsigned long time_left;
> > int count;
> >
> > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > reinit_completion(&vi->completion);
> > virtqueue_kick(vq);
> >
> > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> > - if (!time_left)
> > - dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> > + wait_for_completion(&vi->completion);
>
> I thought we decided on making this in insanely high value instead ?
That wasn't my impression from the previous email thread. Jie was OK
with doing it either way, and only disabling the timeout entirely makes
sense to me given the risk for memory corruption otherwise.
What "insanely high" timeout value do you have in mind and why would it
be acceptable to corrupt kernel memory after that time?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
@ 2021-11-19 15:30 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-11-19 15:30 UTC (permalink / raw)
To: Viresh Kumar
Cc: mst, linux-kernel, virtualization, wsa, kernel, linux-i2c, conghui.chen
On Fri, Nov 12, 2021 at 03:35:29AM +0100, Viresh Kumar wrote:
> On 11-11-21, 17:04, Vincent Whitchurch wrote:
> > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > struct virtqueue *vq = vi->vq;
> > struct virtio_i2c_req *reqs;
> > - unsigned long time_left;
> > int count;
> >
> > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > reinit_completion(&vi->completion);
> > virtqueue_kick(vq);
> >
> > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> > - if (!time_left)
> > - dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> > + wait_for_completion(&vi->completion);
>
> I thought we decided on making this in insanely high value instead ?
That wasn't my impression from the previous email thread. Jie was OK
with doing it either way, and only disabling the timeout entirely makes
sense to me given the risk for memory corruption otherwise.
What "insanely high" timeout value do you have in mind and why would it
be acceptable to corrupt kernel memory after that time?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-11 16:04 ` Vincent Whitchurch
` (2 preceding siblings ...)
(?)
@ 2021-11-23 9:52 ` Wolfram Sang
2021-11-23 9:54 ` Viresh Kumar
-1 siblings, 1 reply; 27+ messages in thread
From: Wolfram Sang @ 2021-11-23 9:52 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: jie.deng, viresh.kumar, conghui.chen, mst, virtualization,
linux-i2c, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:
> If a timeout is hit, it can result is incorrect data on the I2C bus
> and/or memory corruptions in the guest since the device can still be
> operating on the buffers it was given while the guest has freed them.
>
> Here is, for example, the start of a slub_debug splat which was
> triggered on the next transfer after one transfer was forced to timeout
> by setting a breakpoint in the backend (rust-vmm/vhost-device):
>
> BUG kmalloc-1k (Not tainted): Poison overwritten
> First byte 0x1 instead of 0x6b
> Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> __kmalloc+0xc2/0x1c9
> virtio_i2c_xfer+0x65/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
> Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> kfree+0x1bd/0x1cc
> virtio_i2c_xfer+0x32e/0x35c
> __i2c_transfer+0x429/0x57d
> i2c_transfer+0x115/0x134
> i2cdev_ioctl_rdwr+0x16a/0x1de
> i2cdev_ioctl+0x247/0x2ed
> vfs_ioctl+0x21/0x30
> sys_ioctl+0xb18/0xb41
>
> There is no simple fix for this (the driver would have to always create
> bounce buffers and hold on to them until the device eventually returns
> the buffers), so just disable the timeout support for now.
>
> Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> Acked-by: Jie Deng <jie.deng@intel.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-23 9:52 ` Wolfram Sang
@ 2021-11-23 9:54 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-23 9:54 UTC (permalink / raw)
To: Wolfram Sang, Vincent Whitchurch, jie.deng, conghui.chen, mst,
virtualization, linux-i2c, linux-kernel, kernel
On 23-11-21, 10:52, Wolfram Sang wrote:
> On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:
> > If a timeout is hit, it can result is incorrect data on the I2C bus
> > and/or memory corruptions in the guest since the device can still be
> > operating on the buffers it was given while the guest has freed them.
> >
> > Here is, for example, the start of a slub_debug splat which was
> > triggered on the next transfer after one transfer was forced to timeout
> > by setting a breakpoint in the backend (rust-vmm/vhost-device):
> >
> > BUG kmalloc-1k (Not tainted): Poison overwritten
> > First byte 0x1 instead of 0x6b
> > Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> > __kmalloc+0xc2/0x1c9
> > virtio_i2c_xfer+0x65/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> > Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> > kfree+0x1bd/0x1cc
> > virtio_i2c_xfer+0x32e/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> >
> > There is no simple fix for this (the driver would have to always create
> > bounce buffers and hold on to them until the device eventually returns
> > the buffers), so just disable the timeout support for now.
> >
> > Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> > Acked-by: Jie Deng <jie.deng@intel.com>
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Applied to for-current, thanks!
>
Thanks, I completely forgot replying to the last email from Vincent.
FWIW,
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
@ 2021-11-23 9:54 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-23 9:54 UTC (permalink / raw)
To: Wolfram Sang, Vincent Whitchurch, jie.deng, conghui.chen, mst,
virtualization, linux-i2c, linux-kernel, kernel
On 23-11-21, 10:52, Wolfram Sang wrote:
> On Thu, Nov 11, 2021 at 05:04:11PM +0100, Vincent Whitchurch wrote:
> > If a timeout is hit, it can result is incorrect data on the I2C bus
> > and/or memory corruptions in the guest since the device can still be
> > operating on the buffers it was given while the guest has freed them.
> >
> > Here is, for example, the start of a slub_debug splat which was
> > triggered on the next transfer after one transfer was forced to timeout
> > by setting a breakpoint in the backend (rust-vmm/vhost-device):
> >
> > BUG kmalloc-1k (Not tainted): Poison overwritten
> > First byte 0x1 instead of 0x6b
> > Allocated in virtio_i2c_xfer+0x65/0x35c age=350 cpu=0 pid=29
> > __kmalloc+0xc2/0x1c9
> > virtio_i2c_xfer+0x65/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> > Freed in virtio_i2c_xfer+0x32e/0x35c age=244 cpu=0 pid=29
> > kfree+0x1bd/0x1cc
> > virtio_i2c_xfer+0x32e/0x35c
> > __i2c_transfer+0x429/0x57d
> > i2c_transfer+0x115/0x134
> > i2cdev_ioctl_rdwr+0x16a/0x1de
> > i2cdev_ioctl+0x247/0x2ed
> > vfs_ioctl+0x21/0x30
> > sys_ioctl+0xb18/0xb41
> >
> > There is no simple fix for this (the driver would have to always create
> > bounce buffers and hold on to them until the device eventually returns
> > the buffers), so just disable the timeout support for now.
> >
> > Fixes: 3cfc88380413d20f ("i2c: virtio: add a virtio i2c frontend driver")
> > Acked-by: Jie Deng <jie.deng@intel.com>
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> Applied to for-current, thanks!
>
Thanks, I completely forgot replying to the last email from Vincent.
FWIW,
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] i2c: virtio: disable timeout handling
2021-11-23 9:54 ` Viresh Kumar
(?)
@ 2021-11-23 9:55 ` Wolfram Sang
-1 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2021-11-23 9:55 UTC (permalink / raw)
To: Viresh Kumar
Cc: Vincent Whitchurch, jie.deng, conghui.chen, mst, virtualization,
linux-i2c, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 185 bytes --]
> Thanks, I completely forgot replying to the last email from Vincent.
His reasoning was convincing me :)
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Thanks, I added it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
2021-11-11 16:04 ` Vincent Whitchurch
@ 2021-11-24 23:55 ` Michael S. Tsirkin
-1 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-24 23:55 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: wsa, jie.deng, viresh.kumar, conghui.chen, virtualization,
linux-i2c, linux-kernel, kernel
On Thu, Nov 11, 2021 at 05:04:10PM +0100, Vincent Whitchurch wrote:
> 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.
>
> Changes in v2:
> - Added Acked-by and Fixes tags
What are the plans for this patchset?
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
@ 2021-11-24 23:55 ` Michael S. Tsirkin
0 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2021-11-24 23:55 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: viresh.kumar, linux-kernel, virtualization, wsa, kernel,
linux-i2c, conghui.chen
On Thu, Nov 11, 2021 at 05:04:10PM +0100, Vincent Whitchurch wrote:
> 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.
>
> Changes in v2:
> - Added Acked-by and Fixes tags
What are the plans for this patchset?
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
2021-11-24 23:55 ` Michael S. Tsirkin
@ 2021-11-25 3:21 ` Viresh Kumar
-1 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-25 3:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel,
linux-i2c, conghui.chen
On 24-11-21, 18:55, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 05:04:10PM +0100, Vincent Whitchurch wrote:
> > 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.
> >
> > Changes in v2:
> > - Added Acked-by and Fixes tags
>
>
> What are the plans for this patchset?
Wolfram applied the first patch, but not the second.
Wolfram, you can apply that one as well, it looks okay.
--
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
@ 2021-11-25 3:21 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-25 3:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Vincent Whitchurch, wsa, jie.deng, conghui.chen, virtualization,
linux-i2c, linux-kernel, kernel
On 24-11-21, 18:55, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 05:04:10PM +0100, Vincent Whitchurch wrote:
> > 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.
> >
> > Changes in v2:
> > - Added Acked-by and Fixes tags
>
>
> What are the plans for this patchset?
Wolfram applied the first patch, but not the second.
Wolfram, you can apply that one as well, it looks okay.
--
viresh
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
2021-11-25 3:21 ` Viresh Kumar
(?)
@ 2021-11-25 6:24 ` Wolfram Sang
2021-11-25 6:47 ` Viresh Kumar
-1 siblings, 1 reply; 27+ messages in thread
From: Wolfram Sang @ 2021-11-25 6:24 UTC (permalink / raw)
To: Viresh Kumar
Cc: Michael S. Tsirkin, Vincent Whitchurch, jie.deng, conghui.chen,
virtualization, linux-i2c, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 242 bytes --]
> Wolfram, you can apply that one as well, it looks okay.
Is it? I read that the code only waits for the last request while
Michael suggested to wait for all of them? And he did not ack patch 2
while he acked patch 1. Did I misunderstand?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
2021-11-25 6:24 ` Wolfram Sang
@ 2021-11-25 6:47 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-25 6:47 UTC (permalink / raw)
To: Wolfram Sang, Michael S. Tsirkin, Vincent Whitchurch, jie.deng,
conghui.chen, virtualization, linux-i2c, linux-kernel, kernel
On 25-11-21, 07:24, Wolfram Sang wrote:
>
> > Wolfram, you can apply that one as well, it looks okay.
>
> Is it? I read that the code only waits for the last request while
> Michael suggested to wait for all of them? And he did not ack patch 2
> while he acked patch 1. Did I misunderstand?
Okay, I misread it then.
To clarify, we should initialize the completion for each buffer and
wait for all of them to be completed before returning back to the
user.
Lets wait for an update by Vincent for that then.
--
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/2] virtio-i2c: Fix buffer handling
@ 2021-11-25 6:47 ` Viresh Kumar
0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2021-11-25 6:47 UTC (permalink / raw)
To: Wolfram Sang, Michael S. Tsirkin, Vincent Whitchurch, jie.deng,
conghui.chen, virtualization, linux-i2c, linux-kernel, kernel
On 25-11-21, 07:24, Wolfram Sang wrote:
>
> > Wolfram, you can apply that one as well, it looks okay.
>
> Is it? I read that the code only waits for the last request while
> Michael suggested to wait for all of them? And he did not ack patch 2
> while he acked patch 1. Did I misunderstand?
Okay, I misread it then.
To clarify, we should initialize the completion for each buffer and
wait for all of them to be completed before returning back to the
user.
Lets wait for an update by Vincent for that then.
--
viresh
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] i2c: virtio: fix completion handling
2021-11-11 16:57 ` Michael S. Tsirkin
@ 2021-12-02 15:34 ` Vincent Whitchurch
-1 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-12-02 15:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jie.deng, viresh.kumar, linux-kernel, virtualization, wsa,
kernel, linux-i2c, conghui.chen
On Thu, Nov 11, 2021 at 05:57:30PM +0100, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 05:04:12PM +0100, Vincent Whitchurch wrote:
> > - wait_for_completion(&vi->completion);
> > + /*
> > + * We only need to wait for the last one since the device is required
> > + * to complete requests in order.
> > + */
>
> Hmm the spec only says:
>
> A device MUST guarantee the requests in the virtqueue being processed in order
> if multiple requests are received at a time.
>
> it does not seem to require using the buffers in order.
> In any case, just waiting for all of them in a loop
> seems cleaner and likely won't take longer ...
Thank you, I've fixed this in v3.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] i2c: virtio: fix completion handling
@ 2021-12-02 15:34 ` Vincent Whitchurch
0 siblings, 0 replies; 27+ messages in thread
From: Vincent Whitchurch @ 2021-12-02 15:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: viresh.kumar, linux-kernel, virtualization, wsa, kernel,
linux-i2c, conghui.chen
On Thu, Nov 11, 2021 at 05:57:30PM +0100, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 05:04:12PM +0100, Vincent Whitchurch wrote:
> > - wait_for_completion(&vi->completion);
> > + /*
> > + * We only need to wait for the last one since the device is required
> > + * to complete requests in order.
> > + */
>
> Hmm the spec only says:
>
> A device MUST guarantee the requests in the virtqueue being processed in order
> if multiple requests are received at a time.
>
> it does not seem to require using the buffers in order.
> In any case, just waiting for all of them in a loop
> seems cleaner and likely won't take longer ...
Thank you, I've fixed this in v3.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-12-02 15:34 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 16:04 [PATCH v2 0/2] virtio-i2c: Fix buffer handling Vincent Whitchurch
2021-11-11 16:04 ` Vincent Whitchurch
2021-11-11 16:04 ` [PATCH v2 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch
2021-11-11 16:04 ` Vincent Whitchurch
2021-11-11 16:46 ` Michael S. Tsirkin
2021-11-11 16:46 ` Michael S. Tsirkin
2021-11-12 2:35 ` Viresh Kumar
2021-11-12 2:35 ` Viresh Kumar
2021-11-19 15:30 ` Vincent Whitchurch
2021-11-19 15:30 ` Vincent Whitchurch
2021-11-23 9:52 ` Wolfram Sang
2021-11-23 9:54 ` Viresh Kumar
2021-11-23 9:54 ` Viresh Kumar
2021-11-23 9:55 ` Wolfram Sang
2021-11-11 16:04 ` [PATCH v2 2/2] i2c: virtio: fix completion handling Vincent Whitchurch
2021-11-11 16:04 ` Vincent Whitchurch
2021-11-11 16:57 ` Michael S. Tsirkin
2021-11-11 16:57 ` Michael S. Tsirkin
2021-12-02 15:34 ` Vincent Whitchurch
2021-12-02 15:34 ` Vincent Whitchurch
2021-11-24 23:55 ` [PATCH v2 0/2] virtio-i2c: Fix buffer handling Michael S. Tsirkin
2021-11-24 23:55 ` Michael S. Tsirkin
2021-11-25 3:21 ` Viresh Kumar
2021-11-25 3:21 ` Viresh Kumar
2021-11-25 6:24 ` Wolfram Sang
2021-11-25 6:47 ` Viresh Kumar
2021-11-25 6:47 ` Viresh Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.