Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/3] hugetlbfs: close race between MADV_DONTNEED and page fault | From | Rik van Riel <> | Date | Thu, 05 Oct 2023 09:23:05 -0400 |
| |
On Wed, 2023-10-04 at 20:19 -0700, Mike Kravetz wrote: > On 10/03/23 23:25, riel@surriel.com wrote: > > > > @@ -5457,11 +5460,12 @@ void __unmap_hugepage_range_final(struct > > mmu_gather *tlb, > > * someone else. > > */ > > __hugetlb_vma_unlock_write_free(vma); > > - i_mmap_unlock_write(vma->vm_file->f_mapping); > > } else { > > - i_mmap_unlock_write(vma->vm_file->f_mapping); > > hugetlb_vma_unlock_write(vma); > > } > > + > > + if (vma->vm_file) > > + i_mmap_unlock_write(vma->vm_file->f_mapping); > > } > > In the case of a mmap(hugetlbfs_file_mmap) error, the per-vma hugetlb > lock will not be setup. The hugetlb_vma_lock/unlock routines do not > check for this as they were previously always called after the lock > was > set up. So, we can now get:
Wait, the hugetlb_vma_(un)lock_{read,write} functions do have checks for the presence of the lock:
void hugetlb_vma_lock_read(struct vm_area_struct *vma) { if (__vma_shareable_lock(vma)) { struct hugetlb_vma_lock *vma_lock = vma- >vm_private_data;
down_read(&vma_lock->rw_sema); } else if (__vma_private_lock(vma)) { struct resv_map *resv_map = vma_resv_map(vma);
down_read(&resv_map->rw_sema); } }
Both __vma_shareable_lock and __vma_private_lock check that vma->vm_private_data points at something.
Exactly what corner case am I missing here?
What leaves vma->vm_private_data pointing at something invalid?
> > +++ b/mm/hugetlb.c > @@ -5503,10 +5503,12 @@ void __unmap_hugepage_range(struct mmu_gather > *tlb, struct vm_area_struct *vma, > void __hugetlb_zap_begin(struct vm_area_struct *vma, > unsigned long *start, unsigned long *end) > { > + if (!vma->vm_file) /* hugetlbfs_file_mmap error */ > + return; > +
This does not seem quite correct, because the locking is needed to avoid the race between MADV_DONTNEED and the page fault path.
> Another way to resolve would be to fix up the hugetlb_vma_lock/unlock > routines > to check for and handle a null lock.
I thought I had that already.
Does __vma_shareable_lock need to check for !vma->vm_file ?
-- All Rights Reversed.
| |