On 1/21/2021 2:32 PM, David Laight wrote:

From: Randy Dunlap

quoted

Sent: 21 January 2021 22:19

On 1/21/21 2:16 PM, David Laight wrote:

quoted

From: Yu, Yu-cheng

quoted

On 1/21/2021 10:44 AM, Borislav Petkov wrote:

quoted

On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:

[...]

quoted

quoted

@@ -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;

quoted

quoted

?

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;

quoted

quoted

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