Re: [PATCH 1/2] i2c: virtio: disable timeout handling
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
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On Wed, Nov 03, 2021 at 07:37:45AM +0100, Viresh Kumar wrote: > On 03-11-21, 06:18, Chen, Conghui wrote: > > >>> Over the long term, I think the backend should provide that timeout > > >>> value and guarantee that its processing time should not exceed that > > >>> value. > > >> If you mean that the spec should be changed to allow the virtio driver > > >> to be able to program a certain timeout for I2C transactions in the > > >> virtio device, yes, that does sound reasonable. > > > > > > > > >Due to changes in my work, I will pass my virtio-i2c maintenance to > > >Conghui. > > > > > >She may work on this in the future. > > > > > > > I'll try to update the spec first. > > I don't think the spec should be changed for timeout. Timeout-interval > here isn't the property of just the host firmware/kernel, but the > entire setup plays a role here. > > Host have its own timeframe to take care of things (I think HZ should > really be enough for that, since kernel can manage it for busses > normally with just that). Then comes the virtualization, context > switches, guest OS, backend, etc, which add to this delay. All this is > not part of the virtio protocol and so shouldn't be made part of it. The suggested timeout is not meant to take into account the overhead of virtualization, but to be used by the virtio device as a timeout for the transaction on the I2C bus (presumably by programming this value to the physical I2C controller, if one exists). Assume that userspace (or an I2C client driver) asks for a timeout of 20 ms for a particular transfer because it, say, knows that the particular connected I2C peripheral either responds within 10 ms to a particular register read or never responds, so it doesn't want to waste time waiting unnecessarily long for the transfer to complete. If the virtio device end does not have any information on what timeout is required (as in the current spec), it must assume some high value which will never cause I2C transactions to spuriously timeout, say 10 seconds. Even if the virtio driver is fixed to copy and hold all buffers to avoid memory corruption and to time out and return to the caller after the requested 20 ms, the next I2C transfer can not be issued until 10 seconds have passed, since the virtio device end will still be waiting for the hardcoded 10 second timeout and may not respond to new requests until that transfer has timed out. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 03-11-21, 06:18, Chen, Conghui wrote: > >>> Over the long term, I think the backend should provide that timeout > >>> value and guarantee that its processing time should not exceed that > >>> value. > >> If you mean that the spec should be changed to allow the virtio driver > >> to be able to program a certain timeout for I2C transactions in the > >> virtio device, yes, that does sound reasonable. > > > > > >Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui. > > > >She may work on this in the future. > > > > I'll try to update the spec first. I don't think the spec should be changed for timeout. Timeout-interval here isn't the property of just the host firmware/kernel, but the entire setup plays a role here. Host have its own timeframe to take care of things (I think HZ should really be enough for that, since kernel can manage it for busses normally with just that). Then comes the virtualization, context switches, guest OS, backend, etc, which add to this delay. All this is not part of the virtio protocol and so shouldn't be made part of it. -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/29 20:24, Vincent Whitchurch wrote: On Thu, Oct 21, 2021 at 05:30:28AM +0200, Jie Deng wrote: For this moment, we can solve the problem by using a hardcoded big value or disabling the timeout. Is that an Acked-by on this patch which does the latter? Yes, you can add my Acked-by. Let's see if other people still have different opinions. Over the long term, I think the backend should provide that timeout value and guarantee that its processing time should not exceed that value. If you mean that the spec should be changed to allow the virtio driver to be able to program a certain timeout for I2C transactions in the virtio device, yes, that does sound reasonable. Due to changes in my work, I will pass my virtio-i2c maintenance to Conghui. She may work on this in the future. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On Thu, Oct 21, 2021 at 05:30:28AM +0200, Jie Deng wrote: > On 2021/10/20 19:03, Viresh Kumar wrote: > > On 20-10-21, 12:55, Vincent Whitchurch wrote: > >> If the timeout cannot be disabled, then the driver should be fixed to > >> always copy buffers and hold on to them to avoid memory corruption in > >> the case of timeout, as I mentioned in my commit message. That would be > >> quite a substantial change to the driver so it's not something I'm > >> personally comfortable with doing, especially not this late in the -rc > >> cycle, so I'd leave that to others. > > Or we can avoid clearing up and freeing the buffers here until the > > point where the buffers are returned by the host. Until that happens, > > we can avoid taking new requests but return to the earlier caller with > > timeout failure. That would avoid corruption, by freeing buffers > > sooner, and not hanging of the kernel. > > It seems similar to use "wait_for_completion". If the other side is > hacked, the guest may never get the buffers returned by the host, > right ? Note that it is trivial for the host to DoS the guest. All the host has to do is stop responding to I/O requests (I2C or others), then the guest will not be able to perform its intended functions, regardless of whether this particular driver waits forever or not. Even TDX (which Greg mentioned) does not prevent that, see: https://lore.kernel.org/virtualization/?q=tdx+dos > For this moment, we can solve the problem by using a hardcoded big > value or disabling the timeout. Is that an Acked-by on this patch which does the latter? > Over the long term, I think the backend should provide that timeout > value and guarantee that its processing time should not exceed that > value. If you mean that the spec should be changed to allow the virtio driver to be able to program a certain timeout for I2C transactions in the virtio device, yes, that does sound reasonable. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/20 19:03, Viresh Kumar wrote: On 20-10-21, 12:55, Vincent Whitchurch wrote: If the timeout cannot be disabled, then the driver should be fixed to always copy buffers and hold on to them to avoid memory corruption in the case of timeout, as I mentioned in my commit message. That would be quite a substantial change to the driver so it's not something I'm personally comfortable with doing, especially not this late in the -rc cycle, so I'd leave that to others. Or we can avoid clearing up and freeing the buffers here until the point where the buffers are returned by the host. Until that happens, we can avoid taking new requests but return to the earlier caller with timeout failure. That would avoid corruption, by freeing buffers sooner, and not hanging of the kernel. It seems similar to use "wait_for_completion". If the other side is hacked, the guest may never get the buffers returned by the host, right ? For this moment, we can solve the problem by using a hardcoded big value or disabling the timeout. Over the long term, I think the backend should provide that timeout value and guarantee that its processing time should not exceed that value. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 20-10-21, 12:55, Vincent Whitchurch wrote: > If the timeout cannot be disabled, then the driver should be fixed to > always copy buffers and hold on to them to avoid memory corruption in > the case of timeout, as I mentioned in my commit message. That would be > quite a substantial change to the driver so it's not something I'm > personally comfortable with doing, especially not this late in the -rc > cycle, so I'd leave that to others. Or we can avoid clearing up and freeing the buffers here until the point where the buffers are returned by the host. Until that happens, we can avoid taking new requests but return to the earlier caller with timeout failure. That would avoid corruption, by freeing buffers sooner, and not hanging of the kernel. -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On Wed, Oct 20, 2021 at 09:04:45AM +0200, Jie Deng wrote: > On 2021/10/20 14:41, Viresh Kumar wrote: > > On 20-10-21, 14:35, Jie Deng wrote: > >> Yes, but we need to know what's the best value to be configured for a > >> specific "other side". > >> > >> I think the "other side" should be more aware of what value is reasonable > >> to > >> be used. > > If we _really_ need that, then it would require an update to the > > specification first. > > > > I am not sure if the other side is the only party in play here. It > > depends on the entire setup and not just the hardware device. > > Specially with virtualisation things become really slow because of > > context switches, etc. It may be better for the guest userspace to > > decide on the value. > > > > Since this is specially for virtualisation, the kernel may set the > > value to few HZ by default, lets say 10 (Yeah its really high) :). > > I'm OK with this way for the simplicity. That would not be safe. Userspace can change this timeout and the end result with the current implementation of the driver is potentially kernel memory corruption, which is something userspace of course never should be able to trigger. Even if the timeout were hardcoded in the driver and the driver made to ignore what userspace requests, it would need to be set to a ridiculously high value so that we can guarantee that the timeout never ever occurs (since the result is potentially random kernel memory corruption). Which is basically equivalent to disabling the timeout entirely as my patch does. If the timeout cannot be disabled, then the driver should be fixed to always copy buffers and hold on to them to avoid memory corruption in the case of timeout, as I mentioned in my commit message. That would be quite a substantial change to the driver so it's not something I'm personally comfortable with doing, especially not this late in the -rc cycle, so I'd leave that to others. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/20 14:41, Viresh Kumar wrote: On 20-10-21, 14:35, Jie Deng wrote: Yes, but we need to know what's the best value to be configured for a specific "other side". I think the "other side" should be more aware of what value is reasonable to be used. If we _really_ need that, then it would require an update to the specification first. I am not sure if the other side is the only party in play here. It depends on the entire setup and not just the hardware device. Specially with virtualisation things become really slow because of context switches, etc. It may be better for the guest userspace to decide on the value. Since this is specially for virtualisation, the kernel may set the value to few HZ by default, lets say 10 (Yeah its really high) :). I'm OK with this way for the simplicity. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 20-10-21, 14:35, Jie Deng wrote: > Yes, but we need to know what's the best value to be configured for a > specific "other side". > > I think the "other side" should be more aware of what value is reasonable to > be used. If we _really_ need that, then it would require an update to the specification first. I am not sure if the other side is the only party in play here. It depends on the entire setup and not just the hardware device. Specially with virtualisation things become really slow because of context switches, etc. It may be better for the guest userspace to decide on the value. Since this is specially for virtualisation, the kernel may set the value to few HZ by default, lets say 10 (Yeah its really high) :). -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/20 13:36, Greg KH wrote: On Wed, Oct 20, 2021 at 12:20:13PM +0800, Jie Deng wrote: On 2021/10/20 2:14, Wolfram Sang wrote: I think it is set to HZ currently, though I haven't tried big transfers but I still get into some issues with Qemu based stuff. Maybe we can bump it up to few seconds :) If you use adapter->timeout, this can even be set at runtime using a ioctl. So, it can adapt to use cases. Of course, the driver should initialize it to a sane default if the automatic default (HZ) is not suitable. I think a big value may solve most cases. but the driver never know how big is enough by static configuration. Can we make this value to be configurable, just let the other side provide this value ? If an ioctl can change it, that would mean it is configurable, right? Yes, but we need to know what's the best value to be configured for a specific "other side". I think the "other side" should be more aware of what value is reasonable to be used. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On Wed, Oct 20, 2021 at 12:20:13PM +0800, Jie Deng wrote: > > On 2021/10/20 2:14, Wolfram Sang wrote: > > > I think it is set to HZ currently, though I haven't tried big > > > transfers but I still get into some issues with Qemu based stuff. > > > Maybe we can bump it up to few seconds :) > > If you use adapter->timeout, this can even be set at runtime using a > > ioctl. So, it can adapt to use cases. Of course, the driver should > > initialize it to a sane default if the automatic default (HZ) is not > > suitable. > > > I think a big value may solve most cases. but the driver never know how big > is enough by static configuration. > > Can we make this value to be configurable, just let the other side provide > this value ? If an ioctl can change it, that would mean it is configurable, right? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/20 2:14, Wolfram Sang wrote: I think it is set to HZ currently, though I haven't tried big transfers but I still get into some issues with Qemu based stuff. Maybe we can bump it up to few seconds :) If you use adapter->timeout, this can even be set at runtime using a ioctl. So, it can adapt to use cases. Of course, the driver should initialize it to a sane default if the automatic default (HZ) is not suitable. I think a big value may solve most cases. but the driver never know how big is enough by static configuration. Can we make this value to be configurable, just let the other side provide this value ? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 2021/10/19 16:09, Viresh Kumar wrote: Doing this may not be a good thing based on the kernel rules I have understood until now. Maybe Greg and Wolfram can clarify on this. We are waiting here for an external entity (Host kernel) or a firmware that uses virtio for transport. If the other side is hacked, it can make the kernel hang here for ever. I thought that is something that the kernel should never do. I'm also worried about this. We may be able to solve it by setting a big timeout value in the driver. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
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.kuppusw...@linux.intel.com/ > > You can "trust" the hardware, but also handle things when hardware is > broken, which is most often the case in the real world. That's what I was worried about when I got you in, broken or hacked :) > So why is having a timeout a problem here? If you have an overloaded > system, you want things to time out so that you can start to recover. > > And if that hardware stops working? Timeouts are good to have, why not > just bump it up a bit if you are running into it in a real-world > situation? I think it is set to HZ currently, though I haven't tried big transfers but I still get into some issues with Qemu based stuff. Maybe we can bump it up to few seconds :) -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 19-10-21, 13:15, Wolfram Sang wrote: > > > The other side is the software that has access to the _Real_ hardware, > > and so we should trust it. So we can have a actually have a completion > > without timeout here, interesting. > > So, if the other side gets a timeout talking to the HW, then the timeout > error will be propagated? It should be, ideally :) > If so, then we may live with plain wait_for_completion(). -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
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.kuppusw...@linux.intel.com/ You can "trust" the hardware, but also handle things when hardware is broken, which is most often the case in the real world. So why is having a timeout a problem here? If you have an overloaded system, you want things to time out so that you can start to recover. > > Usually we trust the hardware, but if you do not trust the hardware, > > then yes, you need to have a timeout here. > > The other side is the software that has access to the _Real_ hardware, > and so we should trust it. So we can have a actually have a completion > without timeout here, interesting. And if that hardware stops working? Timeouts are good to have, why not just bump it up a bit if you are running into it in a real-world situation? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
On 19-10-21, 11:36, Greg KH wrote: > What is the "other side" here? Is it something that you trust or not? Other side can be a remote processor (for remoteproc over virtio or something similar), or traditionally it can be host OS or host firmware providing virtualisation to a Guest running Linux (this driver). Or something else.. I would incline towards "we trust the other side" here. > Usually we trust the hardware, but if you do not trust the hardware, > then yes, you need to have a timeout here. The other side is the software that has access to the _Real_ hardware, and so we should trust it. So we can have a actually have a completion without timeout here, interesting. -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
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
> > ---
> > drivers/i2c/busses/i2c-virtio.c | 14 +-
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-virtio.c
> > b/drivers/i2c/busses/i2c-virtio.c
> > index f10a603b13fb..7b2474e6876f 100644
> > --- a/drivers/i2c/busses/i2c-virtio.c
> > +++ b/drivers/i2c/busses/i2c-virtio.c
> > @@ -106,11 +106,10 @@ static int virtio_i2c_prepare_reqs(struct virtqueue
> > *vq,
> >
> > static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> > struct virtio_i2c_req *reqs,
> > - struct i2c_msg *msgs, int num,
> > - bool timedout)
> > + struct i2c_msg *msgs, int num)
> > {
> > struct virtio_i2c_req *req;
> > - bool failed = timedout;
> > + bool failed = false;
> > unsigned int len;
> > int i, j = 0;
> >
> > @@ -132,7 +131,7 @@ static int virtio_i2c_complete_reqs(struct virtqueue
> > *vq,
> > j++;
> > }
> >
> > - return timedout ? -ETIMEDOUT : j;
> > + return j;
> > }
> >
> > static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > @@ -141,7 +140,6 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs,
> > struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > struct virtqueue *vq = vi->vq;
> > struct virtio_i2c_req *reqs;
> > - unsigned long time_left;
> > int count;
> >
> > reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > @@ -164,11 +162,9 @@ static int virtio_i2c_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs,
> > reinit_completion(&vi->completion);
> > virtqueue_kick(vq);
> >
> > - time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> > - if (!time_left)
> > - dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> > + wait_for_completion(&vi->completion);
>
> Doing this may not be a good thing based on the kernel rules I have
> understood until now. Maybe Greg and Wolfram can clarify on this.
>
> We are waiting here for an external entity (Host kernel) or a firmware
> that uses virtio for transport. If the other side is hacked, it can
> make the kernel hang here for ever. I thought that is something that
> the kernel should never do.
What is the "other side" here? Is it something that you trust or not?
Usually we trust the hardware, but if you do not trust the hardware,
then yes, you need to have a timeout here.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] i2c: virtio: disable timeout handling
+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
> ---
> 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
