Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: missing error return after VM_WARN_ON_ONCE for VMA_PFNMAP --
commit dfcef6542267c02a389d52c15954f3b2b60a0695 Author: Lorenzo Stoakes (ARM) <[email protected]> Subject: mm: add mmap action for discontiguous kernel page mapping This commit introduces discontig_kernel_page_ops to allow drivers to map discontiguous arrays of page pointers into VMAs. It also updates vm_insert_page() and vm_insert_pages() to assert that the mmap write lock is held, converting BUG_ON checks to VM_WARN_ON_ONCE while unifying the code paths. > diff --git a/mm/memory.c b/mm/memory.c > index 448342883e9da..e5c1d1c35bc70 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -2599,27 +2599,34 @@ int vm_insert_pages(struct vm_area_struct *vma, > unsigned long addr, > > if (!range_in_vma(vma, addr, end)) > return -EFAULT; > - if (!(vma->vm_flags & VM_MIXEDMAP)) { > - VM_WARN_ON_ONCE(mmap_read_trylock(vma->vm_mm)); > - VM_WARN_ON_ONCE(vma->vm_flags & VM_PFNMAP); > - vm_flags_set(vma, VM_MIXEDMAP); > + if (!vma_test(vma, VMA_MIXEDMAP_BIT)) { > + mmap_assert_write_locked(vma->vm_mm); > + VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT)); [Severity: High] If this warning triggers, execution will fall through because there is no error return. On production kernels where VM_WARN_ON_ONCE is a no-op, this will silently set VMA_MIXEDMAP_BIT and insert page entries into a PFN-mapped VMA. During VMA teardown, zap_pte_range() will not decrement the reference counts for these pages since vm_normal_page() returns NULL for VMAs with VM_PFNMAP set. Could this result in a permanent memory leak? Should this return an error such as -EINVAL if the bit is set? > + vma_start_write(vma); > + vma_set_flags(vma, VMA_MIXEDMAP_BIT); > } > /* Defer page refcount checking till we're about to map that page. */ > return insert_pages(vma, addr, pages, num, vma->vm_page_prot); > } [ ... ] > +static void __map_kernel_pages_prepare(struct vm_area_desc *desc) > +{ > + if (vma_desc_test(desc, VMA_MIXEDMAP_BIT)) > + return; > + > + mmap_assert_write_locked(desc->mm); > + VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT)); [Severity: High] Similar to the check in vm_insert_pages(), this warns but does not abort the operation if the VMA has the PFNMAP bit set. Should this function be changed to return an int so it can propagate an error and prevent inserting refcounted pages into a PFN-mapped VMA? > + vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT); > +} [ ... ] > @@ -2675,10 +2774,11 @@ int vm_insert_page(struct vm_area_struct *vma, > unsigned long addr, > { > if (addr < vma->vm_start || addr >= vma->vm_end) > return -EFAULT; > - if (!(vma->vm_flags & VM_MIXEDMAP)) { > - BUG_ON(mmap_read_trylock(vma->vm_mm)); > - BUG_ON(vma->vm_flags & VM_PFNMAP); > - vm_flags_set(vma, VM_MIXEDMAP); > + if (!vma_test(vma, VMA_MIXEDMAP_BIT)) { > + mmap_assert_write_locked(vma->vm_mm); > + VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT)); [Severity: High] This replaces a strict BUG_ON() with a warning but allows execution to continue. Does this need an early return here as well to prevent a potential page reference leak? > + vma_start_write(vma); > + vma_set_flags(vma, VMA_MIXEDMAP_BIT); > } > return insert_page(vma, addr, page, vma->vm_page_prot, false); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261009-b4-mmap-prepare-vma-flag-sanify-v5-0-923b70125...@kernel.org?part=7
