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

Powered by blists - more mailing lists