Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935544AbdHYXlw (ORCPT ); Fri, 25 Aug 2017 19:41:52 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:24531 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933238AbdHYXlu (ORCPT ); Fri, 25 Aug 2017 19:41:50 -0400 Subject: Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch added to -mm tree To: Nadav Amit Cc: "ebiggers@google.com" , Andrew Morton , Andrea Arcangeli , Dmitry Vyukov , Hugh Dickins , Minchan Kim , "rientjes@google.com" , "stable@vger.kernel.org" , "mm-commits@vger.kernel.org" , "open list:MEMORY MANAGEMENT" , Linux Kernel Mailing List , Michal Hocko , "nyc@holomorphy.com" References: <599df681.NreP1dR3/HGSfpCe%akpm@linux-foundation.org> <20170824060957.GA29811@dhcp22.suse.cz> <81C11D6F-653D-4B14-A3A6-E6BB6FB5436D@vmware.com> <3452db57-d847-ec8e-c9be-7710f4ddd5d4@oracle.com> <10E0D3D9-F7D4-4A0F-AD2F-9E40F3DE6CCC@vmware.com> From: Mike Kravetz Message-ID: Date: Fri, 25 Aug 2017 16:41:36 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <10E0D3D9-F7D4-4A0F-AD2F-9E40F3DE6CCC@vmware.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2432 Lines: 61 On 08/25/2017 03:51 PM, Nadav Amit wrote: > Mike Kravetz wrote: > >> On 08/25/2017 03:02 PM, Nadav Amit wrote: >>> Michal Hocko wrote: >>> >>>> Hmm, I do not see this neither in linux-mm nor LKML. Strange >>>> >>>> On Wed 23-08-17 14:41:21, Andrew Morton wrote: >>>>> From: Eric Biggers >>>>> Subject: mm/madvise.c: fix freeing of locked page with MADV_FREE >>>>> >>>>> If madvise(..., MADV_FREE) split a transparent hugepage, it called >>>>> put_page() before unlock_page(). This was wrong because put_page() can >>>>> free the page, e.g. if a concurrent madvise(..., MADV_DONTNEED) has >>>>> removed it from the memory mapping. put_page() then rightfully complained >>>>> about freeing a locked page. >>>>> >>>>> Fix this by moving the unlock_page() before put_page(). >>> >>> Quick grep shows that a similar flow (put_page() followed by an >>> unlock_page() ) also happens in hugetlbfs_fallocate(). Isn’t it a problem as >>> well? >> >> I assume you are asking about this block of code? > > Yes. > >> >> /* >> * page_put due to reference from alloc_huge_page() >> * unlock_page because locked by add_to_page_cache() >> */ >> put_page(page); >> unlock_page(page); >> >> Well, there is a typo (page_put) in the comment. :( >> >> However, in this case we have just added the huge page to a hugetlbfs >> file. The put_page() is there just to drop the reference count on the >> page (taken when allocated). It will still be non-zero as we have >> successfully added it to the page cache. So, we are not freeing the >> page here, just dropping the reference count. >> >> This should not cause a problem like that seen in madvise. > > Thanks for the quick response. > > I am not too familiar with this piece of code, so just for the matter of > understanding: what prevents the page from being removed from the page cache > shortly after it is added (even if it is highly unlikely)? The page lock? The > inode lock? Someone would need to acquire the inode lock to remove the page. This is held until we exit the routine. Also note that put_page for this type of huge page almost always results in the page being put back on a free list within the hugetlb(fs) subsystem. It is not returned to the 'normal' memory allocators for general use. -- Mike Kravetz