lkml.org 
[lkml]   [2021]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v17 08/26] x86/mm: Introduce _PAGE_COW
From
Date
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

\
 
 \ /
  Last update: 2021-01-22 23:01    [W:0.203 / U:1.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site