On Tue, 17 Jul 2018, jiang.bi...@zte.com.cn wrote: > > On 07/15/2018 09:03 PM, Jiang Biao wrote: > >> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c > >> index 4d418e7..be9e5bc 100644 > >> --- a/arch/x86/mm/pti.c > >> +++ b/arch/x86/mm/pti.c > >> @@ -195,8 +195,10 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned > >> long address) > >> static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address) > >> { > >> gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO); > >> - p4d_t *p4d = pti_user_pagetable_walk_p4d(address); > >> pud_t *pud; > >> + p4d_t *p4d = pti_user_pagetable_walk_p4d(address); > >> + if (WARN_ON(!p4d)) > >> + return NULL; > > > > First of all, I don't think we need the (new) warning here. > > pti_user_pagetable_walk_p4d() only returns NULL if you try it on a > > userspace _address_ or the page allocation fails. It already warns on > > the address case. > > > > If you think the allocation path needs a warning, please do it as close > > as possible to the _source_ of the warning (the failed allocation), not > > in the caller. > Hi, > Just taking pti_clone_pmds() as reference, which add warning for NULL > *target_pmd*. The warning does not really matter, I will remove the > *WARN_ON* here and add a warning close to the failed allocation. > Furthermore do you think we need to check _NULL_ return value? > to avoid NULL pointer dereference when the return value is used later, > such as, > p4d_large(*p4d) //p4d is return value of pti_user_pagetable_walk_p4d > *user_p4d = *kernel_p4d;//user_p4d is return value of > pti_user_pagetable_walk_p4d
The Null pointer check makes sense. Thanks, tglx