Re: [PATCH v17 08/26] x86/mm: Introduce _PAGE_COW

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/21/2021 2:32 PM, David Laight wrote:
From: Randy Dunlap
Sent: 21 January 2021 22:19

On 1/21/21 2:16 PM, David Laight wrote:
From: Yu, Yu-cheng

On 1/21/2021 10:44 AM, Borislav Petkov wrote:
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte)

   static inline pte_t pte_wrprotect(pte_t pte)
   {
+	/*
+	 * Blindly clearing _PAGE_RW might accidentally create
+	 * a shadow stack PTE (RW=0, Dirty=1).  Move the hardware
+	 * dirty value to the software bit.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
+		pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;

Why the unreadable shifting when you can simply do:

                  if (pte.pte & _PAGE_DIRTY)
                          pte.pte |= _PAGE_COW;


?

It clears _PAGE_DIRTY and sets _PAGE_COW.  That is,

if (pte.pte & _PAGE_DIRTY) {
	pte.pte &= ~_PAGE_DIRTY;
	pte.pte |= _PAGE_COW;
}

So, shifting makes resulting code more efficient.

Does the compiler manage to do one shift?

How can it clear anything?

It could shift it off either end since there are both << and >>.

It is still:
	pte.pte |= xxxxxxx;

There is only an |= against the target.

Something horrid with ^= might set and clear.

It could be 4 instructions:
	is_dirty = pte.pte & PAGE_DIRTY;
	pte.pte &= ~PAGE_DIRTY; // or pte.pte ^= is_dirty
	is_cow = is_dirty << (BIT_COW - BIT_DIRTY); // or equivalent >>
	pte.pte |= is_cow;
provided you've a three operand form for one of the first two instructions.
Something like ARM might manage to merge the last two as well.
But the register dependency chain length may matter more than
the number of instructions.
The above is likely to be three long.

I see what you are saying.  The patch is like...

	if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
		pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
		pte = pte_clear_flags(pte, _PAGE_DIRTY);
	}

It is not necessary to do the shifting.  I will make it, simply,

if (pte.pte & _PAGE_DIRTY) {
	pte.pte &= ~PAGE_DIRTY;
	pte.pte |= _PAGE_COW;
}

Thanks for your comments.

--
Yu-cheng



[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux