lkml.org 
[lkml]   [2016]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 327/346] mm: workingset: fix crash in shadow node shrinker caused by replace_page_cache_page()
    3.16.39-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Johannes Weiner <hannes@cmpxchg.org>

    commit 22f2ac51b6d643666f4db093f13144f773ff3f3a upstream.

    Antonio reports the following crash when using fuse under memory pressure:

    kernel BUG at /build/linux-a2WvEb/linux-4.4.0/mm/workingset.c:346!
    invalid opcode: 0000 [#1] SMP
    Modules linked in: all of them
    CPU: 2 PID: 63 Comm: kswapd0 Not tainted 4.4.0-36-generic #55-Ubuntu
    Hardware name: System manufacturer System Product Name/P8H67-M PRO, BIOS 3904 04/27/2013
    task: ffff88040cae6040 ti: ffff880407488000 task.ti: ffff880407488000
    RIP: shadow_lru_isolate+0x181/0x190
    Call Trace:
    __list_lru_walk_one.isra.3+0x8f/0x130
    list_lru_walk_one+0x23/0x30
    scan_shadow_nodes+0x34/0x50
    shrink_slab.part.40+0x1ed/0x3d0
    shrink_zone+0x2ca/0x2e0
    kswapd+0x51e/0x990
    kthread+0xd8/0xf0
    ret_from_fork+0x3f/0x70

    which corresponds to the following sanity check in the shadow node
    tracking:

    BUG_ON(node->count & RADIX_TREE_COUNT_MASK);

    The workingset code tracks radix tree nodes that exclusively contain
    shadow entries of evicted pages in them, and this (somewhat obscure)
    line checks whether there are real pages left that would interfere with
    reclaim of the radix tree node under memory pressure.

    While discussing ways how fuse might sneak pages into the radix tree
    past the workingset code, Miklos pointed to replace_page_cache_page(),
    and indeed there is a problem there: it properly accounts for the old
    page being removed - __delete_from_page_cache() does that - but then
    does a raw raw radix_tree_insert(), not accounting for the replacement
    page. Eventually the page count bits in node->count underflow while
    leaving the node incorrectly linked to the shadow node LRU.

    To address this, make sure replace_page_cache_page() uses the tracked
    page insertion code, page_cache_tree_insert(). This fixes the page
    accounting and makes sure page-containing nodes are properly unlinked
    from the shadow node LRU again.

    Also, make the sanity checks a bit less obscure by using the helpers for
    checking the number of pages and shadows in a radix tree node.

    Fixes: 449dd6984d0e ("mm: keep page cache radix tree nodes in check")
    Link: http://lkml.kernel.org/r/20160919155822.29498-1-hannes@cmpxchg.org
    Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
    Reported-by: Antonio SJ Musumeci <trapexit@spawn.link>
    Debugged-by: Miklos Szeredi <miklos@szeredi.hu>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    [bwh: Backported to 3.16:
    - Implementation of page_cache_tree_insert() is different
    - Adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    --- a/include/linux/swap.h
    +++ b/include/linux/swap.h
    @@ -274,6 +274,7 @@ static inline void workingset_node_pages

    static inline void workingset_node_pages_dec(struct radix_tree_node *node)
    {
    + VM_BUG_ON(!workingset_node_pages(node));
    node->count--;
    }

    @@ -289,6 +290,7 @@ static inline void workingset_node_shado

    static inline void workingset_node_shadows_dec(struct radix_tree_node *node)
    {
    + VM_BUG_ON(!workingset_node_shadows(node));
    node->count -= 1U << RADIX_TREE_COUNT_SHIFT;
    }

    --- a/mm/filemap.c
    +++ b/mm/filemap.c
    @@ -108,6 +108,48 @@
    * ->tasklist_lock (memory_failure, collect_procs_ao)
    */

    +static int page_cache_tree_insert(struct address_space *mapping,
    + struct page *page, void **shadowp)
    +{
    + struct radix_tree_node *node;
    + void **slot;
    + int error;
    +
    + error = __radix_tree_create(&mapping->page_tree, page->index,
    + &node, &slot);
    + if (error)
    + return error;
    + if (*slot) {
    + void *p;
    +
    + p = radix_tree_deref_slot_protected(slot, &mapping->tree_lock);
    + if (!radix_tree_exceptional_entry(p))
    + return -EEXIST;
    + if (shadowp)
    + *shadowp = p;
    + mapping->nrshadows--;
    + if (node)
    + workingset_node_shadows_dec(node);
    + }
    + radix_tree_replace_slot(slot, page);
    + mapping->nrpages++;
    + if (node) {
    + workingset_node_pages_inc(node);
    + /*
    + * Don't track node that contains actual pages.
    + *
    + * Avoid acquiring the list_lru lock if already
    + * untracked. The list_empty() test is safe as
    + * node->private_list is protected by
    + * mapping->tree_lock.
    + */
    + if (!list_empty(&node->private_list))
    + list_lru_del(&workingset_shadow_nodes,
    + &node->private_list);
    + }
    + return 0;
    +}
    +
    static void page_cache_tree_delete(struct address_space *mapping,
    struct page *page, void *shadow)
    {
    @@ -494,7 +536,7 @@ int replace_page_cache_page(struct page

    spin_lock_irq(&mapping->tree_lock);
    __delete_from_page_cache(old, NULL);
    - error = radix_tree_insert(&mapping->page_tree, offset, new);
    + error = page_cache_tree_insert(mapping, new, NULL);
    BUG_ON(error);
    mapping->nrpages++;
    __inc_zone_page_state(new, NR_FILE_PAGES);
    @@ -513,48 +555,6 @@ int replace_page_cache_page(struct page
    }
    EXPORT_SYMBOL_GPL(replace_page_cache_page);

    -static int page_cache_tree_insert(struct address_space *mapping,
    - struct page *page, void **shadowp)
    -{
    - struct radix_tree_node *node;
    - void **slot;
    - int error;
    -
    - error = __radix_tree_create(&mapping->page_tree, page->index,
    - &node, &slot);
    - if (error)
    - return error;
    - if (*slot) {
    - void *p;
    -
    - p = radix_tree_deref_slot_protected(slot, &mapping->tree_lock);
    - if (!radix_tree_exceptional_entry(p))
    - return -EEXIST;
    - if (shadowp)
    - *shadowp = p;
    - mapping->nrshadows--;
    - if (node)
    - workingset_node_shadows_dec(node);
    - }
    - radix_tree_replace_slot(slot, page);
    - mapping->nrpages++;
    - if (node) {
    - workingset_node_pages_inc(node);
    - /*
    - * Don't track node that contains actual pages.
    - *
    - * Avoid acquiring the list_lru lock if already
    - * untracked. The list_empty() test is safe as
    - * node->private_list is protected by
    - * mapping->tree_lock.
    - */
    - if (!list_empty(&node->private_list))
    - list_lru_del(&workingset_shadow_nodes,
    - &node->private_list);
    - }
    - return 0;
    -}
    -
    static int __add_to_page_cache_locked(struct page *page,
    struct address_space *mapping,
    pgoff_t offset, gfp_t gfp_mask,
    --- a/mm/workingset.c
    +++ b/mm/workingset.c
    @@ -340,21 +340,19 @@ static enum lru_status shadow_lru_isolat
    * no pages, so we expect to be able to remove them all and
    * delete and free the empty node afterwards.
    */
    -
    - BUG_ON(!node->count);
    - BUG_ON(node->count & RADIX_TREE_COUNT_MASK);
    + BUG_ON(!workingset_node_shadows(node));
    + BUG_ON(workingset_node_pages(node));

    for (i = 0; i < RADIX_TREE_MAP_SIZE; i++) {
    if (node->slots[i]) {
    BUG_ON(!radix_tree_exceptional_entry(node->slots[i]));
    node->slots[i] = NULL;
    - BUG_ON(node->count < (1U << RADIX_TREE_COUNT_SHIFT));
    - node->count -= 1U << RADIX_TREE_COUNT_SHIFT;
    + workingset_node_shadows_dec(node);
    BUG_ON(!mapping->nrshadows);
    mapping->nrshadows--;
    }
    }
    - BUG_ON(node->count);
    + BUG_ON(workingset_node_shadows(node));
    inc_zone_state(page_zone(virt_to_page(node)), WORKINGSET_NODERECLAIM);
    if (!__radix_tree_delete_node(&mapping->page_tree, node))
    BUG();
    \
     
     \ /
      Last update: 2016-11-14 04:25    [W:2.458 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site