linux-kernel - Re: [PATCH 1/3] iio: light: Add driver for ap3216c

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] [day] [month] [year] [list]

Message-ID: <CAGngYiXg6wPdsXZNmXJ3Z20xRp+cVrysj97gfEHJAxiaL27oNg@mail.gmail.com>
Date:   Wed, 20 Feb 2019 10:09:33 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Jonathan Cameron <jonathan.cameron@...wei.com>,
        Robert Eshleman <bobbyeshleman@...il.com>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/3] iio: light: Add driver for ap3216c

Again thanks for the feedback, Jonathan !

On Wed, Feb 20, 2019 at 7:32 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> Looks to me like that happens (I haven't checked that thoroughly) via
> kernfs_fops_write which takes a mutex - so we have a barrier.
>

Yes, if there's a mutex in the path somewhere (which sysfs/kernfs appears to
provide in this case) then we're ok, as the mutex_unlock() should provide
the barrier which makes the flags visible to the threaded handler.

If that never changes, we are good. As with regmap_bulk_write(), changing the
current behaviour may break many drivers. So it's unlikely to change.

>
> That is a serious - "in theory" circumstance.  The moment we hit any path at
> all that results in a memory barrier it will see it.  Here its not critical
> so we can wait.  In this case this is triggered by a userspace write.

I wish this was an "in theory" circumstance ! I've personally been stung by
this very issue. Code worked great in all tests we could throw at it, but
failed at the customer, of course.

IMHO it's easy for developers to be lulled into a false sense of security.
In many/most cases, there'll be a barrier in the 'invisible' supporting code
somewhere. Or we have to use a lock anyway to protect against concurrent
inconsistent writes, which also implies a barrier. So when developers forget
that these visibility limitations even exist, usually nothing bad will happen.

Until it does, and then we get bitten :)