linux-kernel - Re: [PATCH] mm: Fix force_flush behavior in zap_pte

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:	Sat, 3 May 2014 16:57:27 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Richard Weinberger <richard@....at>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-mm <linux-mm@...ck.org>, Dave Jones <davej@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Sasha Levin <sasha.levin@...cle.com>,
	Hugh Dickins <hughd@...gle.com>,
	Toralf Förster <toralf.foerster@....de>
Subject: Re: [PATCH] mm: Fix force_flush behavior in zap_pte_range()

On Sat, May 3, 2014 at 4:37 PM, Richard Weinberger <richard@....at> wrote:
> Commit 1cf35d47 (mm: split 'tlb_flush_mmu()' into tlb flushing and memory freeing parts)
> accidently changed the behavior of the force_flush variable.

No it didn't. There was nothing accidental about it, and it doesn't
even change it the way you claim.

> Before the patch it was set by __tlb_remove_page(). Now it is only set to 1
> if __tlb_remove_page() returns false but never set back to 0 if __tlb_remove_page()
> returns true.

It starts out as zero. If __tlb_remove_page() returns true, it never
gets set to anything *but* zero, except by the dirty shared mapping
case that *needs* to set it to non-zero, exactly because it *needs* to
flush the TLB before releasing the pte lock.

Which was the whole point of the patch.

Your explanation makes no sense for _another_ reason: even with your
patch, it never gets set back to zero, since if it gets set to one you
have that "break" in there. So the whole "gets set back to zero" is
simply not relevant or true, with or with the patch.

The only place it actually gets zeroed (apart from initialization) is
for the "goto again" case, which does it (and always did it)

> Fixes BUG: Bad rss-counter state ...
> and
> kernel BUG at mm/filemap.c:202!

So tell us more about those actual problems, because your patch and
explanation is clearly wrong.

What hardware, what load, what "kernel BUG at filemap.c:202"?

The shared dirty fix may certainly be exposing some other issue, but
the only report I have seen about filemap.c:202 was reported by Dave
Jones ten *days* before the commit you talk about was even done.

So this whole thing makes no sense what-so-ever.

              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/