[PATCH] x86/mm/pti: remove dead logic during user pagetable population

[PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Jike Song]

Signed-off-by: Jike Song <albcamus@gmail.com>
---
 arch/x86/mm/pti.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 43d4a4a29037..dc611d039bd5 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -164,12 +164,7 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
 		if (!new_p4d_page)
 			return NULL;
 
-		if (pgd_none(*pgd)) {
-			set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page)));
-			new_p4d_page = 0;
-		}
-		if (new_p4d_page)
-			free_page(new_p4d_page);
+		set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page)));
 	}
 	BUILD_BUG_ON(pgd_large(*pgd) != 0);
 
@@ -194,12 +189,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
 		if (!new_pud_page)
 			return NULL;
 
-		if (p4d_none(*p4d)) {
-			set_p4d(p4d, __p4d(_KERNPG_TABLE | __pa(new_pud_page)));
-			new_pud_page = 0;
-		}
-		if (new_pud_page)
-			free_page(new_pud_page);
+		set_p4d(p4d, __p4d(_KERNPG_TABLE | __pa(new_pud_page)));
 	}
 
 	pud = pud_offset(p4d, address);
@@ -213,12 +203,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
 		if (!new_pmd_page)
 			return NULL;
 
-		if (pud_none(*pud)) {
-			set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page)));
-			new_pmd_page = 0;
-		}
-		if (new_pmd_page)
-			free_page(new_pmd_page);
+		set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page)));
 	}
 
 	return pmd_offset(pud, address);
@@ -251,12 +236,7 @@ static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 		if (!new_pte_page)
 			return NULL;
 
-		if (pmd_none(*pmd)) {
-			set_pmd(pmd, __pmd(_KERNPG_TABLE | __pa(new_pte_page)));
-			new_pte_page = 0;
-		}
-		if (new_pte_page)
-			free_page(new_pte_page);
+		set_pmd(pmd, __pmd(_KERNPG_TABLE | __pa(new_pte_page)));
 	}
 
 	pte = pte_offset_kernel(pmd, address);
-- 
2.14.3

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Thomas Gleixner]

On Sun, 7 Jan 2018, Jike Song wrote:

Care to explain why you think this is not needed?

Thanks,

	tglx

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Willy Tarreau]

On Sun, Jan 07, 2018 at 01:50:59AM +0800, Jike Song wrote:
> Signed-off-by: Jike Song <albcamus@gmail.com>

It would be nice to have a commit message, particularly in this quite
sensitive series...

Willy

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Jike Song]

On Sun, Jan 7, 2018 at 3:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 7 Jan 2018, Jike Song wrote:
>
> Care to explain why you think this is not needed?
>

Hi Thomas,

Look at one of the original code snippets:

    162         if (pgd_none(*pgd)) {
    163                 unsigned long new_p4d_page = __get_free_page(gfp);
    164                 if (!new_p4d_page)
    165                         return NULL;
    166
    167                 if (pgd_none(*pgd)) {
    168                         set_pgd(pgd, __pgd(_KERNPG_TABLE |
__pa(new_p4d_page)));
    169                         new_p4d_page = 0;
    170                 }
    171                 if (new_p4d_page)
    172                         free_page(new_p4d_page);
    173         }

Correct me if I'm too dumb to see the rationale here, but to me there
can't be any difference between
two pgd_none(*pgd) of L162 and L167, so it is always false in L171.


> Thanks,
>
>         tglx


-- 
Thanks,
Jike

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Jike Song]

On Sun, Jan 7, 2018 at 4:03 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Sun, Jan 07, 2018 at 01:50:59AM +0800, Jike Song wrote:
>> Signed-off-by: Jike Song <albcamus@gmail.com>
>
> It would be nice to have a commit message, particularly in this quite
> sensitive series...

Yes that's useful, will add it in v2 :)

>
> Willy



-- 
Thanks,
Jike

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Thomas Gleixner]

On Sun, 7 Jan 2018, Jike Song wrote:
> On Sun, Jan 7, 2018 at 3:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Sun, 7 Jan 2018, Jike Song wrote:
> >
> > Care to explain why you think this is not needed?
> >
> 
> Hi Thomas,
> 
> Look at one of the original code snippets:
> 
>     162         if (pgd_none(*pgd)) {
>     163                 unsigned long new_p4d_page = __get_free_page(gfp);
>     164                 if (!new_p4d_page)
>     165                         return NULL;
>     166
>     167                 if (pgd_none(*pgd)) {
>     168                         set_pgd(pgd, __pgd(_KERNPG_TABLE |
> __pa(new_p4d_page)));
>     169                         new_p4d_page = 0;
>     170                 }
>     171                 if (new_p4d_page)
>     172                         free_page(new_p4d_page);
>     173         }
> 
> Correct me if I'm too dumb to see the rationale here, but to me there
> can't be any difference between
> two pgd_none(*pgd) of L162 and L167, so it is always false in L171.

Right, but this kind of explanation wants to be in the changelog. Empty
changelogs for this kind of change are just not acceptable.

Thanks,

	tglx

Re: [PATCH] x86/mm/pti: remove dead logic during user pagetable population

[Jike Song]

On Sun, Jan 7, 2018 at 5:48 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 7 Jan 2018, Jike Song wrote:
>> On Sun, Jan 7, 2018 at 3:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Sun, 7 Jan 2018, Jike Song wrote:
>> >
>> > Care to explain why you think this is not needed?
>> >
>>
>> Hi Thomas,
>>
>> Look at one of the original code snippets:
>>
>>     162         if (pgd_none(*pgd)) {
>>     163                 unsigned long new_p4d_page = __get_free_page(gfp);
>>     164                 if (!new_p4d_page)
>>     165                         return NULL;
>>     166
>>     167                 if (pgd_none(*pgd)) {
>>     168                         set_pgd(pgd, __pgd(_KERNPG_TABLE |
>> __pa(new_p4d_page)));
>>     169                         new_p4d_page = 0;
>>     170                 }
>>     171                 if (new_p4d_page)
>>     172                         free_page(new_p4d_page);
>>     173         }
>>
>> Correct me if I'm too dumb to see the rationale here, but to me there
>> can't be any difference between
>> two pgd_none(*pgd) of L162 and L167, so it is always false in L171.
>
> Right, but this kind of explanation wants to be in the changelog. Empty
> changelogs for this kind of change are just not acceptable.
>

Roger that, just sent v2 out :)

I'm not quite sure but I CCed stable@kernel.org anyway.


> Thanks,
>
>         tglx


-- 
Thanks,
Jike