linux-kernel - Re: [PATCH 1/2] i2c: virtio: disable timeout handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Nov 2021 15:42:41 +0100
From:   Vincent Whitchurch <vincent.whitchurch@...s.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
CC:     "Chen, Conghui" <conghui.chen@...el.com>,
        "Deng, Jie" <jie.deng@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Wolfram Sang <wsa@...nel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kernel <kernel@...s.com>
Subject: 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.