From: Viresh Kumar <viresh.kumar@linaro.org> To: Vincent Whitchurch <vincent.whitchurch@axis.com>, gregkh@linuxfoundation.org Cc: wsa@kernel.org, jie.deng@intel.com, virtualization@lists.linux-foundation.org, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@axis.com Subject: Re: [PATCH 1/2] i2c: virtio: disable timeout handling Date: Tue, 19 Oct 2021 13:39:13 +0530 [thread overview] Message-ID: <20211019080913.oajrvr2msz5enzvz@vireshk-i7> (raw) In-Reply-To: <20211019074647.19061-2-vincent.whitchurch@axis.com> +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
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Vincent Whitchurch <vincent.whitchurch@axis.com>, gregkh@linuxfoundation.org Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wsa@kernel.org, kernel@axis.com, linux-i2c@vger.kernel.org Subject: Re: [PATCH 1/2] i2c: virtio: disable timeout handling Date: Tue, 19 Oct 2021 13:39:13 +0530 [thread overview] Message-ID: <20211019080913.oajrvr2msz5enzvz@vireshk-i7> (raw) In-Reply-To: <20211019074647.19061-2-vincent.whitchurch@axis.com> +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
next prev parent reply other threads:[~2021-10-19 8:09 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 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 7:46 ` [PATCH 1/2] i2c: virtio: disable timeout handling Vincent Whitchurch 2021-10-19 7:46 ` Vincent Whitchurch 2021-10-19 8:09 ` Viresh Kumar [this message] 2021-10-19 8:09 ` Viresh Kumar 2021-10-19 9:36 ` Greg KH 2021-10-19 9:36 ` Greg KH 2021-10-19 9:42 ` Viresh Kumar 2021-10-19 9:42 ` Viresh Kumar 2021-10-19 11:15 ` Wolfram Sang 2021-10-19 14:14 ` Viresh Kumar 2021-10-19 14:14 ` Viresh Kumar 2021-10-19 11:16 ` Greg KH 2021-10-19 11:16 ` Greg KH 2021-10-19 14:37 ` Viresh Kumar 2021-10-19 14:37 ` Viresh Kumar 2021-10-19 18:14 ` Wolfram Sang 2021-10-20 4:20 ` Jie Deng 2021-10-20 4:20 ` Jie Deng 2021-10-20 5:36 ` Greg KH 2021-10-20 5:36 ` Greg KH 2021-10-20 6:35 ` Jie Deng 2021-10-20 6:35 ` Jie Deng 2021-10-20 6:41 ` Viresh Kumar 2021-10-20 6:41 ` Viresh Kumar 2021-10-20 7:04 ` Jie Deng 2021-10-20 7:04 ` Jie Deng 2021-10-20 10:55 ` Vincent Whitchurch 2021-10-20 10:55 ` Vincent Whitchurch 2021-10-20 11:03 ` Viresh Kumar 2021-10-20 11:03 ` Viresh Kumar 2021-10-21 3:30 ` Jie Deng 2021-10-21 3:30 ` Jie Deng 2021-10-29 12:24 ` Vincent Whitchurch 2021-10-29 12:24 ` Vincent Whitchurch 2021-11-01 5:23 ` Jie Deng 2021-11-01 5:23 ` Jie Deng 2021-11-03 6:18 ` Chen, Conghui 2021-11-03 6:37 ` Viresh Kumar 2021-11-03 6:37 ` Viresh Kumar 2021-11-03 14:42 ` Vincent Whitchurch 2021-11-03 14:42 ` Vincent Whitchurch 2021-11-09 4:52 ` Viresh Kumar 2021-11-09 4:52 ` Viresh Kumar 2021-10-20 3:36 ` Jie Deng 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 7:46 ` Vincent Whitchurch 2021-10-19 8:22 ` Viresh Kumar 2021-10-19 8:22 ` Viresh Kumar 2021-10-20 8:54 ` Jie Deng 2021-10-20 8:54 ` Jie Deng 2021-10-20 9:17 ` Viresh Kumar 2021-10-20 9:17 ` Viresh Kumar 2021-10-20 10:38 ` Vincent Whitchurch 2021-10-20 10:38 ` Vincent Whitchurch 2021-10-20 10:47 ` Viresh Kumar 2021-10-20 10:47 ` Viresh Kumar 2021-10-29 11:54 ` Vincent Whitchurch 2021-10-29 11:54 ` Vincent Whitchurch 2021-10-21 5:55 ` Jie Deng 2021-10-21 5:55 ` Jie Deng 2021-10-21 5:58 ` Viresh Kumar 2021-10-21 5:58 ` Viresh Kumar 2021-11-02 4:32 ` Viresh Kumar 2021-11-02 4:32 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211019080913.oajrvr2msz5enzvz@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=gregkh@linuxfoundation.org \ --cc=jie.deng@intel.com \ --cc=kernel@axis.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=vincent.whitchurch@axis.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=wsa@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.