| From: Qi Zheng <zhengqi.arch@bytedance.com> |
| Subject: mm: shrinkers: fix deadlock in shrinker debugfs |
| Date: Thu, 2 Feb 2023 18:56:12 +0800 |
| |
| The debugfs_remove_recursive() is invoked by unregister_shrinker(), which |
| is holding the write lock of shrinker_rwsem. It will waits for the |
| handler of debugfs file complete. The handler also needs to hold the read |
| lock of shrinker_rwsem to do something. So it may cause the following |
| deadlock: |
| |
| CPU0 CPU1 |
| |
| debugfs_file_get() |
| shrinker_debugfs_count_show()/shrinker_debugfs_scan_write() |
| |
| unregister_shrinker() |
| --> down_write(&shrinker_rwsem); |
| debugfs_remove_recursive() |
| // wait for (A) |
| --> wait_for_completion(); |
| |
| // wait for (B) |
| --> down_read_killable(&shrinker_rwsem) |
| debugfs_file_put() -- (A) |
| |
| up_write() -- (B) |
| |
| The down_read_killable() can be killed, so that the above deadlock can be |
| recovered. But it still requires an extra kill action, otherwise it will |
| block all subsequent shrinker-related operations, so it's better to fix |
| it. |
| |
| [akpm@linux-foundation.org: fix CONFIG_SHRINKER_DEBUG=n stub] |
| Link: https://lkml.kernel.org/r/20230202105612.64641-1-zhengqi.arch@bytedance.com |
| Fixes: 5035ebc644ae ("mm: shrinkers: introduce debugfs interface for memory shrinkers") |
| Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> |
| Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> |
| Cc: Kent Overstreet <kent.overstreet@gmail.com> |
| Cc: Muchun Song <songmuchun@bytedance.com> |
| Cc: <stable@vger.kernel.org> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| --- |
| |
| include/linux/shrinker.h | 5 +++-- |
| mm/shrinker_debug.c | 13 ++++++++----- |
| mm/vmscan.c | 6 +++++- |
| 3 files changed, 16 insertions(+), 8 deletions(-) |
| |
| --- a/include/linux/shrinker.h~mm-shrinkers-fix-deadlock-in-shrinker-debugfs |
| +++ a/include/linux/shrinker.h |
| @@ -107,7 +107,7 @@ extern void synchronize_shrinkers(void); |
| |
| #ifdef CONFIG_SHRINKER_DEBUG |
| extern int shrinker_debugfs_add(struct shrinker *shrinker); |
| -extern void shrinker_debugfs_remove(struct shrinker *shrinker); |
| +extern struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker); |
| extern int __printf(2, 3) shrinker_debugfs_rename(struct shrinker *shrinker, |
| const char *fmt, ...); |
| #else /* CONFIG_SHRINKER_DEBUG */ |
| @@ -115,8 +115,9 @@ static inline int shrinker_debugfs_add(s |
| { |
| return 0; |
| } |
| -static inline void shrinker_debugfs_remove(struct shrinker *shrinker) |
| +static inline struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker) |
| { |
| + return NULL; |
| } |
| static inline __printf(2, 3) |
| int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) |
| --- a/mm/shrinker_debug.c~mm-shrinkers-fix-deadlock-in-shrinker-debugfs |
| +++ a/mm/shrinker_debug.c |
| @@ -246,18 +246,21 @@ int shrinker_debugfs_rename(struct shrin |
| } |
| EXPORT_SYMBOL(shrinker_debugfs_rename); |
| |
| -void shrinker_debugfs_remove(struct shrinker *shrinker) |
| +struct dentry *shrinker_debugfs_remove(struct shrinker *shrinker) |
| { |
| + struct dentry *entry = shrinker->debugfs_entry; |
| + |
| lockdep_assert_held(&shrinker_rwsem); |
| |
| kfree_const(shrinker->name); |
| shrinker->name = NULL; |
| |
| - if (!shrinker->debugfs_entry) |
| - return; |
| + if (entry) { |
| + ida_free(&shrinker_debugfs_ida, shrinker->debugfs_id); |
| + shrinker->debugfs_entry = NULL; |
| + } |
| |
| - debugfs_remove_recursive(shrinker->debugfs_entry); |
| - ida_free(&shrinker_debugfs_ida, shrinker->debugfs_id); |
| + return entry; |
| } |
| |
| static int __init shrinker_debugfs_init(void) |
| --- a/mm/vmscan.c~mm-shrinkers-fix-deadlock-in-shrinker-debugfs |
| +++ a/mm/vmscan.c |
| @@ -741,6 +741,8 @@ EXPORT_SYMBOL(register_shrinker); |
| */ |
| void unregister_shrinker(struct shrinker *shrinker) |
| { |
| + struct dentry *debugfs_entry; |
| + |
| if (!(shrinker->flags & SHRINKER_REGISTERED)) |
| return; |
| |
| @@ -749,9 +751,11 @@ void unregister_shrinker(struct shrinker |
| shrinker->flags &= ~SHRINKER_REGISTERED; |
| if (shrinker->flags & SHRINKER_MEMCG_AWARE) |
| unregister_memcg_shrinker(shrinker); |
| - shrinker_debugfs_remove(shrinker); |
| + debugfs_entry = shrinker_debugfs_remove(shrinker); |
| up_write(&shrinker_rwsem); |
| |
| + debugfs_remove_recursive(debugfs_entry); |
| + |
| kfree(shrinker->nr_deferred); |
| shrinker->nr_deferred = NULL; |
| } |
| _ |