[bug report] mm/damon: add access checking for hugetlb pages

[bug report] mm/damon: add access checking for hugetlb pages

[Dan Carpenter]

Hello Baolin Wang,

The patch 86522923bb29: "mm/damon: add access checking for hugetlb
pages" from Dec 30, 2021, leads to the following Smatch static
checker warning:

	mm/damon/vaddr.c:405 damon_hugetlb_mkold()
	warn: 'page' can't be NULL.

mm/damon/vaddr.c
    398 static void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
    399                                 struct vm_area_struct *vma, unsigned long addr)
    400 {
    401         bool referenced = false;
    402         pte_t entry = huge_ptep_get(pte);
    403         struct page *page = pte_page(entry);
    404 
--> 405         if (!page)

I don't think it makes sense to check "page" because we're already dead
if pte_page() starts returning NULL.  Maybe check "entry"?

    406                 return;
    407 
    408         get_page(page);
    409 
    410         if (pte_young(entry)) {
    411                 referenced = true;
    412                 entry = pte_mkold(entry);
    413                 huge_ptep_set_access_flags(vma, addr, pte, entry,
    414                                            vma->vm_flags & VM_WRITE);
    415         }

regards,
dan carpenter

Re: [bug report] mm/damon: add access checking for hugetlb pages

[Baolin Wang]

Hi Dan,

On 1/6/2022 5:12 PM, Dan Carpenter wrote:
> Hello Baolin Wang,
> 
> The patch 86522923bb29: "mm/damon: add access checking for hugetlb
> pages" from Dec 30, 2021, leads to the following Smatch static
> checker warning:
> 
> 	mm/damon/vaddr.c:405 damon_hugetlb_mkold()
> 	warn: 'page' can't be NULL.
> 
> mm/damon/vaddr.c
>      398 static void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
>      399                                 struct vm_area_struct *vma, unsigned long addr)
>      400 {
>      401         bool referenced = false;
>      402         pte_t entry = huge_ptep_get(pte);
>      403         struct page *page = pte_page(entry);
>      404
> --> 405         if (!page)
> 
> I don't think it makes sense to check "page" because we're already dead
> if pte_page() starts returning NULL.  Maybe check "entry"?

Thanks for reporting. Yes, no need to check the 'page' returned from 
pte_page(), and the entry already be checked before to make sure it is a 
valid and present entry. Thus I will send a fix patch to remove the 
redundant page checking. Thanks.