From: Nadav Amit <namit@vmware.com>
To: Mike Kravetz <mike.kravetz@oracle.com>
CC: "ebiggers@google.com" <ebiggers@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        "Dmitry Vyukov" <dvyukov@google.com>, Hugh Dickins <hughd@google.com>,
        Minchan Kim <minchan@kernel.org>,
        "rientjes@google.com" <rientjes@google.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "mm-commits@vger.kernel.org" <mm-commits@vger.kernel.org>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Michal Hocko <mhocko@kernel.org>,
        "nyc@holomorphy.com" <nyc@holomorphy.com>
Subject: Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch
 added to -mm tree
Thread-Topic: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch
 added to -mm tree
Thread-Index: AQHTHe3sx2Dzh3GK2U6IYLyYXvSwb6KVp+UAgAAFngA=
Date: Fri, 25 Aug 2017 22:51:28 +0000
Message-ID: <10E0D3D9-F7D4-4A0F-AD2F-9E40F3DE6CCC@vmware.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>
In-Reply-To: <3452db57-d847-ec8e-c9be-7710f4ddd5d4@oracle.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <A3A524337E079043B9F619C836C35B0A@namprd05.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Aug 2017 22:51:28.7432
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR05MB967
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v7PMpfm0000332
Content-Length: 2013
Lines: 55

Mike Kravetz <mike.kravetz@oracle.com> wrote:

> On 08/25/2017 03:02 PM, Nadav Amit wrote:
>> Michal Hocko <mhocko@kernel.org> 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 <ebiggers@google.com>
>>>> 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?

Thanks again,
Nadav