Re: powerpc/lpar: Don't crash on H_PROTECT errors

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 03/01/2016 04:31 AM, Michael Ellerman wrote:
> On Mon, 2016-29-02 at 11:52:32 UTC, Anshuman Khandual wrote:
>> There are certain condition in which H_PROTECT can return error code
>> other than H_NOT_FOUND and H_SUCCESS. One such being an attempt to
>> update an hpte owned by adjunct partition. Return 0 in that case so
>> that user space will retry the access. In adjunct case this mean we
>> will not make much progress in the user space. But atleast we get a
>> chance to kill the task and avoid taking down the entire box.
> 
> Why is it OK to do nothing and return 0?
> 
> The function's contract is that it either does the update or returns -1, you

Right, the semantics of the function will change with it. The callers of
the function include these places.

(1) __hash_page_4K   (arch/powerpc/mm/hash64_4k.c)
(2) __hash_page_4K   (arch/powerpc/mm/hash64_64k.c)
(3) __hash_page_64K  (arch/powerpc/mm/hash64_64k.c)
(4) __hash_page_huge (arch/powerpc/mm/hugetlbpage-hash64.c)
(5) __hash_page_thp  (arch/powerpc/mm/hugepage-hash64.c)

All of them get called from hash_page_mm (in turn from hash_page)
which is triggered from page fault interrupt (0x300 or 0x400). A
return value of 0 from these individual HPTE management functions
will return 0 from hash_page_mm as well. This returns the control
back to the user space/kernel which would have caused the page
fault but without actually setting the right HPTE. This will cause
fault again. The semantics change does not affect these callers
in any bad way but fakes a correct page fault handling if I am not
missing anything.

But only __hash_page_4K and __hash_page_64K functions get called
from hash_preload (in turn from update_mmu_cache) which is called
from a lot of code path including linux page fault handling path
through handle_pte_fault. Inside hash_preload, the return value
from __hash_page_4K(|64K) function is checked only for -1, detecting
which hash_failure_debug is called. The return value is not passed
up in the call chain. With H_RESOURCE, hash_preload will pretend as
if it did what it was suppose to do and but none of it's callers will
explicitly know about it other than the WARN_RATELIMIT message. I
believed that hash_preload is a best attempt approach and anything
missed here will be corrected with future page faults when the process
starts executing. Please correct me if I am wrong.

> can't change that without auditing all callers - and describing that in the
> change log.

Did not get it, you would like to have something like the above code
path description in the change log ?