* [PATCH 0/2] virtio-i2c: Fix buffer handling @ 2021-10-19 7:46 Vincent Whitchurch 2021-10-19 7:46 ` [PATCH 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch 2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch 0 siblings, 2 replies; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-19 7:46 UTC (permalink / raw) To: wsa, jie.deng, viresh.kumar Cc: linux-kernel, Vincent Whitchurch, kernel, linux-i2c, virtualization 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 7:46 [PATCH 0/2] virtio-i2c: Fix buffer handling Vincent Whitchurch @ 2021-10-19 7:46 ` Vincent Whitchurch 2021-10-19 8:09 ` Viresh Kumar 2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch 1 sibling, 1 reply; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-19 7:46 UTC (permalink / raw) To: wsa, jie.deng, viresh.kumar Cc: linux-kernel, Vincent Whitchurch, kernel, linux-i2c, virtualization 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 7:46 ` [PATCH 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch @ 2021-10-19 8:09 ` Viresh Kumar 2021-10-19 9:36 ` Greg KH 2021-10-20 3:36 ` Jie Deng 0 siblings, 2 replies; 32+ messages in thread From: Viresh Kumar @ 2021-10-19 8:09 UTC (permalink / raw) To: Vincent Whitchurch, gregkh Cc: linux-kernel, virtualization, wsa, kernel, linux-i2c +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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 8:09 ` Viresh Kumar @ 2021-10-19 9:36 ` Greg KH 2021-10-19 9:42 ` Viresh Kumar 2021-10-20 3:36 ` Jie Deng 1 sibling, 1 reply; 32+ messages in thread From: Greg KH @ 2021-10-19 9:36 UTC (permalink / raw) To: Viresh Kumar Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 9:36 ` Greg KH @ 2021-10-19 9:42 ` Viresh Kumar 2021-10-19 11:16 ` Greg KH [not found] ` <YW6owLxoYbxG5GxT@ninjato> 0 siblings, 2 replies; 32+ messages in thread From: Viresh Kumar @ 2021-10-19 9:42 UTC (permalink / raw) To: Greg KH Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 9:42 ` Viresh Kumar @ 2021-10-19 11:16 ` Greg KH 2021-10-19 14:37 ` Viresh Kumar [not found] ` <YW6owLxoYbxG5GxT@ninjato> 1 sibling, 1 reply; 32+ messages in thread From: Greg KH @ 2021-10-19 11:16 UTC (permalink / raw) To: Viresh Kumar Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 11:16 ` Greg KH @ 2021-10-19 14:37 ` Viresh Kumar [not found] ` <YW8LFTcBuN1bB3PD@ninjato> 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-19 14:37 UTC (permalink / raw) To: Greg KH Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <YW8LFTcBuN1bB3PD@ninjato>]
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling [not found] ` <YW8LFTcBuN1bB3PD@ninjato> @ 2021-10-20 4:20 ` Jie Deng 2021-10-20 5:36 ` Greg KH 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-20 4:20 UTC (permalink / raw) To: Wolfram Sang, Viresh Kumar, Greg KH, Vincent Whitchurch, virtualization, linux-i2c, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 4:20 ` Jie Deng @ 2021-10-20 5:36 ` Greg KH 2021-10-20 6:35 ` Jie Deng 0 siblings, 1 reply; 32+ messages in thread From: Greg KH @ 2021-10-20 5:36 UTC (permalink / raw) To: Jie Deng Cc: Viresh Kumar, Vincent Whitchurch, linux-kernel, virtualization, Wolfram Sang, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 5:36 ` Greg KH @ 2021-10-20 6:35 ` Jie Deng 2021-10-20 6:41 ` Viresh Kumar 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-20 6:35 UTC (permalink / raw) To: Greg KH Cc: Viresh Kumar, Vincent Whitchurch, linux-kernel, virtualization, Wolfram Sang, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 6:35 ` Jie Deng @ 2021-10-20 6:41 ` Viresh Kumar 2021-10-20 7:04 ` Jie Deng 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-20 6:41 UTC (permalink / raw) To: Jie Deng Cc: Greg KH, Vincent Whitchurch, linux-kernel, virtualization, Wolfram Sang, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 6:41 ` Viresh Kumar @ 2021-10-20 7:04 ` Jie Deng 2021-10-20 10:55 ` Vincent Whitchurch 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-20 7:04 UTC (permalink / raw) To: Viresh Kumar Cc: Greg KH, Vincent Whitchurch, linux-kernel, virtualization, Wolfram Sang, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 7:04 ` Jie Deng @ 2021-10-20 10:55 ` Vincent Whitchurch 2021-10-20 11:03 ` Viresh Kumar 0 siblings, 1 reply; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-20 10:55 UTC (permalink / raw) To: Jie Deng Cc: Viresh Kumar, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org, Greg KH 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 10:55 ` Vincent Whitchurch @ 2021-10-20 11:03 ` Viresh Kumar 2021-10-21 3:30 ` Jie Deng 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-20 11:03 UTC (permalink / raw) To: Vincent Whitchurch Cc: Greg KH, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-20 11:03 ` Viresh Kumar @ 2021-10-21 3:30 ` Jie Deng 2021-10-29 12:24 ` Vincent Whitchurch 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-21 3:30 UTC (permalink / raw) To: Viresh Kumar, Vincent Whitchurch Cc: Greg KH, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-21 3:30 ` Jie Deng @ 2021-10-29 12:24 ` Vincent Whitchurch 2021-11-01 5:23 ` Jie Deng 0 siblings, 1 reply; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-29 12:24 UTC (permalink / raw) To: Jie Deng Cc: Viresh Kumar, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org, Greg KH 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-29 12:24 ` Vincent Whitchurch @ 2021-11-01 5:23 ` Jie Deng [not found] ` <MWHPR11MB0030C3489F38FF2AAF7C3D0A908C9@MWHPR11MB0030.namprd11.prod.outlook.com> 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-11-01 5:23 UTC (permalink / raw) To: Vincent Whitchurch Cc: Viresh Kumar, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org, Greg KH, Conghui Chen 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <MWHPR11MB0030C3489F38FF2AAF7C3D0A908C9@MWHPR11MB0030.namprd11.prod.outlook.com>]
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling [not found] ` <MWHPR11MB0030C3489F38FF2AAF7C3D0A908C9@MWHPR11MB0030.namprd11.prod.outlook.com> @ 2021-11-03 6:37 ` Viresh Kumar 2021-11-03 14:42 ` Vincent Whitchurch 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-11-03 6:37 UTC (permalink / raw) To: Chen, Conghui Cc: Greg KH, Vincent Whitchurch, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-11-03 6:37 ` Viresh Kumar @ 2021-11-03 14:42 ` Vincent Whitchurch 2021-11-09 4:52 ` Viresh Kumar 0 siblings, 1 reply; 32+ messages in thread From: Vincent Whitchurch @ 2021-11-03 14:42 UTC (permalink / raw) To: Viresh Kumar Cc: Greg KH, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org, Chen, Conghui 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-11-03 14:42 ` Vincent Whitchurch @ 2021-11-09 4:52 ` Viresh Kumar 0 siblings, 0 replies; 32+ messages in thread From: Viresh Kumar @ 2021-11-09 4:52 UTC (permalink / raw) To: Vincent Whitchurch Cc: Greg KH, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Wolfram Sang, kernel, linux-i2c@vger.kernel.org, Chen, Conghui 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <YW6owLxoYbxG5GxT@ninjato>]
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling [not found] ` <YW6owLxoYbxG5GxT@ninjato> @ 2021-10-19 14:14 ` Viresh Kumar 0 siblings, 0 replies; 32+ messages in thread From: Viresh Kumar @ 2021-10-19 14:14 UTC (permalink / raw) To: Wolfram Sang, Greg KH, Vincent Whitchurch, jie.deng, virtualization, linux-i2c, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] i2c: virtio: disable timeout handling 2021-10-19 8:09 ` Viresh Kumar 2021-10-19 9:36 ` Greg KH @ 2021-10-20 3:36 ` Jie Deng 1 sibling, 0 replies; 32+ messages in thread From: Jie Deng @ 2021-10-20 3:36 UTC (permalink / raw) To: Viresh Kumar, Vincent Whitchurch, gregkh Cc: wsa, linux-kernel, kernel, linux-i2c, virtualization 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-19 7:46 [PATCH 0/2] virtio-i2c: Fix buffer handling Vincent Whitchurch 2021-10-19 7:46 ` [PATCH 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch @ 2021-10-19 7:46 ` Vincent Whitchurch 2021-10-19 8:22 ` Viresh Kumar ` (2 more replies) 1 sibling, 3 replies; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-19 7:46 UTC (permalink / raw) To: wsa, jie.deng, viresh.kumar Cc: linux-kernel, Vincent Whitchurch, kernel, linux-i2c, virtualization 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch @ 2021-10-19 8:22 ` Viresh Kumar 2021-10-20 8:54 ` Jie Deng 2021-10-21 5:55 ` Jie Deng 2021-11-02 4:32 ` Viresh Kumar 2 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-19 8:22 UTC (permalink / raw) To: Vincent Whitchurch; +Cc: linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-19 8:22 ` Viresh Kumar @ 2021-10-20 8:54 ` Jie Deng 2021-10-20 9:17 ` Viresh Kumar 0 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-20 8:54 UTC (permalink / raw) To: Viresh Kumar, Vincent Whitchurch Cc: wsa, linux-kernel, kernel, linux-i2c, virtualization 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-20 8:54 ` Jie Deng @ 2021-10-20 9:17 ` Viresh Kumar 2021-10-20 10:38 ` Vincent Whitchurch 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-20 9:17 UTC (permalink / raw) To: Jie Deng Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-20 9:17 ` Viresh Kumar @ 2021-10-20 10:38 ` Vincent Whitchurch 2021-10-20 10:47 ` Viresh Kumar 0 siblings, 1 reply; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-20 10:38 UTC (permalink / raw) To: Viresh Kumar Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wsa@kernel.org, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-20 10:38 ` Vincent Whitchurch @ 2021-10-20 10:47 ` Viresh Kumar 2021-10-29 11:54 ` Vincent Whitchurch 0 siblings, 1 reply; 32+ messages in thread From: Viresh Kumar @ 2021-10-20 10:47 UTC (permalink / raw) To: Vincent Whitchurch Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wsa@kernel.org, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-20 10:47 ` Viresh Kumar @ 2021-10-29 11:54 ` Vincent Whitchurch 0 siblings, 0 replies; 32+ messages in thread From: Vincent Whitchurch @ 2021-10-29 11:54 UTC (permalink / raw) To: Viresh Kumar Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wsa@kernel.org, kernel, linux-i2c@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch 2021-10-19 8:22 ` Viresh Kumar @ 2021-10-21 5:55 ` Jie Deng 2021-10-21 5:58 ` Viresh Kumar 2021-11-02 4:32 ` Viresh Kumar 2 siblings, 1 reply; 32+ messages in thread From: Jie Deng @ 2021-10-21 5:55 UTC (permalink / raw) To: Vincent Whitchurch, wsa, viresh.kumar Cc: linux-kernel, kernel, linux-i2c, virtualization 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-21 5:55 ` Jie Deng @ 2021-10-21 5:58 ` Viresh Kumar 0 siblings, 0 replies; 32+ messages in thread From: Viresh Kumar @ 2021-10-21 5:58 UTC (permalink / raw) To: Jie Deng Cc: Vincent Whitchurch, linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] i2c: virtio: fix completion handling 2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch 2021-10-19 8:22 ` Viresh Kumar 2021-10-21 5:55 ` Jie Deng @ 2021-11-02 4:32 ` Viresh Kumar 2 siblings, 0 replies; 32+ messages in thread From: Viresh Kumar @ 2021-11-02 4:32 UTC (permalink / raw) To: Vincent Whitchurch; +Cc: linux-kernel, virtualization, wsa, kernel, linux-i2c 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2021-11-09 4:52 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-19 7:46 [PATCH 0/2] virtio-i2c: Fix buffer handling Vincent Whitchurch
2021-10-19 7:46 ` [PATCH 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch
2021-10-19 8:09 ` Viresh Kumar
2021-10-19 9:36 ` Greg KH
2021-10-19 9:42 ` Viresh Kumar
2021-10-19 11:16 ` Greg KH
2021-10-19 14:37 ` Viresh Kumar
[not found] ` <YW8LFTcBuN1bB3PD@ninjato>
2021-10-20 4:20 ` Jie Deng
2021-10-20 5:36 ` Greg KH
2021-10-20 6:35 ` Jie Deng
2021-10-20 6:41 ` Viresh Kumar
2021-10-20 7:04 ` Jie Deng
2021-10-20 10:55 ` Vincent Whitchurch
2021-10-20 11:03 ` Viresh Kumar
2021-10-21 3:30 ` Jie Deng
2021-10-29 12:24 ` Vincent Whitchurch
2021-11-01 5:23 ` Jie Deng
[not found] ` <MWHPR11MB0030C3489F38FF2AAF7C3D0A908C9@MWHPR11MB0030.namprd11.prod.outlook.com>
2021-11-03 6:37 ` Viresh Kumar
2021-11-03 14:42 ` Vincent Whitchurch
2021-11-09 4:52 ` Viresh Kumar
[not found] ` <YW6owLxoYbxG5GxT@ninjato>
2021-10-19 14:14 ` Viresh Kumar
2021-10-20 3:36 ` Jie Deng
2021-10-19 7:46 ` [PATCH 2/2] i2c: virtio: fix completion handling Vincent Whitchurch
2021-10-19 8:22 ` Viresh Kumar
2021-10-20 8:54 ` Jie Deng
2021-10-20 9:17 ` Viresh Kumar
2021-10-20 10:38 ` Vincent Whitchurch
2021-10-20 10:47 ` Viresh Kumar
2021-10-29 11:54 ` Vincent Whitchurch
2021-10-21 5:55 ` Jie Deng
2021-10-21 5:58 ` Viresh Kumar
2021-11-02 4:32 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).