Re: [GIT pull] x86/pti for 5.4-rc1

[Song Liu <songliubraving@fb.com>]

> On Sep 17, 2019, at 12:01 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Tue, Sep 17, 2019 at 11:49 AM Song Liu <songliubraving@fb.com> wrote:
>> 
>> I guess we need something like the following?
>> 
>> diff --git i/arch/x86/mm/pti.c w/arch/x86/mm/pti.c
>> index b196524759ec..7846916c3bcd 100644
>> --- i/arch/x86/mm/pti.c
>> +++ w/arch/x86/mm/pti.c
>> @@ -306,6 +306,8 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
>> {
>>        unsigned long addr;
>> 
>> +       if (WARN_ON_ONCE(start & ~PAGE_MASK))
>> +               return;
> 
> I don't think we ever care about the low bits of the address below the
> page mask, so this one probably wouldn't make any difference.
> 
> To match the other cases, I'd make it just a plain
> 
>        WARN_ON_ONCE(start & ~PAGE_MASK));
> 
> and leave it at that. The existing commit added the warning, but then
> just made the code work despite it all.  I'd continue that same logic.
> 
>>                if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
>> +                       if (WARN_ON_ONCE(addr & ~PMD_MASK))
>> +                               return;
>>                        target_pmd = pti_user_pagetable_walk_pmd(addr);
>>                        if (WARN_ON(!target_pmd))
>>                                return;
> 
> Again, I think to match the other cases, I'd just do
> 
> -                       addr += PMD_SIZE;
> +                       WARN_ON_ONCE(addr & ~PMD_MASK);
> +                       addr = round_up(addr + 1, PMD_SIZE);
> 
> which now admittedly clones too much, but _does_ clone the requested range.
> 
> But maybe it really doesn't matter, since this condition just
> shouldn't happen in the first place.
> 
> And arguably, the "clone more than requested" issue is true, and maybe
> your "warn and refuse to clone by returning" is the right thing to do.
> 
> So I have very few strong opinions in this area, I just reacted to
> looking at the patch that it didn't seem to cover all the cases.

How about we just do:

diff --git i/arch/x86/mm/pti.c w/arch/x86/mm/pti.c
index b196524759ec..0437f65250db 100644
--- i/arch/x86/mm/pti.c
+++ w/arch/x86/mm/pti.c
@@ -341,6 +341,7 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
                }

                if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
+                       WARN_ON_ONCE(addr & ~PMD_MASK);
                        target_pmd = pti_user_pagetable_walk_pmd(addr);
                        if (WARN_ON(!target_pmd))
                                return;

So it is a "warn and continue" check just for unaligned PMD address. 

Thanks,
Song