From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752472AbdK2DWz (ORCPT ); Tue, 28 Nov 2017 22:22:55 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:26881 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751368AbdK2DWy (ORCPT ); Tue, 28 Nov 2017 22:22:54 -0500 Subject: Re: [PATCH] hugetlbfs: change put_page/unlock_page order in hugetlbfs_fallocate() To: Eric Biggers , Nadav Amit Cc: Nadia Yvette Chambers , linux-kernel@vger.kernel.org, Nadav Amit , Michal Hocko , Andrew Morton References: <20170826210905.GA21712@zzz.localdomain> <20170826191124.51642-1-namit@vmware.com> <20171129023747.GB24001@zzz.localdomain> From: Mike Kravetz Message-ID: <856db20b-79bb-c584-63a6-19df24ac2d95@oracle.com> Date: Tue, 28 Nov 2017 19:22:42 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171129023747.GB24001@zzz.localdomain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [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 >> cc: Mike Kravetz >> >> Signed-off-by: Nadav Amit >> --- >> 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 Acked-by: Michal Hocko 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