Messages in this thread | | | Subject | Re: [PATCH v17 08/26] x86/mm: Introduce _PAGE_COW | From | "Yu, Yu-cheng" <> | Date | Fri, 22 Jan 2021 13:54:53 -0800 |
| |
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
| |