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: <58972586-68FD-4FD6-8B38-E7CA93D2131E@axentia.se>
Date:   Wed, 27 Jun 2018 06:18:55 +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 June 26, 2018 3:46:07 PM GMT+02:00, Wolfram Sang <wsa@...-dreams.de> wrote:
>> I don't think it's that easy as I just thought about another problem
>> with lifting the locking from the emulation function. It calls
>> kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a
>> very good idea from atomic/irq context...
>
>Right. However, we could simply bail out early when we are in atomic
>context. Simply no DMA then...

Yeah, we could bail out early, and perhaps we should. But we risk regressions, see below...

And that should probably be fixed before we add the unlocked __i2c_smbus_xfer function.

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...

Also, the fact that the buffer is DMA-friendly does not mean that DMA is actually going to be used, so the patch that introduced those allocs might have regressed for this case:
- SMBus user from atomic/irq context
- SMBus emulated
- emulation triggering a GFP_KERNEL alloc
- the existing trylock in i2c_transfer succeeding
- driver not ending up doing DMA

Bailing out would break these users, always. 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?

But as usual, I might be missing something...

Cheers,
Peter