Transparent huge pages can change page->flags (PG_compound_lock)
without taking Slab lock. Since THP can not break slab pages we can
safely access compound page without taking compound lock.
Specificly this patch fixes race between compound_unlock and slab
functions which does page-flags update. This can occur when
get_page/put_page is called on page from slab object.
Reported-by: Amey Bhide <[email protected]>
Signed-off-by: Pravin B Shelar <[email protected]>
---
include/linux/mm.h | 2 ++
mm/swap.c | 17 +++++++++++++++++
2 files changed, 19 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 74aa71b..82f86e6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -321,6 +321,7 @@ static inline int is_vmalloc_or_module_addr(const void *x)
static inline void compound_lock(struct page *page)
{
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ VM_BUG_ON(PageSlab(page));
bit_spin_lock(PG_compound_lock, &page->flags);
#endif
}
@@ -328,6 +329,7 @@ static inline void compound_lock(struct page *page)
static inline void compound_unlock(struct page *page)
{
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ VM_BUG_ON(PageSlab(page));
bit_spin_unlock(PG_compound_lock, &page->flags);
#endif
}
diff --git a/mm/swap.c b/mm/swap.c
index 8ff73d8..d4eb9f6 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -82,6 +82,16 @@ static void put_compound_page(struct page *page)
if (likely(page != page_head &&
get_page_unless_zero(page_head))) {
unsigned long flags;
+
+ if (PageSlab(page_head)) {
+ /* THP can not break up slab pages, avoid
+ * taking compound_lock(). */
+ if (put_page_testzero(page_head))
+ VM_BUG_ON(1);
+
+ atomic_dec(&page->_mapcount);
+ goto skip_lock;
+ }
/*
* page_head wasn't a dangling pointer but it
* may not be a head page anymore by the time
@@ -115,6 +125,8 @@ static void put_compound_page(struct page *page)
VM_BUG_ON(atomic_read(&page_head->_count) <= 0);
VM_BUG_ON(atomic_read(&page->_count) != 0);
compound_unlock_irqrestore(page_head, flags);
+
+ skip_lock:
if (put_page_testzero(page_head)) {
if (PageHead(page_head))
__put_compound_page(page_head);
@@ -168,6 +180,11 @@ bool __get_page_tail(struct page *page)
* we obtain the lock. That is ok as long as it
* can't be freed from under us.
*/
+ if (PageSlab(page_head)) {
+ __get_page_tail_foll(page, false);
+ return true;
+ }
+
flags = compound_lock_irqsave(page_head);
/* here __split_huge_page_refcount won't run anymore */
if (likely(PageTail(page))) {
--
1.7.10
On Mon, 14 May 2012, Pravin B Shelar wrote:
> Transparent huge pages can change page->flags (PG_compound_lock)
> without taking Slab lock. Since THP can not break slab pages we can
> safely access compound page without taking compound lock.
>
> Specificly this patch fixes race between compound_unlock and slab
> functions which does page-flags update. This can occur when
> get_page/put_page is called on page from slab object.
You need to also get this revbiewed by the THP folks like Andrea &
friends.
Reviewed-by: Christoph Lameter <[email protected]>
On Mon, May 14, 2012 at 11:53 AM, Christoph Lameter <[email protected]> wrote:
> On Mon, 14 May 2012, Pravin B Shelar wrote:
>
>> Transparent huge pages can change page->flags (PG_compound_lock)
>> without taking Slab lock. Since THP can not break slab pages we can
>> safely access compound page without taking compound lock.
>>
>> Specificly this patch fixes race between compound_unlock and slab
>> functions which does page-flags update. This can occur when
>> get_page/put_page is called on page from slab object.
>
> You need to also get this revbiewed by the THP folks like Andrea &
> friends.
Hi Andrea,
Can you comment on this patch.
Thanks.
>
> Reviewed-by: Christoph Lameter <[email protected]>
Hi Pravin,
On Mon, May 14, 2012 at 11:41:17AM -0700, Pravin B Shelar wrote:
> Transparent huge pages can change page->flags (PG_compound_lock)
> without taking Slab lock. Since THP can not break slab pages we can
> safely access compound page without taking compound lock.
>
> Specificly this patch fixes race between compound_unlock and slab
> functions which does page-flags update. This can occur when
> get_page/put_page is called on page from slab object.
DMA on slab running put_page concurrently with kmem_cache_free/kfree
was unexpected. Is this the scenario where the race happens, right?
> diff --git a/mm/swap.c b/mm/swap.c
> index 8ff73d8..d4eb9f6 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -82,6 +82,16 @@ static void put_compound_page(struct page *page)
> if (likely(page != page_head &&
> get_page_unless_zero(page_head))) {
> unsigned long flags;
> +
> + if (PageSlab(page_head)) {
> + /* THP can not break up slab pages, avoid
> + * taking compound_lock(). */
> + if (put_page_testzero(page_head))
> + VM_BUG_ON(1);
> +
> + atomic_dec(&page->_mapcount);
> + goto skip_lock;
> + }
If a THP is splitted before get_page_unless_zero runs, the head page
may be then freed and reallocated as slab. The "page" then should not
be freed as a tail page anymore, because it's not a tail page. The
head just accidentally become a slab (maybe not even a compound slab).
To avoid such scenario this should be enough:
if (PageSlab(page_head) && PageTail(page)) {
...
}
Thanks,
Andrea
On Wed, May 16, 2012 at 5:44 PM, Andrea Arcangeli <[email protected]> wrote:
> Hi Pravin,
>
> On Mon, May 14, 2012 at 11:41:17AM -0700, Pravin B Shelar wrote:
>> Transparent huge pages can change page->flags (PG_compound_lock)
>> without taking Slab lock. Since THP can not break slab pages we can
>> safely access compound page without taking compound lock.
>>
>> Specificly this patch fixes race between compound_unlock and slab
>> functions which does page-flags update. This can occur when
>> get_page/put_page is called on page from slab object.
>
> DMA on slab running put_page concurrently with kmem_cache_free/kfree
> was unexpected. Is this the scenario where the race happens, right?
>
I have seen slab pages passed for DMA in many instances, e.g. in xfs, ocfs, etc.
>> diff --git a/mm/swap.c b/mm/swap.c
>> index 8ff73d8..d4eb9f6 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -82,6 +82,16 @@ static void put_compound_page(struct page *page)
>> ? ? ? ? ? ? ? if (likely(page != page_head &&
>> ? ? ? ? ? ? ? ? ? ? ? ? ?get_page_unless_zero(page_head))) {
>> ? ? ? ? ? ? ? ? ? ? ? unsigned long flags;
>> +
>> + ? ? ? ? ? ? ? ? ? ? if (PageSlab(page_head)) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* THP can not break up slab pages, avoid
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* taking compound_lock(). */
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (put_page_testzero(page_head))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VM_BUG_ON(1);
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? atomic_dec(&page->_mapcount);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto skip_lock;
>> + ? ? ? ? ? ? ? ? ? ? }
>
> If a THP is splitted before get_page_unless_zero runs, the head page
> may be then freed and reallocated as slab. The "page" then should not
> be freed as a tail page anymore, because it's not a tail page. The
> head just accidentally become a slab (maybe not even a compound slab).
>
> To avoid such scenario this should be enough:
>
> ? ? if (PageSlab(page_head) && PageTail(page)) {
> ? ? ...
> ? ? }
>
right, I will send updated patch.
Thanks,
Pravin.