From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F300C4345F for ; Fri, 19 Apr 2024 14:22:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C7E486B008A; Fri, 19 Apr 2024 10:22:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C2E2B6B0092; Fri, 19 Apr 2024 10:22:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ACE4D6B0093; Fri, 19 Apr 2024 10:22:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 8BA3D6B008A for ; Fri, 19 Apr 2024 10:22:40 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 40800A20FF for ; Fri, 19 Apr 2024 14:22:40 +0000 (UTC) X-FDA: 82026497280.11.41F81BB Received: from mail-qv1-f41.google.com (mail-qv1-f41.google.com [209.85.219.41]) by imf02.hostedemail.com (Postfix) with ESMTP id 39F538000C for ; Fri, 19 Apr 2024 14:22:38 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=ino3Q992; spf=pass (imf02.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.219.41 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713536558; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=LbB4KcsnZXNmzG/MHQb9mgjRPkbbsvmUawk+gcbg5EQ=; b=A7OVQeJ39adAUXk7cpF6hFXcNQn5MBYaKS1VioEiki4KYF2xQ1r4/2PbBeTfvQvk34bpdH aOspbVMpl5juLFK9ZIAso5LOzO4X+C851UsL+amjyRuUjUFD3GgZVVCbvuu66D3daxnPbK z0AQbvjGoBaibDT282z2ScI6SDtFDBY= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=ino3Q992; spf=pass (imf02.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.219.41 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713536558; a=rsa-sha256; cv=none; b=k71hJB3NF9pc7L9gR/EkQSJlAfloFTQHM7jW5Ev0ghustdvUm6n3T8V3gedjxNoJ931Znd AvKY0+uOtcwoqwdsF4HIZoNY6GVR3Sh6du/uJa6fReRI9WvBF2UQ425e4F0984L/lPtIPt g8gp5VY7GHDzyzeqLguzLYitzb6tg8o= Received: by mail-qv1-f41.google.com with SMTP id 6a1803df08f44-69b137d09e3so12690036d6.1 for ; Fri, 19 Apr 2024 07:22:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1713536557; x=1714141357; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=LbB4KcsnZXNmzG/MHQb9mgjRPkbbsvmUawk+gcbg5EQ=; b=ino3Q9922Hmc4/phbAzuT8+i97UtJubFmoC6bA8y2pbMQp9UMse91w6liB0zSbVSWt fWzJcYWfqYw6bdTLOdGzGglU06jvbNiISl9i9CPlvRyJHhF30t+7z6ZDR//pfOW0cAz1 6UCVtrx7d5dXapbEQ02bMnEwomx/irzOLnH1szBr2U/4zfDMlCVERLrboWjPNBtbpD8j D3z4yXcAUV0fHCt/Lai5ubnW5ILQzmmHCFvwo1aNnmJJpQh3uwKQ7Grz5q1SWhpOqVkf xKOMhUpxjzaU11dqagjsrVIvhMKonAlfsn8xuyslUuIxv2ncm39RD403wD4NWuIu1Kyo PA4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713536557; x=1714141357; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LbB4KcsnZXNmzG/MHQb9mgjRPkbbsvmUawk+gcbg5EQ=; b=DndK7CXbmhAwVm3BI06WD4Mvg5IA4DTGvOGGMRWVtgs1wbQhR8qeOWew8I7EtK13qv wQl9ZWDuGai4avUNIy7gUwa1QbfkbgEIT5t5gFjXR+7WyFEqAoecFRGW/F8vsm/3EY3r cJDZIXTtPB6BzqHPYoIzYBiiMWwKcG7W7HazeXDgeq2YMgLFghOv18NVeQf6JNtNNA8/ kXwV5+swNGYFFfuvBtA+vYU+PYpJ9KLk2o0SKgv5g9nwzbUsJbuVPubALO7a9ggEd2Nl BJQpUFhIPHUMmv+P7rRl+0tav4aFOtFZnopChyR1oGMBdXY2f66o2GlVBXQwZGN7d6in kc6w== X-Forwarded-Encrypted: i=1; AJvYcCVkt3bH7V2WypaDrraC6DwOJyqsguGDgd8Ly7WGM3nivQYlS7vUrnNY6rYVfRT43ppse8nL+56gBjsKElcIuoWDBwI= X-Gm-Message-State: AOJu0YyPZi7URlYm2z+SoOLuvzAZfBNv6k6OD+TWP1B/9dVPreNdRNaN F4ptS5VzMwX4+XFF5V3Vb8Nyatvb2gjC2aTTimCDbTzBLRYRFWVWhvEY9pRA/ls= X-Google-Smtp-Source: AGHT+IGAI3mZDZDKIm7knl47VQHYokRRNSjt45GZvvX762bmRlykoI881JSyUDU3V8LFn5GMhKzNWA== X-Received: by 2002:ad4:5981:0:b0:69b:2897:4fc4 with SMTP id ek1-20020ad45981000000b0069b28974fc4mr2177086qvb.58.1713536557076; Fri, 19 Apr 2024 07:22:37 -0700 (PDT) Received: from localhost ([2620:10d:c091:400::5:fe4f]) by smtp.gmail.com with ESMTPSA id i11-20020a0cab4b000000b0069b59897310sm1582606qvb.63.2024.04.19.07.22.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Apr 2024 07:22:36 -0700 (PDT) Date: Fri, 19 Apr 2024 10:22:31 -0400 From: Johannes Weiner To: Yosry Ahmed Cc: Christian Heusel , Chengming Zhou , Nhat Pham , Seth Jennings , Dan Streetman , Vitaly Wool , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, David Runge , "Richard W.M. Jones" , Mark W , regressions@lists.linux.dev Subject: Re: [REGRESSION] Null pointer dereference while shrinking zswap Message-ID: <20240419142231.GD1055428@cmpxchg.org> References: <3iccc6vjl5gminut3lvpl4va2lbnsgku5ei2d7ylftoofy3n2v@gcfdvtsq6dx2> <246c1f4d-af13-40fa-b968-fbaf36b8f91f@linux.dev> <20240417143324.GA1055428@cmpxchg.org> <4c3ppfjxnrqx6g52qvvhqzcc4pated2q5g4mi32l22nwtrkqfq@a4lk6s5zcwvb> <20240418124043.GC1055428@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Stat-Signature: zphzzkdhy6weitujhn68cg5qqr8c3hq3 X-Rspamd-Queue-Id: 39F538000C X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1713536558-709132 X-HE-Meta: U2FsdGVkX1/mr9L48Yn57gANSEt6fj1cZKvypithA2FG7hDiG4hO5B6S0CZXHyIBRpYPpUF+9DSTTveRPKMQzhmhCG1J3nz68/9Ktd8obfTWwyeWWgeI8G2kMVlfO4Qsc66wF2FOrLRk1RP2sHljW89i/HGZ1VW+AvuD5p9Yo7hVeXDvWecNMoWDLIUv7HxN4On3luwUZzrShiaS6sbi0e5uU/eQ5Rm76XjcSdVqA4fITBHuCLk8RJv8Dj/EhckvQ3p45U+4o6rTAxJ53DQJq3pwj1PakcCKKIr0w9RoB9/ifZ/CnR5ls6dmEfrvHc4ibYeLtemZkA7A5dHeRCACe2LR5DpeaNVp4Hm/+mGsIR12cXHOhp1IWR8XJNQ0tWJCF8/Ley5wSTIynedCCN0OfCSt+WsNabjU9IpG3ilxFccgYC95Ns/b56BAeD18jaSoHhJ6r/vHNIMRLSz0eQyrlx0TuCngc2+SvmM0MPsW+cEx5kI7CkU/cFVtxOy1Ugq0shn8TtG6Qdjeku+xiPMpESHn4g2rHefBL8UqhpRXNQ+mvhLQIRxtF3WEMvcHglQ/6Y6DxYabdbHqFdjHAXw1QSFD9iX/1PFcHC5onq+15MSM6vqeBt6e+laKX6nErbA3YYemcnBRuJFvfpuh6pkWSL3WquFZKe3o9jEgO4t28myZXjN4w8sMKP8wB9Mx4Qcc11EyM4mUiPGUeF/LMOC2n6+8wipP3+ZDqwFlZiUTmpClIBMqHL/NpBWsOy8tP7AavbmvP9MuWO/S+YTdu5cMDL24xc33EZkBU91HMULgUAkYgEzf3oxkOkw7KYWn5pDn+FAlbc+jDlIJmI66ZVCj3iUNnSsJO3EScLq4ysML3dwBR43xEc8iSiULOOUUOWZmqX1c88HjHGNfcWU7BZStee0Od5WX84drwZQYiUZdXKpgowvK8BguIdt7/s+BVFfI2DMEkGxPMpq5H+9rVv/ BL6q92u3 8VX5x0PDr5jGFx2KpwP2gaQ0OE1cw01KqNrKNj8ZoEBXcSotDu8/GcUHnxOnJejU3lGKrbDvQEHnWQ+zUN1MdSFbvA83kBR4aidKieOsEIYsMEy2USU1yE2A7MpplhpNt1JjWbQii2yk/Y7I= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, Apr 18, 2024 at 01:09:22PM -0700, Yosry Ahmed wrote: > On Thu, Apr 18, 2024 at 5:40 AM Johannes Weiner wrote: > > > > On Wed, Apr 17, 2024 at 07:18:14PM +0200, Christian Heusel wrote: > > > On 24/04/17 10:33AM, Johannes Weiner wrote: > > > > > > > > Christian, can you please test the below patch on top of current > > > > upstream? > > > > > > > > > > Hey Johannes, > > > > > > I have applied your patch on top of 6.9-rc4 and it did solve the crash for > > > me, thanks for hacking together a fix so quickly! 🤗 > > > > > > Tested-By: Christian Heusel > > > > Thanks for confirming it, and sorry about the breakage. > > > > Andrew, can you please use the updated changelog below? > > > > --- > > > > From 52f67f5fab6a743c2aedfc8e04a582a9d1025c28 Mon Sep 17 00:00:00 2001 > > From: Johannes Weiner > > Date: Thu, 18 Apr 2024 08:26:28 -0400 > > Subject: [PATCH] mm: zswap: fix shrinker NULL crash with cgroup_disable=memory > > > > Christian reports a NULL deref in zswap that he bisected down to the > > zswap shrinker. The issue also cropped up in the bug trackers of > > libguestfs [1] and the Red Hat bugzilla [2]. > > > > The problem is that when memcg is disabled with the boot time flag, > > the zswap shrinker might get called with sc->memcg == NULL. This is > > okay in many places, like the lruvec operations. But it crashes in > > memcg_page_state() - which is only used due to the non-node accounting > > of cgroup's the zswap memory to begin with. > > > > Nhat spotted that the memcg can be NULL in the memcg-disabled case, > > and I was then able to reproduce the crash locally as well. > > > > [1] https://github.com/libguestfs/libguestfs/issues/139 > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2275252 > > > > Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure") > > Cc: stable@vger.kernel.org [v6.8] > > Link: https://lkml.kernel.org/r/20240417143324.GA1055428@cmpxchg.org > > Reported-by: Christian Heusel > > Debugged-by: Nhat Pham > > Suggested-by: Nhat Pham > > Tested-By: Christian Heusel > > Signed-off-by: Johannes Weiner > > Thanks for fixing this. A couple of comments/questions below, but anyway LGTM: > > Acked-by: Yosry Ahmed > > > --- > > mm/zswap.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index caed028945b0..6f8850c44b61 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -1331,15 +1331,22 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker, > > if (!gfp_has_io_fs(sc->gfp_mask)) > > return 0; > > > > -#ifdef CONFIG_MEMCG_KMEM > > - mem_cgroup_flush_stats(memcg); > > - nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT; > > - nr_stored = memcg_page_state(memcg, MEMCG_ZSWAPPED); > > -#else > > - /* use pool stats instead of memcg stats */ > > - nr_backing = zswap_pool_total_size >> PAGE_SHIFT; > > - nr_stored = atomic_read(&zswap_nr_stored); > > -#endif > > + /* > > + * For memcg, use the cgroup-wide ZSWAP stats since we don't > > + * have them per-node and thus per-lruvec. Careful if memcg is > > + * runtime-disabled: we can get sc->memcg == NULL, which is ok > > + * for the lruvec, but not for memcg_page_state(). > > + * > > + * Without memcg, use the zswap pool-wide metrics. > > + */ > > + if (!mem_cgroup_disabled()) { > > With the current shrinker code, it seems like we cannot get sc->memcg > == NULL unless mem_cgroup_disabled() is true indeed. However, maybe > it's better to check if sc->memcg is not NULL directly instead? > > This would be more resilient in case the shrinker code changes and > passing sc->memcg == NULL becomes possible in other cases (e.g. during > global shrinking). It seems like other shrinkers do this, for example > see count_shadow_nodes() and deferred_split_count(). Eh, I'm not sure it's better or worse, so it's a bit hard to care. We shouldn't get NULL here when memcg is enabled, and if somebody introduces that bug it's better to catch it early than run into subtle priority inversions when the kernel is deployed to millions of hosts. > I am also wondering if we should also check !mem_cgroup_is_root() > here? We can avoid the expensive global flush and use the global stats > directly in this case. I could also send a follow up patch for this if > that's preferred. I'd rather not proliferate more memcg internals in this code. If this is a concern, optimizing it in the flush and stat functions would make more sense. Reclaim already flushes the subtree before getting here, so odds are good this is a no-op in most cases.