Messages in this thread | | | Subject | Re: [PATCH] hugetlbfs: change put_page/unlock_page order in hugetlbfs_fallocate() | From | Mike Kravetz <> | Date | Tue, 28 Nov 2017 19:22:42 -0800 |
| |
[CC Andrew, Michal]
On 11/28/2017 06:37 PM, Eric Biggers wrote: > On Sat, Aug 26, 2017 at 12:11:24PM -0700, Nadav Amit wrote: >> hugetlfs_fallocate() currently performs put_page() before unlock_page(). >> This scenario opens a small time window, from the time the page is added >> to the page cache, until it is unlocked, in which the page might be >> removed from the page-cache by another core. If the page is removed >> during this time windows, it might cause a memory corruption, as the >> wrong page will be unlocked. >> >> It is arguable whether this scenario can happen in a real system, and >> there are several mitigating factors. The issue was found by code >> inspection (actually grep), and not by actually triggering the flow. >> Yet, since putting the page before unlocking is incorrect it should be >> fixed, if only to prevent future breakage or someone copy-pasting this >> code. >> >> Fixes: 70c3547e36f5c ("hugetlbfs: add hugetlbfs_fallocate()") >> >> cc: Eric Biggers <ebiggers3@gmail.com> >> cc: Mike Kravetz <mike.kravetz@oracle.com> >> >> Signed-off-by: Nadav Amit <namit@vmware.com> >> --- >> fs/hugetlbfs/inode.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >> index 28d2753be094..9475fee79cee 100644 >> --- a/fs/hugetlbfs/inode.c >> +++ b/fs/hugetlbfs/inode.c >> @@ -655,11 +655,11 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, >> mutex_unlock(&hugetlb_fault_mutex_table[hash]); >> >> /* >> - * page_put due to reference from alloc_huge_page() >> * unlock_page because locked by add_to_page_cache() >> + * page_put due to reference from alloc_huge_page() >> */ >> - put_page(page); >> unlock_page(page); >> + put_page(page); >> } >> >> if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size) >> -- > > This patch wasn't ever applied. Nadia, do you take patches for hugetlbfs, or > does this need to go through Andrew Morton? > > Eric
Nadia has not been active for some time on hugetlbfs, so best to go through Andrew. Added Andrew and Michal on CC.
This patch has a: Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Acked-by: Michal Hocko <mhocko@suse.com>
I am still of the opinion that this does not need to be sent to stable. Although the ordering is current code is incorrect, there is no way for this to be a problem with current locking. In addition, I verified that the perhaps bigger issue with sys_fadvise64(POSIX_FADV_DONTNEED) for hugetlbfs and other filesystems is addressed in commit 3a77d214807c. -- Mike Kravetz
| |