linux-kernel - Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus

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]

Message-ID: <4570C1C1-1494-41B8-846F-CB906949C46B@axentia.se>
Date:   Sun, 01 Jul 2018 18:40:06 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Wolfram Sang <wsa@...-dreams.de>
CC:     linux-kernel@...r.kernel.org,
        Vadim Pasternak <vadimp@...lanox.com>,
        Michael Shych <michaelsh@...lanox.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Akinobu Mita <akinobu.mita@...il.com>,
        Jean Delvare <jdelvare@...e.com>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

On July 1, 2018 2:13:20 PM GMT+02:00, Wolfram Sang <wsa@...-dreams.de> wrote:
>On Wed, Jun 27, 2018 at 09:08:18AM +0200, Wolfram Sang wrote:
>> 
>> > Because, thinking more about it, the problem with those allocs are
>not
>> > related to the locking details; adding another trylock to the mix
>just
>> > makes it so much more obvious. I mean, first we would specifically
>> > handle atomic/irq context with a trylock "documenting" that
>atomic/irq
>> > users are welcome to at least try xfers, and then we blattantly
>break
>> > the rulez with a GFP_KERNEL alloc...
>> 
>> Yes, thinking more about it, I came to the conclusion that we should
>not
>> add trylock to SMBus and keep the requirement to allow sleeping.
>> 
>> True, SMBus is not consistent with I2C then, but actually, I'd prefer
>> the consistency the other way around: I wish we had a clear statement
>> that i2c_transfer may sleep. And have a dedicated irqless,
>non-sleeping
>> callback for handling the atomic case instead.
>> 
>> I really don't like the commit which introduced the trylock
>> into i2c_transfer[1]. Its commit message even says: "It is the
>> reponsability of the caller to ensure that the underlying i2c bus
>driver
>> will not sleep either." Which seems broken to me because I can't see
>how
>> the caller should do that? And most bus drivers will sleep. But that
>> commit is upstream for 10 years now, so there are probably users.
>Which
>> also are very hard to spot, I am afraid. I wouldn't see a way to
>convert
>> them off the top of my head.
>> 
>> [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts")
>> 
>> > Currently, I assume they are only broken when the alloc happens to
>> > need to do more than is allowed from the given context. Something
>> > which might or might not be common?
>> 
>> The only regression now would be using smbus_emulated from atomic
>> context. Which is a bug on the caller side because it cannot know if
>> smbus_emulated will be used or not. For the non-emulated case, it
>must
>> not be atomic anyhow.
>> 
>> So, unless I overlooked something, if we decide to not add trylock to
>> smbus_xfer, we are all fine?
>> 
>> And I think we should really keep this clean rule of smbus functions
>> being non-atomic.
>> 
>> D'accord?
>
>So, if no other arguments drop in, I'll apply this series as is next
>week.

Right, I had the below response sitting in my drafts folder.  I thought I had sent it, but apparently I didn't...



Well, IF there are SMBus users that in fact do rely on the emulation allowing calls from atomic/irq context then it will be a regression even if those users are "buggy". But if someone complains, I think the correct response is to open-code the trylock dance and call the new unlocked __i2c_smbus_xfer at the affected location. So, I think we have a contingency plan.

Other than that, we are in violent agreement, and I think you also agree with the above.

I guess that also means the series is fine as-is (modulo the fixes recently made in the tail of the first hunk of patch 1/5 that causes a trivial but annoying conflict when applying it, i.e. below the i2c_transfer -> __i2c_transfer update in the emulation function).

As far as I'm concerned, you can take this whole series directly even if most patches are i2c-mux patches. I don't have anything in my tree yet so I'll simply base any other stuff on this once I can fetch it from you...

Ok?

Cheers,
Peter