[PATCH] x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd

[PATCH] x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd

[Pan Xinhui]

If pmd or pud is not set, we may set a wrong page mapping level.

Base knowledge:
1) If we have passed pgd_none() check, then current level is at least
PG_LEVEL_1G.
2) If we have passed pud_large() ||!pud_present() check, then current
level is at least PG_LEVEL_2M.

This patch does not change the behavior when a non-NULL value is
returned.

Let's take bask knowledge 2 as an example.
We know *address* belongs to *pud*, however for some reasons *pmd* is
NULL. For example, this address has no physical pages mapped. What we
could benefit from this patch are below:
1) We can walk memory range easier.
If addressA passed to lookup_address(), and NULL returned. We can pass
addressA + level_to_size(level) to lookup_address() in next loop.
...
if (!pte) {
	/* level_to_size has not been implemented in upstream*/
	address += level_to_size(level);
	continue;
}
...
2) keep same behavior because level is set to PG_LEVEL_4K even when pte
is NULL.

So let *level* meaningful even lookup_address_in_pgd return NULL by
moving the assignment before pud/pmd_offset.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
 arch/x86/mm/pageattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 727158c..a887704 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -336,19 +336,19 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,
 	if (pgd_none(*pgd))
 		return NULL;
 
+	*level = PG_LEVEL_1G;
 	pud = pud_offset(pgd, address);
 	if (pud_none(*pud))
 		return NULL;
 
-	*level = PG_LEVEL_1G;
 	if (pud_large(*pud) || !pud_present(*pud))
 		return (pte_t *)pud;
 
+	*level = PG_LEVEL_2M;
 	pmd = pmd_offset(pud, address);
 	if (pmd_none(*pmd))
 		return NULL;
 
-	*level = PG_LEVEL_2M;
 	if (pmd_large(*pmd) || !pmd_present(*pmd))
 		return (pte_t *)pmd;
 
-- 
1.9.1

Re: [PATCH] x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd

[Thomas Gleixner]

On Tue, 14 Jul 2015, Pan Xinhui wrote:
> If pmd or pud is not set, we may set a wrong page mapping level.

No. The behaviour is simply undefined, if the return value of the
function is NULL.

So what you are trying to do is to make the level information accurate
even for the failure case.
 
> We know *address* belongs to *pud*, however for some reasons *pmd* is
> NULL. For example, this address has no physical pages mapped. What we
> could benefit from this patch are below:
> 1) We can walk memory range easier.
> If addressA passed to lookup_address(), and NULL returned. We can pass
> addressA + level_to_size(level) to lookup_address() in next loop.
> ...
> if (!pte) {
> 	/* level_to_size has not been implemented in upstream*/
> 	address += level_to_size(level);
> 	continue;
> }

This example is completely useless because we do not see how the loop
itself looks like and how that improves anything. The proper way to do
this is to post:

     - the patch which changes the function
     - another patch which makes use of the change 

But so far I cannot see any reason why we want to change it.

> ...
> 2) keep same behavior because level is set to PG_LEVEL_4K even when pte
> is NULL.

And what's the actual benefit of #2? Keeping the same behaviour is a
requirement if you don't want to break any users of that function.

Thanks,

	tglx

Re: [PATCH] x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd

[Pan Xinhui]

hi, tglx
	thanks for your reply.

On 2015年07月17日 22:50, Thomas Gleixner wrote:
> On Tue, 14 Jul 2015, Pan Xinhui wrote:
>> If pmd or pud is not set, we may set a wrong page mapping level.
> 
> No. The behaviour is simply undefined, if the return value of the
> function is NULL.
> 
> So what you are trying to do is to make the level information accurate
> even for the failure case.
>  
yes. it's good to report level information. then we can handle some errors.


>> We know *address* belongs to *pud*, however for some reasons *pmd* is
>> NULL. For example, this address has no physical pages mapped. What we
>> could benefit from this patch are below:
>> 1) We can walk memory range easier.
>> If addressA passed to lookup_address(), and NULL returned. We can pass
>> addressA + level_to_size(level) to lookup_address() in next loop.
>> ...
>> if (!pte) {
>> 	/* level_to_size has not been implemented in upstream*/
>> 	address += level_to_size(level);
>> 	continue;
>> }
> 
> This example is completely useless because we do not see how the loop
> itself looks like and how that improves anything. The proper way to do
> this is to post:
> 
>      - the patch which changes the function
>      - another patch which makes use of the change 
> 
> But so far I cannot see any reason why we want to change it.
> 
sorry for that. There are some debug patches protected. I will try to make a simple example in other mails.

>> ...
>> 2) keep same behavior because level is set to PG_LEVEL_4K even when pte
>> is NULL.
> 
> And what's the actual benefit of #2? Keeping the same behaviour is a
> requirement if you don't want to break any users of that function.
> 
agree with you. :)
I did not explain it in correct ways.
When pte is NULL, lookup_address will return NULL on failure. however the level is correct and set to PG_LEVEL_4K.
So what I am trying to do is that if lookup_address return NULL on *pud* or *pmd* NULL failure, level is still correct or more correct.
A correct level information is very useful when we walk a large range of memory.

thanks
xinhui

> Thanks,
> 
> 	tglx
>