linux-kernel - Re: [PATCH] x86/mm: fix return value of p[um]dp_set_access

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: <307c9866-c037-5d87-709f-840bdb577283@intel.com>
Date:   Thu, 19 Sep 2019 10:25:05 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Wei Yang <richardw.yang@...ux.intel.com>,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/mm: fix return value of p[um]dp_set_access_flags

On 9/19/19 1:25 AM, Wei Yang wrote:
> Function p[um]dp_set_access_flags is used with update_mmu_cache_p[um]d
> and the return value from p[um]dp_set_access_flags indicates whether it
> is necessary to do the cache update.

If this change is correct, why was it not applied to
ptep_set_access_flags()?  That function has the same form.

BTW, I rather dislike the 'dirty' variable name.  It seems to be
indicating whether the fault was a write fault or not and whether we
*expect* the dirty bit to be set in 'entry'.

> From current code logic, only when changed && dirty, related page table
> entry would be updated. It is not necessary to update cache when the
> real page table entry is not changed.

This logic doesn't really hold up, though.

If we are only writing accessed and/or dirty bits, then we *never* need
to flush.  The flush might avoid a stale TLB entry causing an extra page
walk by the hardware, but it's probably not ever worth the cost of the
flush.

Unless there's something weird happening with paravirt, I can't ever see
a good reason to flush the TLB when just setting accessed/dirty bits on
bare-metal.

This seems like a place where a debugging check to validate that only
accessed/dirty bits are only being set would be a good idea.