From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754530AbcKNPnJ (ORCPT ); Mon, 14 Nov 2016 10:43:09 -0500 Received: from gum.cmpxchg.org ([85.214.110.215]:47694 "EHLO gum.cmpxchg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933875AbcKNPnD (ORCPT ); Mon, 14 Nov 2016 10:43:03 -0500 Date: Mon, 14 Nov 2016 10:42:49 -0500 From: Johannes Weiner To: Ben Hutchings Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, akpm@linux-foundation.org, Antonio SJ Musumeci , Linus Torvalds Subject: Re: [PATCH 3.16 327/346] mm: workingset: fix crash in shadow node shrinker caused by replace_page_cache_page() Message-ID: <20161114154249.GA3291@cmpxchg.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 14, 2016 at 12:14:20AM +0000, Ben Hutchings wrote: > 3.16.39-rc1 review patch. If anyone has any objections, please let me know. > > ------------------ > > From: Johannes Weiner > > 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 > Reported-by: Antonio SJ Musumeci > Debugged-by: Miklos Szeredi > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds > [bwh: Backported to 3.16: > - Implementation of page_cache_tree_insert() is different > - Adjust context] > Signed-off-by: Ben Hutchings The added sanity checks in this patch can crash kernels built with CONFIG_DEBUG_VM. While I doubt many people run 3.16 with that enabled, please also consider taking the following two changes. The first one changes the new VM_BUG_ONs to mere warnings, and the second patch addresses the underlying issue that triggered them in the first place. 21f54ddae449 Using BUG_ON() as an assert() is _never_ acceptable d3798ae8c6f3 mm: filemap: don't plant shadow entries without radix tree node Attaching the latter below, since it's drastically different than the upstream change, but a lot simpler because it predates the DAX stuff. Thanks --- >>From 06313bb20559e6da67dcc7fe6c66e928f713d061 Mon Sep 17 00:00:00 2001 From: Johannes Weiner Date: Tue, 4 Oct 2016 22:02:08 +0200 Subject: [PATCH] mm: filemap: don't plant shadow entries without radix tree node commit d3798ae8c6f3767c726403c2ca6ecc317752c9dd upstream. When the underflow checks were added to workingset_node_shadow_dec(), they triggered immediately: kernel BUG at ./include/linux/swap.h:276! invalid opcode: 0000 [#1] SMP Modules linked in: isofs usb_storage fuse xt_CHECKSUM ipt_MASQUERADE nf_nat_masquerade_ipv4 tun nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_REJECT nf_reject_ipv6 soundcore wmi acpi_als pinctrl_sunrisepoint kfifo_buf tpm_tis industrialio acpi_pad pinctrl_intel tpm_tis_core tpm nfsd auth_rpcgss nfs_acl lockd grace sunrpc dm_crypt CPU: 0 PID: 20929 Comm: blkid Not tainted 4.8.0-rc8-00087-gbe67d60ba944 #1 Hardware name: System manufacturer System Product Name/Z170-K, BIOS 1803 05/06/2016 task: ffff8faa93ecd940 task.stack: ffff8faa7f478000 RIP: page_cache_tree_insert+0xf1/0x100 Call Trace: __add_to_page_cache_locked+0x12e/0x270 add_to_page_cache_lru+0x4e/0xe0 mpage_readpages+0x112/0x1d0 blkdev_readpages+0x1d/0x20 __do_page_cache_readahead+0x1ad/0x290 force_page_cache_readahead+0xaa/0x100 page_cache_sync_readahead+0x3f/0x50 generic_file_read_iter+0x5af/0x740 blkdev_read_iter+0x35/0x40 __vfs_read+0xe1/0x130 vfs_read+0x96/0x130 SyS_read+0x55/0xc0 entry_SYSCALL_64_fastpath+0x13/0x8f Code: 03 00 48 8b 5d d8 65 48 33 1c 25 28 00 00 00 44 89 e8 75 19 48 83 c4 18 5b 41 5c 41 5d 41 5e 5d c3 0f 0b 41 bd ef ff ff ff eb d7 <0f> 0b e8 88 68 ef ff 0f 1f 84 00 RIP page_cache_tree_insert+0xf1/0x100 This is a long-standing bug in the way shadow entries are accounted in the radix tree nodes. The shrinker needs to know when radix tree nodes contain only shadow entries, no pages, so node->count is split in half to count shadows in the upper bits and pages in the lower bits. Unfortunately, the radix tree implementation doesn't know of this and assumes all entries are in node->count. When there is a shadow entry directly in root->rnode and the tree is later extended, the radix tree implementation will copy that entry into the new node and and bump its node->count, i.e. increases the page count bits. Once the shadow gets removed and we subtract from the upper counter, node->count underflows and triggers the warning. Afterwards, without node->count reaching 0 again, the radix tree node is leaked. Limit shadow entries to when we have actual radix tree nodes and can count them properly. That means we lose the ability to detect refaults from files that had only the first page faulted in at eviction time. Fixes: 449dd6984d0e ("mm: keep page cache radix tree nodes in check") Signed-off-by: Johannes Weiner Reported-and-tested-by: Linus Torvalds Reviewed-by: Jan Kara Cc: Andrew Morton Cc: stable@vger.kernel.org Signed-off-by: Linus Torvalds --- mm/filemap.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/mm/filemap.c b/mm/filemap.c index 900edfaf6df5..eb228111ae6e 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -121,6 +121,13 @@ static void page_cache_tree_delete(struct address_space *mapping, __radix_tree_lookup(&mapping->page_tree, page->index, &node, &slot); + /* + * We need a node to properly account shadow + * entries. Don't plant any without. XXX + */ + if (!node) + shadow = NULL; + if (shadow) { mapping->nrshadows++; /* -- 2.10.1