Re: [PATCH 3/5] powerpc/mm: Fix bugs in huge page hashing

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2010-07-23 at 10:41 +1000, Benjamin Herrenschmidt wrote:
> There's a couple of nasty bugs lurking in our huge page hashing code.
> 
> First, we don't check the access permission atomically with setting
> the _PAGE_BUSY bit, which means that the PTE value we end up using
> for the hashing might be different than the one we have checked
> the access permissions for.
> 
> We've seen cases where that leads us to try to use an invalidated
> PTE for hashing, causing all sort of "interesting" issues.
> 
> Then, we also failed to set _PAGE_DIRTY on a write access.
> 
> This fixes both, while also simplifying the code a bit.

The changeset lacks a comment about a change to the return value
when hitting _PAGE_BUSY and ...

>  	do {
>  		old_pte = pte_val(*ptep);
> -		if (old_pte & _PAGE_BUSY)
> -			goto out;
> +		/* If PTE busy, retry the access */
> +		if (unlikely(old_pte & _PAGE_BUSY))
> +			return 0;
> +		/* If PTE permissions don't match, take page fault */
> +		if (unlikely(access & ~pte_val(*ptep)))
> +			return 1;

old_pte will do just fine instead of reloading it

Thanks Michael !

I'll respin that one.

Cheers,
Ben.

> +		/* Try to lock the PTE, add ACCESSED and DIRTY if it was
> +		 * a write access */
>  		new_pte = old_pte | _PAGE_BUSY | _PAGE_ACCESSED;
> +		if (access & _PAGE_RW)
> +			new_pte |= _PAGE_DIRTY;
>  	} while(old_pte != __cmpxchg_u64((unsigned long *)ptep,
>  					 old_pte, new_pte));
>  
> @@ -127,8 +127,7 @@ repeat:
>  		 */
>  		if (unlikely(slot == -2)) {
>  			*ptep = __pte(old_pte);
> -			err = -1;
> -			goto out;
> +			return -1;
>  		}
>  
>  		new_pte |= (slot << 12) & (_PAGE_F_SECOND | _PAGE_F_GIX);
> @@ -138,9 +137,5 @@ repeat:
>  	 * No need to use ldarx/stdcx here
>  	 */
>  	*ptep = __pte(new_pte & ~_PAGE_BUSY);
> -
> -	err = 0;
> -
> - out:
> -	return err;
> +	return 0;
>  }