linux-kernel - Re: v4.2-rc dcache regression, probably 75a6f82a0d10

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:	Fri, 31 Jul 2015 15:52:38 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Dominique Martinet <asmadeus@...ewreck.org>
Cc:	Hugh Dickins <hughd@...gle.com>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	Dominique Martinet <dominique.martinet@....fr>,
	Al Viro <viro@...iv.linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	David Howells <dhowells@...hat.com>
Subject: Re: v4.2-rc dcache regression, probably 75a6f82a0d10

On Fri, Jul 31, 2015 at 1:50 PM, Dominique Martinet
<asmadeus@...ewreck.org> wrote:
>
> It's probably an old race that was very hard to hit because of cache
> coherency.
> Basically, before the wmb/rmb, the dentry was always updated closely to
> its flags, so the other CPU would "usually" get both updates at the same
> time; the barriers make it so the updates are split and it's possible to
> get it, and would explain why I could pick 4bf46a2726 as "the one"

I doubt that's it. wmb/rmb is actually a no-op on x86, and only
affects instruction scheduling. So yes, timing could be affected, but
it's such a _tiny_ effect that I don't really believe it's an issue.

I'd be more suspicious about other effects. For example, iot's not at
all obvious that the commit in question just changes the order of the
flags/inode field accesses, there are potentialy bigger changes there.
For example, this part (in __d_obtain_alias):

-       tmp->d_inode = inode;
-       tmp->d_flags |= add_flags;
+       __d_set_inode_and_type(tmp, inode, add_flags);

looks a bit off, because it *used* to just add those flags, but now,
through __d_set_inode_and_type, it does

+       dentry->d_inode = inode;
+       smp_wmb();
+       flags = READ_ONCE(dentry->d_flags);
+       flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
+       flags |= type_flags;
+       WRITE_ONCE(dentry->d_flags, flags);

so it clears DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU.

Is that correct? Maybe, I haven't checked. And maybe it's a big bad
bug. Regardless, it sure as hell isn't just changing the order of the
access to those fields. That "DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU"
clearing came from __d_instantiate(), but now it hits __d_obtain_alias
too.

There may be other changes like that for all I know. I didn't look
more closely.  But I'd blame changes like that rather than any timing
by rmb/wmb.

(Side note: the instruction scheduling changes can be big - especially
together with the changes to use READ_ONCE/WRITE_ONCE, it basically
means that gcc won't be using r-m-w instructions etc to set flags, so
I'm not saying rmb/wmb is necessarily a no-op in general, but it's
definitely generally not a hugely noticeable thing).

Added DavidH to the cc list, since it's his commit. But Dominique, it
would probably be a good idea for you to try to double-check that that
commit really is what matters for you and causes problems.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/