linux-kernel - Re: [RFC PATCH 0/7] Introduce cache_is

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: <65b8173160ec8_59028294b3@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Mon, 29 Jan 2024 13:22:57 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Dan Williams
	<dan.j.williams@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Dave
 Jiang" <dave.jiang@...el.com>
CC: <linux-kernel@...r.kernel.org>, Mathieu Desnoyers
	<mathieu.desnoyers@...icios.com>, Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>, <linux-mm@...ck.org>,
	<linux-arch@...r.kernel.org>, Matthew Wilcox <willy@...radead.org>,
	<linux-cxl@...r.kernel.org>, <nvdimm@...ts.linux.dev>
Subject: Re: [RFC PATCH 0/7] Introduce cache_is_aliasing() to fix DAX
 regression

Mathieu Desnoyers wrote:
> This commit introduced in v5.13 prevents building FS_DAX on 32-bit ARM,
> even on ARMv7 which does not have virtually aliased dcaches:
> 
> commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
> 
> It used to work fine before: I have customers using dax over pmem on
> ARMv7, but this regression will likely prevent them from upgrading their
> kernel.
> 
> The root of the issue here is the fact that DAX was never designed to
> handle virtually aliased dcache (VIVT and VIPT with aliased dcache). It
> touches the pages through their linear mapping, which is not consistent
> with the userspace mappings on virtually aliased dcaches. 
> 
> This patch series introduces cache_is_aliasing() with new Kconfig
> options:
> 
>   * ARCH_HAS_CACHE_ALIASING
>   * ARCH_HAS_CACHE_ALIASING_DYNAMIC
> 
> and implements it for all architectures. The "DYNAMIC" implementation
> implements cache_is_aliasing() as a runtime check, which is what is
> needed on architectures like 32-bit ARMV6 and ARMV6K.
> 
> With this we can basically narrow down the list of architectures which
> are unsupported by DAX to those which are really affected.
> 
> Feedback is welcome,

Hi Mathieu, this looks good overall, just some quibbling about the
ordering.

I would introduce dax_is_supported() with the current overly broad
interpretation of "!(ARM || MIPS || SPARC)" using IS_ENABLED(), then
fixup the filesystems to use the new helper, and finally go back and
convert dax_is_supported() to use cache_is_aliasing() internally.

Separately, it is not clear to me why ARCH_HAS_CACHE_ALIASING_DYNAMIC
needs to exist. As long as all paths that care are calling
cache_is_aliasing() then whether it is dynamic or not is something only
the compiler cares about. If those dynamic archs do not want to pay the
text size increase they can always do CONFIG_FS_DAX=n, right?