Skip to contentSkip to conversation

mm/damon: add access checking for hugetlb pages

bug report Dan Carpenter 2 participants 2 messages Jan 6, 2022 Memory management

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw

Hi Dan,

Quoted 17 lines · 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
Quoted 4 lines

--> 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.