| Message ID | 20211019074647.19061-3-vincent.whitchurch@axis.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | virtio-i2c: Fix buffer handling | expand |
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.
On 2021/10/19 16:22, Viresh Kumar wrote: > On 19-10-21, 09:46, Vincent Whitchurch wrote: >> static void virtio_i2c_msg_done(struct virtqueue *vq) >> { >> - struct virtio_i2c *vi = vq->vdev->priv; >> + struct virtio_i2c_req *req; >> + unsigned int len; >> >> - complete(&vi->completion); >> + while ((req = virtqueue_get_buf(vq, &len))) >> + complete(&req->completion); > Instead of adding a completion for each request and using only the > last one, maybe we can do this instead here: > > while ((req = virtqueue_get_buf(vq, &len))) { > if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT)) Is this for the last one check ? For the last one, this bit should be cleared, right ? > complete(&vi->completion); > } > > Since we already know which is the last one, we can also check at this > point if buffers for all other requests are received or not. >
On 20-10-21, 16:54, Jie Deng wrote: > > On 2021/10/19 16:22, Viresh Kumar wrote: > > On 19-10-21, 09:46, Vincent Whitchurch wrote: > > > static void virtio_i2c_msg_done(struct virtqueue *vq) > > > { > > > - struct virtio_i2c *vi = vq->vdev->priv; > > > + struct virtio_i2c_req *req; > > > + unsigned int len; > > > - complete(&vi->completion); > > > + while ((req = virtqueue_get_buf(vq, &len))) > > > + complete(&req->completion); > > Instead of adding a completion for each request and using only the > > last one, maybe we can do this instead here: > > > > while ((req = virtqueue_get_buf(vq, &len))) { > > if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT)) > > > Is this for the last one check ? For the last one, this bit should be > cleared, right ? Oops, you are right. This should be `!=` instead. Thanks.
On Wed, Oct 20, 2021 at 11:17:21AM +0200, Viresh Kumar wrote: > On 20-10-21, 16:54, Jie Deng wrote: > > > > On 2021/10/19 16:22, Viresh Kumar wrote: > > > On 19-10-21, 09:46, Vincent Whitchurch wrote: > > > > static void virtio_i2c_msg_done(struct virtqueue *vq) > > > > { > > > > - struct virtio_i2c *vi = vq->vdev->priv; > > > > + struct virtio_i2c_req *req; > > > > + unsigned int len; > > > > - complete(&vi->completion); > > > > + while ((req = virtqueue_get_buf(vq, &len))) > > > > + complete(&req->completion); > > > Instead of adding a completion for each request and using only the > > > last one, maybe we can do this instead here: > > > > > > while ((req = virtqueue_get_buf(vq, &len))) { > > > if (req->out_hdr.flags == cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT)) > > > > > > Is this for the last one check ? For the last one, this bit should be > > cleared, right ? > > Oops, you are right. This should be `!=` instead. Thanks. I don't quite understand how that would be safe since virtqueue_add_sgs() can fail after a few iterations and all queued request buffers can have FAIL_NEXT set. In such a case, we would end up waiting forever with your proposed change, wouldn't we?
On 20-10-21, 12:38, Vincent Whitchurch wrote: > I don't quite understand how that would be safe since > virtqueue_add_sgs() can fail after a few iterations and all queued > request buffers can have FAIL_NEXT set. In such a case, we would end up > waiting forever with your proposed change, wouldn't we? Good point. I didn't think of that earlier. I think a good simple way of handling this is counting the number of buffers sent and received. Once they match, we are done. That shouldn't break anything else I believe.
On 2021/10/19 15:46, Vincent Whitchurch wrote: > The driver currently assumes that the notify callback is only received > when the device is done with all the queued buffers. > > However, this is not true, since the notify callback could be called > without any of the queued buffers being completed (for example, with > virtio-pci and shared interrupts) or with only some of the buffers being > completed (since the driver makes them available to the device in > multiple separate virtqueue_add_sgs() calls). Can the backend driver control the time point of interrupt injection ? I can't think of why the backend has to send an early interrupt. This operation should be avoided in the backend driver if possible. However, this change make sense if early interrupt can't be avoid. > > This can lead to incorrect data on the I2C bus or memory corruption in > the guest if the device operates on buffers which are have been freed by > the driver. (The WARN_ON in the driver is also triggered.) >
On 21-10-21, 13:55, Jie Deng wrote: > Can the backend driver control the time point of interrupt injection ? I > can't think of > > why the backend has to send an early interrupt. This operation should be > avoided > > in the backend driver if possible. However, this change make sense if early > interrupt > > can't be avoid. The backend driver probably won't send an event, but the notification event, if it comes, shouldn't have side effects like what it currently have (where we finish the ongoing transfer by calling complete()).
On Wed, Oct 20, 2021 at 12:47:09PM +0200, Viresh Kumar wrote: > On 20-10-21, 12:38, Vincent Whitchurch wrote: > > I don't quite understand how that would be safe since > > virtqueue_add_sgs() can fail after a few iterations and all queued > > request buffers can have FAIL_NEXT set. In such a case, we would end up > > waiting forever with your proposed change, wouldn't we? > > Good point. I didn't think of that earlier. > > I think a good simple way of handling this is counting the number of > buffers sent and received. Once they match, we are done. That > shouldn't break anything else I believe. That could work, but it's not so straightforward since you would have to introduce locking to prevent races since the final count is only known after virtio_i2c_prepare_reqs() completes, while the callback could be called before that. Please do not hesitate to send out a patch to fix it that way if that is what you prefer.
On 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>
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;
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(-)