linux-kernel - Re: [PATCH 3/5] userns: Don't read extents twice in m

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: <20171101141654.fr4rs2m5cygouktb@hirez.programming.kicks-ass.net>
Date:   Wed, 1 Nov 2017 15:16:54 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Christian Brauner <christian.brauner@...onical.com>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Nikolay Borisov <nborisov@...e.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        tycho@...ho.ws, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] userns: Don't read extents twice in m_start

On Wed, Nov 01, 2017 at 03:01:45PM +0100, Christian Brauner wrote:
> Tbf, this isn't solely Eric's fault. I'm to blame here too since I didn't
> document the already existing smb_rmb()s and the new one I introduced when
> writing the patches. I didn't know that there was a hard-set requirement to
> document those. I also didn't see anything in the kernel coding style or the
> memory barriers documentation (But it has been some time since I read those.).

There's too many documents to read.. I'm not sure we changed
coding-style, and I suspect that'll just end up being another bike-shed
in any case.

We did get checkpatch changed though, which is a strong enough clue that
something needs to happen.

But What Nikolay said; memory ordering is hard enough if you're clear on
what exactly you intend to do. But if you later try and reconstruct
without comments, its nearly impossible.

It gets even better if someone changes the ordering requirements over
time and you grow hidden and non-obvious dependencies :/

> > Also, you probably want READ_ONCE() here and WRITE_ONCE() in
> > map_write(), the compiler is free to do unordered byte loads/stores
> > without it.
> > 
> > And finally, did you want to use smp_store_release() and
> > smp_load_acquire() instead?
> 
> Maybe a stupid question but do you suspect this is a real problem in
> this case since you're phrasing it as a question?

Rhetorical question mostly, I suspect its just what you meant to do, as
per the proposed patch.

> Iirc, *_acquire() operations include
> locking operations and might come with a greater performance impact then
> smb_{rmb,wmb}(). Given that this is a very performance critical path we should
> be sure.

No locking what so ever. LOAD-ACQUIRE and STORE-RELEASE are memory ordering
flavours that are paired with the respective memory operation.

It is true that locking ops provide these exact orderings, but that
doesn't imply the reverse.

In short, store-release is a store that ensures all prior load _and_
stores happen-before this store. A load-acquire is a load which
happens-before any subsequent load or stores.

But a release does not constrain later loads or stores, and an acquire
does not constrain prior load or stores.