From patchwork Tue Apr 10 13:28:22 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Minchan Kim X-Patchwork-Id: 10333181 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A06B560365 for ; Tue, 10 Apr 2018 13:28:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 91167288A7 for ; Tue, 10 Apr 2018 13:28:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 855F4288BA; Tue, 10 Apr 2018 13:28:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 11FCA288A7 for ; Tue, 10 Apr 2018 13:28:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753638AbeDJN2d (ORCPT ); Tue, 10 Apr 2018 09:28:33 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:42863 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753280AbeDJN2b (ORCPT ); Tue, 10 Apr 2018 09:28:31 -0400 Received: by mail-pf0-f193.google.com with SMTP id o16so8294634pfk.9; Tue, 10 Apr 2018 06:28:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=IidKi2ge/CU75LDj+7J9GB99MWG6ys1k7H/qQ308Wh4=; b=sTtjluAhoNRs50/JPnlj2v2fmqeM/geAL4EsEHF9X1o/7t64HTT3ZX9NW5fjfpuJ+t Di1pAGMNSNji5G2KFAohDfeMRO7i3dchXtxgDxbHeDjsKyRYvxBz7sQumN2C+b/PX5ya VR8Hrawf92LV/vH12UE/nc97iRLYrg9w/5UrFBS+ZKxRcdJDpjhBme4uYYZhc2gS2B1d 9ce4h/Z8Jn+cNce78TGCSGB144enDJcHx9rLWeuY1eQjie/vuWUk7WDHVYT3CG7yiglV 8EL1OTHESzY0DNktYLY4/eo6H7835m1ftv8xzI1A399JGuW1h0jrxmJKiOTiBzI2rO8B Ze9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=IidKi2ge/CU75LDj+7J9GB99MWG6ys1k7H/qQ308Wh4=; b=dVMngverBbEBd3C5IXoMQ3ERhucEWtblKvKpxi6uONrBo8YaX10OB0FaFLfVZ0rzVB uU04TGqvC5EJ8+SDtmJM0y5Jf4Z6y9qgh3z4W23kAVYiRHIg6D4QonVbrsi1Q2qX4P95 cRI7//LbMcUSZ8AKEXfIax7P6ByvRcfRXARpjffXGJWeEAJOOQqcKY4UuHSnVdbYfy2K NWntyudmNd0xmG+PFGnfbX7SFEn21MR8Elz1drjr8ImaAGCBomCi2SLsoNaqi/uUWQ38 bhpaf8yE2w/Dee0dq6FTkwibMv0yylRFQt/aXhe3wF4QBOTMM1u0QeiikEDLaeOzzjnQ ogDg== X-Gm-Message-State: ALQs6tBdflxBth975XMGhqaACvWVSnNtoTEu8lmND+hGFQsApVa1Hz/x mHR4HsaJIywmNwzJp8j6vcA= X-Google-Smtp-Source: AIpwx489lrfatH4yxZiYqbSqUsRzBtvH9EAeTbCyjE+H1yms9r8F+qcFqqqaMRrdYgsEGvxsqTQcEA== X-Received: by 10.101.77.13 with SMTP id i13mr336701pgt.70.1523366910929; Tue, 10 Apr 2018 06:28:30 -0700 (PDT) Received: from rodete-laptop-imager.corp.google.com ([122.38.223.241]) by smtp.gmail.com with ESMTPSA id p86sm7176842pfi.55.2018.04.10.06.28.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 10 Apr 2018 06:28:29 -0700 (PDT) Date: Tue, 10 Apr 2018 22:28:22 +0900 From: Minchan Kim To: Matthew Wilcox Cc: Michal Hocko , Jaegeuk Kim , Christopher Lameter , Andrew Morton , linux-mm , LKML , Johannes Weiner , Jan Kara , Chris Fries , Chao Yu , linux-f2fs-devel@lists.sourceforge.net, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] mm: workingset: fix NULL ptr dereference Message-ID: <20180410132822.GA32026@rodete-laptop-imager.corp.google.com> References: <20180409015815.235943-1-minchan@kernel.org> <20180409024925.GA21889@bombadil.infradead.org> <20180409030930.GA214930@rodete-desktop-imager.corp.google.com> <20180409111403.GA31652@bombadil.infradead.org> <20180409112514.GA195937@rodete-laptop-imager.corp.google.com> <20180409183827.GD17558@jaegeuk-macbookpro.roam.corp.google.com> <20180409194044.GA15295@bombadil.infradead.org> <20180410082643.GX21835@dhcp22.suse.cz> <20180410120528.GB22118@bombadil.infradead.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180410120528.GB22118@bombadil.infradead.org> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Apr 10, 2018 at 05:05:28AM -0700, Matthew Wilcox wrote: > On Tue, Apr 10, 2018 at 10:26:43AM +0200, Michal Hocko wrote: > > On Mon 09-04-18 12:40:44, Matthew Wilcox wrote: > > > The problem is that the mapping gfp flags are used not only for allocating > > > pages, but also for allocating the page cache data structures that hold > > > the pages. F2FS is the only filesystem that set the __GFP_ZERO bit, > > > so it's the first time anyone's noticed that the page cache passes the > > > __GFP_ZERO bit through to the radix tree allocation routines, which > > > causes the radix tree nodes to be zeroed instead of constructed. > > > > > > I think the right solution to this is: > > > > This just hides the underlying problem that the node is not fully and > > properly initialized. Relying on the previous released state is just too > > subtle. > > That's the fundamental design of slab-with-constructors. The user provides > a constructor, so all newly allocagted objects are initialised to a known > state, then the user will restore the object to that state when it frees > the object to slab. > > > Are you going to blacklist all potential gfp flags that come > > from the mapping? This is just unmaintainable! If anything this should > > be an explicit & with the allowed set of allowed flags. > > Oh, I agree that using the set of flags used to allocate the page > in order to allocate the radix tree nodes is a pretty horrible idea. > > Your suggestion, then, is: > > - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM); > + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK); > > correct? > Looks much better. Finally, it seems everyone agree on this. However, I won't include warning part of slab allocator because I think it's improve stuff not bug fix so it could be separted. If anyone really want to include it in this stable patch, please discuss with slub maintainers before. Thanks for the reivew, Matthew, Michal, Jan and Johannes. From 652bb75124896fa040df78b98496a354f54fc524 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Tue, 10 Apr 2018 22:13:50 +0900 Subject: [PATCH v4] mm: workingset: fix NULL ptr dereference GFP mask passed to page cache functions (often coming from mapping->gfp_mask) is used both for allocation of page cache page and for allocation of radix tree metadata necessary to add the page to the page cache. When the mask contains __GFP_ZERO (as is the case for some f2fs metadata mappings), this breaks radix tree code as that code expects allocated radix tree nodes to be properly initialized by the slab constructor and not zeroed. In particular node->private_list is failing list_empty() check and the following list operation in workingset_update_node() will dereference NULL. Fix the problem by removing non-reclimable flags by GFP_RECLAIM_MASK for radix tree allocations. Fixes: 449dd6984d0e ("mm: keep page cache radix tree nodes in check") Cc: Johannes Weiner Cc: Jan Kara Cc: Jaegeuk Kim Cc: Chao Yu Cc: Christopher Lameter Cc: linux-fsdevel@vger.kernel.org Cc: stable@vger.kernel.org Suggested-by: Matthew Wilcox Reported-by: Chris Fries Signed-off-by: Minchan Kim --- mm/filemap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/filemap.c b/mm/filemap.c index ab77e19ab09c..5f3311edfea4 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -786,7 +786,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) VM_BUG_ON_PAGE(!PageLocked(new), new); VM_BUG_ON_PAGE(new->mapping, new); - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM); + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK); if (!error) { struct address_space *mapping = old->mapping; void (*freepage)(struct page *); @@ -842,7 +842,7 @@ static int __add_to_page_cache_locked(struct page *page, return error; } - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM); + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK); if (error) { if (!huge) mem_cgroup_cancel_charge(page, memcg, false);