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: <f84c48ec-2963-4754-9b6a-8eb0c473d7d0@efficios.com>
Date: Tue, 30 Jan 2024 10:14:48 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: 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, 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,
 Linux-Fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC PATCH 0/7] Introduce cache_is_aliasing() to fix DAX
 regression

On 2024-01-29 16:22, Dan Williams wrote:
> 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.

Thanks for having a look !

> 
> 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.

Will do.

> 
> 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?

Good point. It will help reduce complexity and improve test coverage.

I also intend to rename "cache_is_aliasing()" to "dcache_is_aliasing()",
so if we introduce an "icache_is_aliasing()" in the future, it won't be
confusing. Having aliasing icache-dcache but not dcache-dcache seems to
be fairly common.

So basically:

If an arch selects ARCH_HAS_CACHE_ALIASING, it implements
dcache_is_aliasing() (for now), and eventually we can implement
icache_is_aliasing() as well.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com