From patchwork Wed May 3 01:32:32 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Joan_Bruguera_Mic=C3=B3?= X-Patchwork-Id: 13229436 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 8DC99C7EE21 for ; Wed, 3 May 2023 01:32:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0A0006B0071; Tue, 2 May 2023 21:32:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0506A6B0072; Tue, 2 May 2023 21:32:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E80CD6B0074; Tue, 2 May 2023 21:32:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by kanga.kvack.org (Postfix) with ESMTP id 974B76B0071 for ; Tue, 2 May 2023 21:32:44 -0400 (EDT) Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-2fc3f1d6f8cso2757180f8f.3 for ; Tue, 02 May 2023 18:32:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683077564; x=1685669564; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=mTZgBzt/Dt27iMmQO6dhb2LS1gvv1g1KyF6acTixJxs=; b=O5S0To9nyxHcJIw7kPTikmqTRPb8hGKGfJiX/+CaUyeBf/Q8KNQLEa5qQG7qOvFFiw CYsfOJ7l4XASubNxVyVApEX3Bl8ME+BvLsmqnS1g/MpfUqgX3eWAfB7LkMTyA3/44KTV E6ldthUiLttApU2v+jWdw4olhg8atOZJWE9U9BFeTr4Tu/QEjekRGMPBFc0/omb2gC4D dnuzDaxjCI7QyW6Lxzc7kDZXsZ2w67pDeVr8DvaBQmaNAjJRmI2mp4E9mrJ1BnsoYQSf zMHaX0ZXKLm5nsetB8esqGry5Kv3k3CRAZmIE8brVX8X4l3Zf1mtk5xJwhAo+nl1PlBI NJOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683077564; x=1685669564; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=mTZgBzt/Dt27iMmQO6dhb2LS1gvv1g1KyF6acTixJxs=; b=F/Pw2+gNBEfHaSSog6esbYdSpIkuUFxl/z8G2jMzMBN7dfMD4l0oZCa50Fq9NVtWa7 4Z1mTras3KskQqGAxDhes+0GWikx3Dj3M+htk1g+OJ67xXK7uTO7xq/izjEQuyONW/Vp dorKB8IqzSNn3WiFE83nPanWtrs6hBwtOg80h/AAqY3uU8mBcEZULRORCucbxkRAebuF 5NFxva2n4P662Gn/M8IccKOvbILdAdNNuc2RiKQwMIpdQxuRQ14Qew5v3aX5AWwHL34O rsC0tjGjx8GGve+paVk/NzvHeO1YYxj5DZCNmV1m8hxo0ePH8UgCOC7FAIk5fqY7Nw1q jA8Q== X-Gm-Message-State: AC+VfDyDgL3/JtFKJWco+J3+Vq+L4id5WHa6kxQm7JvL5Xfig4SXVVaS zuLBVAP/d942uBvgr40RQh4= X-Google-Smtp-Source: ACHHUZ4wJxiINWHqiPZ1tK0Urj08OmEq65prz3nTqgIPb9vpzcA8vu2n1QK7yL9oCdNarP6qzxagjQ== X-Received: by 2002:a05:6000:1191:b0:306:34f6:de8a with SMTP id g17-20020a056000119100b0030634f6de8amr3239356wrx.71.1683077563489; Tue, 02 May 2023 18:32:43 -0700 (PDT) Received: from solport.. (24.red-83-42-52.dynamicip.rima-tde.net. [83.42.52.24]) by smtp.gmail.com with ESMTPSA id h7-20020a5d6e07000000b003048084a57asm22744019wrz.79.2023.05.02.18.32.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 May 2023 18:32:42 -0700 (PDT) From: =?utf-8?q?Joan_Bruguera_Mic=C3=B3?= To: Qi Zheng , Andrew Morton Cc: Roman Gushchin , linux-mm@kvack.org, linux-kernel@vger.kernel.org, =?utf-8?q?Joan_Bruguera_Mic=C3=B3?= Subject: [PATCH] mm: shrinkers: fix race condition on debugfs cleanup Date: Wed, 3 May 2023 01:32:32 +0000 Message-Id: <20230503013232.299211-1-joanbrugueram@gmail.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 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: When something registers and unregisters many shrinkers, such as: for x in $(seq 10000); do unshare -Ui true; done Sometimes the following error is printed to the kernel log: debugfs: Directory '...' with parent 'shrinker' already present! This occurs since commit badc28d4924b ("mm: shrinkers: fix deadlock in shrinker debugfs") / v6.2: Since the call to `debugfs_remove_recursive` was moved outside the `shrinker_rwsem`/`shrinker_mutex` lock, but the call to `ida_free` stayed inside, a newly registered shrinker can be re-assigned that ID and attempt to create the debugfs directory before the directory from the previous shrinker has been removed. The locking changes in commit f95bdb700bc6 ("mm: vmscan: make global slab shrink lockless") made the race condition more likely, though it existed before then. Commit badc28d4924b ("mm: shrinkers: fix deadlock in shrinker debugfs") could be reverted since the issue is addressed should no longer occur since the count and scan operations are lockless since commit 20cd1892fcc3 ("mm: shrinkers: make count and scan in shrinker debugfs lockless"). However, since this is a contended lock, prefer instead moving `ida_free` outside the lock to avoid the race. Fixes: badc28d4924b ("mm: shrinkers: fix deadlock in shrinker debugfs") Signed-off-by: Joan Bruguera Micó --- include/linux/shrinker.h | 13 +++++++++++-- mm/shrinker_debug.c | 15 ++++++++++----- mm/vmscan.c | 5 +++-- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 7bde8e1c228a..224293b2dd06 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -107,7 +107,10 @@ extern void synchronize_shrinkers(void); #ifdef CONFIG_SHRINKER_DEBUG extern int shrinker_debugfs_add(struct shrinker *shrinker); -extern struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker); +extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, + int *debugfs_id); +extern void shrinker_debugfs_remove(struct dentry *debugfs_entry, + int debugfs_id); extern int __printf(2, 3) shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...); #else /* CONFIG_SHRINKER_DEBUG */ @@ -115,10 +118,16 @@ static inline int shrinker_debugfs_add(struct shrinker *shrinker) { return 0; } -static inline struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker) +static inline struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, + int *debugfs_id) { + *debugfs_id = -1; return NULL; } +static inline void shrinker_debugfs_remove(struct dentry *debugfs_entry, + int debugfs_id) +{ +} static inline __printf(2, 3) int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) { diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c index 3f83b10c5031..fe10436d9911 100644 --- a/mm/shrinker_debug.c +++ b/mm/shrinker_debug.c @@ -237,7 +237,8 @@ int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) } EXPORT_SYMBOL(shrinker_debugfs_rename); -struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker) +struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, + int *debugfs_id) { struct dentry *entry = shrinker->debugfs_entry; @@ -246,14 +247,18 @@ struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker) kfree_const(shrinker->name); shrinker->name = NULL; - if (entry) { - ida_free(&shrinker_debugfs_ida, shrinker->debugfs_id); - shrinker->debugfs_entry = NULL; - } + *debugfs_id = entry ? shrinker->debugfs_id : -1; + shrinker->debugfs_entry = NULL; return entry; } +void shrinker_debugfs_remove(struct dentry *debugfs_entry, int debugfs_id) +{ + debugfs_remove_recursive(debugfs_entry); + ida_free(&shrinker_debugfs_ida, debugfs_id); +} + static int __init shrinker_debugfs_init(void) { struct shrinker *shrinker; diff --git a/mm/vmscan.c b/mm/vmscan.c index 5bde07409303..c7d0faa343e0 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -805,6 +805,7 @@ EXPORT_SYMBOL(register_shrinker); void unregister_shrinker(struct shrinker *shrinker) { struct dentry *debugfs_entry; + int debugfs_id; if (!(shrinker->flags & SHRINKER_REGISTERED)) return; @@ -814,13 +815,13 @@ void unregister_shrinker(struct shrinker *shrinker) shrinker->flags &= ~SHRINKER_REGISTERED; if (shrinker->flags & SHRINKER_MEMCG_AWARE) unregister_memcg_shrinker(shrinker); - debugfs_entry = shrinker_debugfs_remove(shrinker); + debugfs_entry = shrinker_debugfs_detach(shrinker, &debugfs_id); mutex_unlock(&shrinker_mutex); atomic_inc(&shrinker_srcu_generation); synchronize_srcu(&shrinker_srcu); - debugfs_remove_recursive(debugfs_entry); + shrinker_debugfs_remove(debugfs_entry, debugfs_id); kfree(shrinker->nr_deferred); shrinker->nr_deferred = NULL;