Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720503

Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch added to -mm tree

Show all headers | View raw

On 08/25/2017 03:51 PM, Nadav Amit wrote:
> 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?

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch  added to -mm tree Nadav Amit <namit@vmware.com> - 2017-08-26 00:10 +0200
  Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch  added to -mm tree Mike Kravetz <mike.kravetz@oracle.com> - 2017-08-26 00:40 +0200
    Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch  added to -mm tree Nadav Amit <namit@vmware.com> - 2017-08-26 01:00 +0200
      Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch  added to -mm tree Mike Kravetz <mike.kravetz@oracle.com> - 2017-08-26 01:50 +0200
        Re: + mm-madvise-fix-freeing-of-locked-page-with-madv_free.patch  added to -mm tree Eric Biggers <ebiggers3@gmail.com> - 2017-08-26 23:10 +0200
          [PATCH] hugetlbfs: change put_page/unlock_page order in hugetlbfs_fallocate() Nadav Amit <namit@vmware.com> - 2017-08-27 04:30 +0200
            Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Mike Kravetz <mike.kravetz@oracle.com> - 2017-08-27 19:20 +0200
              Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Nadav Amit <namit@vmware.com> - 2017-08-27 22:10 +0200
                Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Mike Kravetz <mike.kravetz@oracle.com> - 2017-08-28 19:50 +0200
                Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Michal Hocko <mhocko@kernel.org> - 2017-08-28 20:10 +0200
                Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Mike Kravetz <mike.kravetz@oracle.com> - 2017-08-28 21:00 +0200
            Re: [PATCH] hugetlbfs: change put_page/unlock_page order in  hugetlbfs_fallocate() Michal Hocko <mhocko@kernel.org> - 2017-08-28 15:50 +0200

csiph-web