- Linux-stable-mirror - lists.linaro.org

+ mm-memory_hotplug-fix-call-folio_test_large-with-tail-page-in-do_migrate_range.patch added to mm-hotfixes-unstable branch

by Andrew Morton

The patch titled Subject: mm/memory_hotplug: fix call folio_test_large with tail page in do_migrate_range has been added to the -mm mm-hotfixes-unstable branch. Its filename is mm-memory_hotplug-fix-call-folio_test_large-with-tail-page-in-do_migrate_range.patch This patch will shortly appear at https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patche… This patch will later appear in the mm-hotfixes-unstable branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's *** Remember to use Documentation/process/submit-checklist.rst when testing your code *** The -mm tree is included into linux-next via the mm-everything branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm and is updated there every 2-3 working days ------------------------------------------------------ From: Jinjiang Tu <tujinjiang(a)huawei.com> Subject: mm/memory_hotplug: fix call folio_test_large with tail page in do_migrate_range Date: Mon, 24 Mar 2025 21:17:50 +0800 We triggered the below BUG: page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x2 pfn:0x240402 head: order:9 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0 flags: 0x1ffffe0000000040(head|node=1|zone=3|lastcpupid=0x1ffff) page_type: f4(hugetlb) page dumped because: VM_BUG_ON_PAGE(page->compound_head & 1) ------------[ cut here ]------------ kernel BUG at ./include/linux/page-flags.h:310! Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP Modules linked in: CPU: 7 UID: 0 PID: 166 Comm: sh Not tainted 6.14.0-rc7-dirty #374 Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015 pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : const_folio_flags+0x3c/0x58 lr : const_folio_flags+0x3c/0x58 Call trace: const_folio_flags+0x3c/0x58 (P) do_migrate_range+0x164/0x720 offline_pages+0x63c/0x6fc memory_subsys_offline+0x190/0x1f4 device_offline+0xc0/0x13c state_store+0x90/0xd8 dev_attr_store+0x18/0x2c sysfs_kf_write+0x44/0x54 kernfs_fop_write_iter+0x120/0x1cc vfs_write+0x240/0x378 ksys_write+0x70/0x108 __arm64_sys_write+0x1c/0x28 invoke_syscall+0x48/0x10c el0_svc_common.constprop.0+0x40/0xe0 When allocating a hugetlb folio, between the folio is taken from buddy and prep_compound_page() is called, start_isolate_page_range() and do_migrate_range() is called. When do_migrate_range() scans the head page of the hugetlb folio, the compound_head field isn't set, so scans the tail page next. And at this time, the compound_head field of tail page is set, folio_test_large() is called by tail page, thus triggers VM_BUG_ON(). To fix it, get folio refcount before calling folio_test_large(). Link: https://lkml.kernel.org/r/20250324131750.1551884-1-tujinjiang@huawei.com Fixes: 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp migration") Signed-off-by: Jinjiang Tu <tujinjiang(a)huawei.com> Cc: David Hildenbrand <david(a)redhat.com> Cc: Kefeng Wang <wangkefeng.wang(a)huawei.com> Cc: Nanyong Sun <sunnanyong(a)huawei.com> Cc: Naoya Horiguchi <nao.horiguchi(a)gmail.com> Cc: Oscar Salvador <osalvador(a)suse.de> Cc: <stable(a)vger.kernel.org> Signed-off-by: Andrew Morton <akpm(a)linux-foundation.org> --- mm/memory_hotplug.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) --- a/mm/memory_hotplug.c~mm-memory_hotplug-fix-call-folio_test_large-with-tail-page-in-do_migrate_range +++ a/mm/memory_hotplug.c @@ -1813,21 +1813,15 @@ static void do_migrate_range(unsigned lo page = pfn_to_page(pfn); folio = page_folio(page); - /* - * No reference or lock is held on the folio, so it might - * be modified concurrently (e.g. split). As such, - * folio_nr_pages() may read garbage. This is fine as the outer - * loop will revisit the split folio later. - */ - if (folio_test_large(folio)) - pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; - if (!folio_try_get(folio)) continue; if (unlikely(page_folio(page) != folio)) goto put_folio; + if (folio_test_large(folio)) + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; + if (folio_contain_hwpoisoned_page(folio)) { if (WARN_ON(folio_test_lru(folio))) folio_isolate_lru(folio); _ Patches currently in -mm which might be from tujinjiang(a)huawei.com are mm-contig_alloc-fix-alloc_contig_range-when-__gfp_comp-and-order-max_order.patch mm-memory_hotplug-fix-call-folio_test_large-with-tail-page-in-do_migrate_range.patch

2 months, 4 weeks

1
0
0 0

[PATCH 5.10.y] btrfs: handle errors from btrfs_dec_ref() properly

by jianqi.ren.cn＠windriver.com

From: Josef Bacik <josef(a)toxicpanda.com> [ Upstream commit 5eb178f373b4f16f3b42d55ff88fc94dd95b93b1 ] In walk_up_proc() we BUG_ON(ret) from btrfs_dec_ref(). This is incorrect, we have proper error handling here, return the error. Signed-off-by: Josef Bacik <josef(a)toxicpanda.com> Reviewed-by: David Sterba <dsterba(a)suse.com> Signed-off-by: David Sterba <dsterba(a)suse.com> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- fs/btrfs/extent-tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index a8089bf2be98..cf2b65be04b5 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5240,7 +5240,10 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, ret = btrfs_dec_ref(trans, root, eb, 1); else ret = btrfs_dec_ref(trans, root, eb, 0); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + btrfs_abort_transaction(trans, ret); + return ret; + } if (is_fstree(root->root_key.objectid)) { ret = btrfs_qgroup_trace_leaf_items(trans, eb); if (ret) { -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH 5.15.y] btrfs: handle errors from btrfs_dec_ref() properly

by jianqi.ren.cn＠windriver.com

From: Josef Bacik <josef(a)toxicpanda.com> [ Upstream commit 5eb178f373b4f16f3b42d55ff88fc94dd95b93b1 ] In walk_up_proc() we BUG_ON(ret) from btrfs_dec_ref(). This is incorrect, we have proper error handling here, return the error. Signed-off-by: Josef Bacik <josef(a)toxicpanda.com> Reviewed-by: David Sterba <dsterba(a)suse.com> Signed-off-by: David Sterba <dsterba(a)suse.com> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- fs/btrfs/extent-tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index e9659e29d657..551faae77bc3 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5461,7 +5461,10 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, ret = btrfs_dec_ref(trans, root, eb, 1); else ret = btrfs_dec_ref(trans, root, eb, 0); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + btrfs_abort_transaction(trans, ret); + return ret; + } if (is_fstree(root->root_key.objectid)) { ret = btrfs_qgroup_trace_leaf_items(trans, eb); if (ret) { -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH 6.1.y] btrfs: handle errors from btrfs_dec_ref() properly

by jianqi.ren.cn＠windriver.com

From: Josef Bacik <josef(a)toxicpanda.com> [ Upstream commit 5eb178f373b4f16f3b42d55ff88fc94dd95b93b1 ] In walk_up_proc() we BUG_ON(ret) from btrfs_dec_ref(). This is incorrect, we have proper error handling here, return the error. Signed-off-by: Josef Bacik <josef(a)toxicpanda.com> Reviewed-by: David Sterba <dsterba(a)suse.com> Signed-off-by: David Sterba <dsterba(a)suse.com> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- fs/btrfs/extent-tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 50bc553cc73a..9040108eda64 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5575,7 +5575,10 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, ret = btrfs_dec_ref(trans, root, eb, 1); else ret = btrfs_dec_ref(trans, root, eb, 0); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + btrfs_abort_transaction(trans, ret); + return ret; + } if (is_fstree(root->root_key.objectid)) { ret = btrfs_qgroup_trace_leaf_items(trans, eb); if (ret) { -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH 6.6.y] btrfs: handle errors from btrfs_dec_ref() properly

by jianqi.ren.cn＠windriver.com

From: Josef Bacik <josef(a)toxicpanda.com> [ Upstream commit 5eb178f373b4f16f3b42d55ff88fc94dd95b93b1 ] In walk_up_proc() we BUG_ON(ret) from btrfs_dec_ref(). This is incorrect, we have proper error handling here, return the error. Signed-off-by: Josef Bacik <josef(a)toxicpanda.com> Reviewed-by: David Sterba <dsterba(a)suse.com> Signed-off-by: David Sterba <dsterba(a)suse.com> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- fs/btrfs/extent-tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 021cf468274b..af03a1c6ba76 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5540,7 +5540,10 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, ret = btrfs_dec_ref(trans, root, eb, 1); else ret = btrfs_dec_ref(trans, root, eb, 0); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + btrfs_abort_transaction(trans, ret); + return ret; + } if (is_fstree(root->root_key.objectid)) { ret = btrfs_qgroup_trace_leaf_items(trans, eb); if (ret) { -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH 6.1.y] kunit/overflow: Fix UB in overflow_allocation_test

by jianqi.ren.cn＠windriver.com

From: Ivan Orlov <ivan.orlov0322(a)gmail.com> [ Upstream commit 92e9bac18124682c4b99ede9ee3bcdd68f121e92 ] The 'device_name' array doesn't exist out of the 'overflow_allocation_test' function scope. However, it is being used as a driver name when calling 'kunit_driver_create' from 'kunit_device_register'. It produces the kernel panic with KASAN enabled. Since this variable is used in one place only, remove it and pass the device name into kunit_device_register directly as an ascii string. Signed-off-by: Ivan Orlov <ivan.orlov0322(a)gmail.com> Reviewed-by: David Gow <davidgow(a)google.com> Link: https://lore.kernel.org/r/20240815000431.401869-1-ivan.orlov0322@gmail.com Signed-off-by: Kees Cook <kees(a)kernel.org> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- lib/overflow_kunit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c index b8556a2e7bb1..e499e13856fb 100644 --- a/lib/overflow_kunit.c +++ b/lib/overflow_kunit.c @@ -608,7 +608,6 @@ DEFINE_TEST_ALLOC(devm_kzalloc, devm_kfree, 1, 1, 0); static void overflow_allocation_test(struct kunit *test) { - const char device_name[] = "overflow-test"; struct device *dev; int count = 0; @@ -618,7 +617,7 @@ static void overflow_allocation_test(struct kunit *test) } while (0) /* Create dummy device for devm_kmalloc()-family tests. */ - dev = root_device_register(device_name); + dev = root_device_register("overflow-test"); KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), "Cannot register test device\n"); -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH 6.6.y] kunit/overflow: Fix UB in overflow_allocation_test

by jianqi.ren.cn＠windriver.com

From: Ivan Orlov <ivan.orlov0322(a)gmail.com> [ Upstream commit 92e9bac18124682c4b99ede9ee3bcdd68f121e92 ] The 'device_name' array doesn't exist out of the 'overflow_allocation_test' function scope. However, it is being used as a driver name when calling 'kunit_driver_create' from 'kunit_device_register'. It produces the kernel panic with KASAN enabled. Since this variable is used in one place only, remove it and pass the device name into kunit_device_register directly as an ascii string. Signed-off-by: Ivan Orlov <ivan.orlov0322(a)gmail.com> Reviewed-by: David Gow <davidgow(a)google.com> Link: https://lore.kernel.org/r/20240815000431.401869-1-ivan.orlov0322@gmail.com Signed-off-by: Kees Cook <kees(a)kernel.org> Signed-off-by: Jianqi Ren <jianqi.ren.cn(a)windriver.com> Signed-off-by: He Zhe <zhe.he(a)windriver.com> --- Verified the build test --- lib/overflow_kunit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c index 34db0b3aa502..9493a1b28b9e 100644 --- a/lib/overflow_kunit.c +++ b/lib/overflow_kunit.c @@ -608,7 +608,6 @@ DEFINE_TEST_ALLOC(devm_kzalloc, devm_kfree, 1, 1, 0); static void overflow_allocation_test(struct kunit *test) { - const char device_name[] = "overflow-test"; struct device *dev; int count = 0; @@ -618,7 +617,7 @@ static void overflow_allocation_test(struct kunit *test) } while (0) /* Create dummy device for devm_kmalloc()-family tests. */ - dev = root_device_register(device_name); + dev = root_device_register("overflow-test"); KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), "Cannot register test device\n"); -- 2.34.1

2 months, 4 weeks

1
0
0 0

[PATCH v5] KEYS: Add a list for unreferenced keys

by Jarkko Sakkinen

From: Jarkko Sakkinen <jarkko.sakkinen(a)opinsys.com> Add an isolated list of unreferenced keys to be queued for deletion, and try to pin the keys in the garbage collector before processing anything. Skip unpinnable keys. Use this list for blocking the reaping process during the teardown: 1. First off, the keys added to `keys_graveyard` are snapshotted, and the list is flushed. This the very last step in `key_put()`. 2. `key_put()` reaches zero. This will mark key as busy for the garbage collector. 3. `key_garbage_collector()` will try to increase refcount, which won't go above zero. Whenever this happens, the key will be skipped. Cc: stable(a)vger.kernel.org # v6.1+ Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen(a)opinsys.com> --- v5: - Rebased on top of v6.15-rc - Updated commit message to explain how spin lock and refcount isolate the time window in key_put(). v4: - Pin the key while processing key type teardown. Skip dead keys. - Revert key_gc_graveyard back key_gc_unused_keys. - Rewrote the commit message. - "unsigned long flags" declaration somehow did make to the previous patch (sorry). v3: - Using spin_lock() fails since key_put() is executed inside IRQs. Using spin_lock_irqsave() would neither work given the lock is acquired for /proc/keys. Therefore, separate the lock for graveyard and key_graveyard before reaping key_serial_tree. v2: - Rename key_gc_unused_keys as key_gc_graveyard, and re-document the function. --- include/linux/key.h | 7 ++----- security/keys/gc.c | 21 +++++++++++++++++++++ security/keys/internal.h | 2 ++ security/keys/key.c | 11 ++++------- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/include/linux/key.h b/include/linux/key.h index ba05de8579ec..c50659184bdf 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -195,10 +195,8 @@ enum key_state { struct key { refcount_t usage; /* number of references */ key_serial_t serial; /* key serial number */ - union { - struct list_head graveyard_link; - struct rb_node serial_node; - }; + struct list_head graveyard_link; /* key->usage == 0 */ + struct rb_node serial_node; #ifdef CONFIG_KEY_NOTIFICATIONS struct watch_list *watchers; /* Entities watching this key for changes */ #endif @@ -236,7 +234,6 @@ struct key { #define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */ #define KEY_FLAG_KEEP 8 /* set if key should not be removed */ #define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */ -#define KEY_FLAG_FINAL_PUT 10 /* set if final put has happened on key */ /* the key type and key description string * - the desc is used to match a key against search criteria diff --git a/security/keys/gc.c b/security/keys/gc.c index 4a7f32a1208b..e32534027494 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -193,6 +193,7 @@ static void key_garbage_collector(struct work_struct *work) struct rb_node *cursor; struct key *key; time64_t new_timer, limit, expiry; + unsigned long flags; kenter("[%lx,%x]", key_gc_flags, gc_state); @@ -210,17 +211,36 @@ static void key_garbage_collector(struct work_struct *work) new_timer = TIME64_MAX; + spin_lock_irqsave(&key_graveyard_lock, flags); + list_splice_init(&key_graveyard, &graveyard); + spin_unlock_irqrestore(&key_graveyard_lock, flags); + + list_for_each_entry(key, &graveyard, graveyard_link) { + spin_lock(&key_serial_lock); + kdebug("unrefd key %d", key->serial); + rb_erase(&key->serial_node, &key_serial_tree); + spin_unlock(&key_serial_lock); + } + /* As only this function is permitted to remove things from the key * serial tree, if cursor is non-NULL then it will always point to a * valid node in the tree - even if lock got dropped. */ spin_lock(&key_serial_lock); + key = NULL; cursor = rb_first(&key_serial_tree); continue_scanning: + key_put(key); while (cursor) { key = rb_entry(cursor, struct key, serial_node); cursor = rb_next(cursor); + /* key_get(), unless zero: */ + if (!refcount_inc_not_zero(&key->usage)) { + key = NULL; + gc_state |= KEY_GC_REAP_AGAIN; + goto skip_dead_key; + } if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) { if (key->type == key_gc_dead_keytype) { @@ -273,6 +293,7 @@ static void key_garbage_collector(struct work_struct *work) spin_lock(&key_serial_lock); goto continue_scanning; } + key_put(key); /* We've completed the pass. Set the timer if we need to and queue a * new cycle if necessary. We keep executing cycles until we find one diff --git a/security/keys/internal.h b/security/keys/internal.h index 676d4ce8b431..4e3d9b322390 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -69,6 +69,8 @@ extern spinlock_t key_graveyard_lock; extern struct rb_root key_user_tree; extern spinlock_t key_user_lock; extern struct key_user root_key_user; +extern struct list_head key_graveyard; +extern spinlock_t key_graveyard_lock; extern struct key_user *key_user_lookup(kuid_t uid); extern void key_user_put(struct key_user *user); diff --git a/security/keys/key.c b/security/keys/key.c index 23cfa62f9c7e..7511f2017b6b 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -22,6 +22,8 @@ DEFINE_SPINLOCK(key_serial_lock); struct rb_root key_user_tree; /* tree of quota records indexed by UID */ DEFINE_SPINLOCK(key_user_lock); +LIST_HEAD(key_graveyard); +DEFINE_SPINLOCK(key_graveyard_lock); unsigned int key_quota_root_maxkeys = 1000000; /* root's key count quota */ unsigned int key_quota_root_maxbytes = 25000000; /* root's key space quota */ @@ -658,14 +660,9 @@ void key_put(struct key *key) key->user->qnbytes -= key->quotalen; spin_unlock_irqrestore(&key->user->lock, flags); } - smp_mb(); /* key->user before FINAL_PUT set. */ - set_bit(KEY_FLAG_FINAL_PUT, &key->flags); - spin_lock(&key_serial_lock); - rb_erase(&key->serial_node, &key_serial_tree); - spin_unlock(&key_serial_lock); - spin_lock(&key_graveyard_lock); + spin_lock_irqsave(&key_graveyard_lock, flags); list_add_tail(&key->graveyard_link, &key_graveyard); - spin_unlock(&key_graveyard_lock); + spin_unlock_irqrestore(&key_graveyard_lock, flags); schedule_work(&key_gc_work); } } -- 2.39.5

2 months, 4 weeks

1
1
0 0

[PATCH v6] KEYS: Add a list for unreferenced keys

by Jarkko Sakkinen

From: Jarkko Sakkinen <jarkko.sakkinen(a)opinsys.com> Add an isolated list of unreferenced keys to be queued for deletion, and try to pin the keys in the garbage collector before processing anything. Skip unpinnable keys. Use this list for blocking the reaping process during the teardown: 1. First off, the keys added to `keys_graveyard` are snapshotted, and the list is flushed. This the very last step in `key_put()`. 2. `key_put()` reaches zero. This will mark key as busy for the garbage collector. 3. `key_garbage_collector()` will try to increase refcount, which won't go above zero. Whenever this happens, the key will be skipped. Cc: stable(a)vger.kernel.org # v6.1+ Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen(a)opinsys.com> --- v6: - Rebase went wrong in v5. v5: - Rebased on top of v6.15-rc - Updated commit message to explain how spin lock and refcount isolate the time window in key_put(). v4: - Pin the key while processing key type teardown. Skip dead keys. - Revert key_gc_graveyard back key_gc_unused_keys. - Rewrote the commit message. - "unsigned long flags" declaration somehow did make to the previous patch (sorry). v3: - Using spin_lock() fails since key_put() is executed inside IRQs. Using spin_lock_irqsave() would neither work given the lock is acquired for /proc/keys. Therefore, separate the lock for graveyard and key_graveyard before reaping key_serial_tree. v2: - Rename key_gc_unused_keys as key_gc_graveyard, and re-document the function. --- include/linux/key.h | 7 ++----- security/keys/gc.c | 44 +++++++++++++++++++++++++--------------- security/keys/internal.h | 5 +++++ security/keys/key.c | 7 +++++-- 4 files changed, 40 insertions(+), 23 deletions(-) diff --git a/include/linux/key.h b/include/linux/key.h index ba05de8579ec..c50659184bdf 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -195,10 +195,8 @@ enum key_state { struct key { refcount_t usage; /* number of references */ key_serial_t serial; /* key serial number */ - union { - struct list_head graveyard_link; - struct rb_node serial_node; - }; + struct list_head graveyard_link; /* key->usage == 0 */ + struct rb_node serial_node; #ifdef CONFIG_KEY_NOTIFICATIONS struct watch_list *watchers; /* Entities watching this key for changes */ #endif @@ -236,7 +234,6 @@ struct key { #define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */ #define KEY_FLAG_KEEP 8 /* set if key should not be removed */ #define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */ -#define KEY_FLAG_FINAL_PUT 10 /* set if final put has happened on key */ /* the key type and key description string * - the desc is used to match a key against search criteria diff --git a/security/keys/gc.c b/security/keys/gc.c index f27223ea4578..e32534027494 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -10,6 +10,10 @@ #include <keys/keyring-type.h> #include "internal.h" +LIST_HEAD(key_graveyard); +DEFINE_SPINLOCK(key_graveyard_lock); + + /* * Delay between key revocation/expiry in seconds */ @@ -189,6 +193,7 @@ static void key_garbage_collector(struct work_struct *work) struct rb_node *cursor; struct key *key; time64_t new_timer, limit, expiry; + unsigned long flags; kenter("[%lx,%x]", key_gc_flags, gc_state); @@ -206,21 +211,35 @@ static void key_garbage_collector(struct work_struct *work) new_timer = TIME64_MAX; + spin_lock_irqsave(&key_graveyard_lock, flags); + list_splice_init(&key_graveyard, &graveyard); + spin_unlock_irqrestore(&key_graveyard_lock, flags); + + list_for_each_entry(key, &graveyard, graveyard_link) { + spin_lock(&key_serial_lock); + kdebug("unrefd key %d", key->serial); + rb_erase(&key->serial_node, &key_serial_tree); + spin_unlock(&key_serial_lock); + } + /* As only this function is permitted to remove things from the key * serial tree, if cursor is non-NULL then it will always point to a * valid node in the tree - even if lock got dropped. */ spin_lock(&key_serial_lock); + key = NULL; cursor = rb_first(&key_serial_tree); continue_scanning: + key_put(key); while (cursor) { key = rb_entry(cursor, struct key, serial_node); cursor = rb_next(cursor); - - if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) { - smp_mb(); /* Clobber key->user after FINAL_PUT seen. */ - goto found_unreferenced_key; + /* key_get(), unless zero: */ + if (!refcount_inc_not_zero(&key->usage)) { + key = NULL; + gc_state |= KEY_GC_REAP_AGAIN; + goto skip_dead_key; } if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) { @@ -274,6 +293,7 @@ static void key_garbage_collector(struct work_struct *work) spin_lock(&key_serial_lock); goto continue_scanning; } + key_put(key); /* We've completed the pass. Set the timer if we need to and queue a * new cycle if necessary. We keep executing cycles until we find one @@ -286,6 +306,10 @@ static void key_garbage_collector(struct work_struct *work) key_schedule_gc(new_timer); } + spin_lock(&key_graveyard_lock); + list_splice_init(&key_graveyard, &graveyard); + spin_unlock(&key_graveyard_lock); + if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2) || !list_empty(&graveyard)) { /* Make sure that all pending keyring payload destructions are @@ -328,18 +352,6 @@ static void key_garbage_collector(struct work_struct *work) kleave(" [end %x]", gc_state); return; - /* We found an unreferenced key - once we've removed it from the tree, - * we can safely drop the lock. - */ -found_unreferenced_key: - kdebug("unrefd key %d", key->serial); - rb_erase(&key->serial_node, &key_serial_tree); - spin_unlock(&key_serial_lock); - - list_add_tail(&key->graveyard_link, &graveyard); - gc_state |= KEY_GC_REAP_AGAIN; - goto maybe_resched; - /* We found a restricted keyring and need to update the restriction if * it is associated with the dead key type. */ diff --git a/security/keys/internal.h b/security/keys/internal.h index 2cffa6dc8255..4e3d9b322390 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -63,9 +63,14 @@ struct key_user { int qnbytes; /* number of bytes allocated to this user */ }; +extern struct list_head key_graveyard; +extern spinlock_t key_graveyard_lock; + extern struct rb_root key_user_tree; extern spinlock_t key_user_lock; extern struct key_user root_key_user; +extern struct list_head key_graveyard; +extern spinlock_t key_graveyard_lock; extern struct key_user *key_user_lookup(kuid_t uid); extern void key_user_put(struct key_user *user); diff --git a/security/keys/key.c b/security/keys/key.c index 7198cd2ac3a3..7511f2017b6b 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -22,6 +22,8 @@ DEFINE_SPINLOCK(key_serial_lock); struct rb_root key_user_tree; /* tree of quota records indexed by UID */ DEFINE_SPINLOCK(key_user_lock); +LIST_HEAD(key_graveyard); +DEFINE_SPINLOCK(key_graveyard_lock); unsigned int key_quota_root_maxkeys = 1000000; /* root's key count quota */ unsigned int key_quota_root_maxbytes = 25000000; /* root's key space quota */ @@ -658,8 +660,9 @@ void key_put(struct key *key) key->user->qnbytes -= key->quotalen; spin_unlock_irqrestore(&key->user->lock, flags); } - smp_mb(); /* key->user before FINAL_PUT set. */ - set_bit(KEY_FLAG_FINAL_PUT, &key->flags); + spin_lock_irqsave(&key_graveyard_lock, flags); + list_add_tail(&key->graveyard_link, &key_graveyard); + spin_unlock_irqrestore(&key_graveyard_lock, flags); schedule_work(&key_gc_work); } } -- 2.39.5

2 months, 4 weeks

1
0
0 0

[RFC PATCH net-next v2 01/11] net: phylink: fix possible circular locking dep with config in-band

by Christian Marangi

Debug lock detection revealed a possible circular locking dependency between phylink_major_config() and phylink_bringup_phy(). This was introduced by the addition of phy_config_inband(), which acquires the phydev lock. This made the locking order in phylink_bringup_phy() inconsistent, as it acquires the phydev lock before phylink's state_mutex. A deadlock can occur when phylink_major_config() is called from phylink_resolve(), where the state_mutex is taken first and then the phydev lock. This is the reverse of the order used in phylink_bringup_phy(). To avoid this circular dependency, change the locking order in phylink_bringup_phy() to match the pattern used in phylink_resolve(): take state_mutex first, then the phydev lock. A sample lockdep warning is included below for reference: [ 147.749178] [ 147.750682] ====================================================== [ 147.756850] WARNING: possible circular locking dependency detected [ 147.763019] 6.14.0-next-20250404+ #0 Tainted: G O [ 147.769189] ------------------------------------------------------ [ 147.775356] kworker/u16:0/12 is trying to acquire lock: [ 147.780571] ffffff80ce9bcf08 (&dev->lock#2){+.+.}-{4:4}, at: phy_config_inband+0x44/0x90 [ 147.788672] [ 147.788672] but task is already holding lock: [ 147.794492] ffffff80c0dfbda0 (&pl->state_mutex){+.+.}-{4:4}, at: phylink_resolve+0x2c/0x6a8 [ 147.802840] [ 147.802840] which lock already depends on the new lock. [ 147.802840] [ 147.811002] [ 147.811002] the existing dependency chain (in reverse order) is: [ 147.818472] [ 147.818472] -> #1 (&pl->state_mutex){+.+.}-{4:4}: [ 147.824647] __mutex_lock+0x90/0x924 [ 147.828738] mutex_lock_nested+0x20/0x28 [ 147.833173] phylink_bringup_phy+0x210/0x700 [ 147.837954] phylink_fwnode_phy_connect+0xe0/0x124 [ 147.843256] phylink_of_phy_connect+0x18/0x20 [ 147.848124] dsa_user_create+0x210/0x414 [ 147.852561] dsa_port_setup+0xd4/0x14c [ 147.856823] dsa_register_switch+0xbb0/0xe40 [ 147.861605] mt7988_probe+0xf8/0x140 [ 147.865694] platform_probe+0x64/0xbc [ 147.869869] really_probe+0xbc/0x388 [ 147.873955] __driver_probe_device+0x78/0x154 [ 147.878823] driver_probe_device+0x3c/0xd4 [ 147.883430] __device_attach_driver+0xb0/0x144 [ 147.888383] bus_for_each_drv+0x6c/0xb0 [ 147.892732] __device_attach+0x9c/0x19c [ 147.897078] device_initial_probe+0x10/0x18 [ 147.901771] bus_probe_device+0xa8/0xac [ 147.906118] deferred_probe_work_func+0xb8/0x118 [ 147.911245] process_one_work+0x224/0x610 [ 147.915769] worker_thread+0x1b8/0x35c [ 147.920029] kthread+0x11c/0x1e8 [ 147.923769] ret_from_fork+0x10/0x20 [ 147.927857] [ 147.927857] -> #0 (&dev->lock#2){+.+.}-{4:4}: [ 147.933686] __lock_acquire+0x12b8/0x1ff0 [ 147.938209] lock_acquire+0xf4/0x2d8 [ 147.942295] __mutex_lock+0x90/0x924 [ 147.946383] mutex_lock_nested+0x20/0x28 [ 147.950817] phy_config_inband+0x44/0x90 [ 147.955252] phylink_major_config+0x684/0xa64 [ 147.960120] phylink_resolve+0x24c/0x6a8 [ 147.964554] process_one_work+0x224/0x610 [ 147.969075] worker_thread+0x1b8/0x35c [ 147.973335] kthread+0x11c/0x1e8 [ 147.977075] ret_from_fork+0x10/0x20 [ 147.981162] [ 147.981162] other info that might help us debug this: [ 147.981162] [ 147.989150] Possible unsafe locking scenario: [ 147.989150] [ 147.995056] CPU0 CPU1 [ 147.999575] ---- ---- [ 148.004094] lock(&pl->state_mutex); [ 148.007748] lock(&dev->lock#2); [ 148.013572] lock(&pl->state_mutex); [ 148.019742] lock(&dev->lock#2); [ 148.023051] [ 148.023051] *** DEADLOCK *** [ 148.023051] [ 148.028958] 3 locks held by kworker/u16:0/12: [ 148.033304] #0: ffffff80c0011d48 ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x1a8/0x610 [ 148.044082] #1: ffffffc081ca3dd8 ((work_completion)(&pl->resolve)){+.+.}-{0:0}, at: process_one_work+0x1d0/0x610 [ 148.054338] #2: ffffff80c0dfbda0 (&pl->state_mutex){+.+.}-{4:4}, at: phylink_resolve+0x2c/0x6a8 [ 148.063119] [ 148.063119] stack backtrace: [ 148.067465] CPU: 3 UID: 0 PID: 12 Comm: kworker/u16:0 Tainted: G O 6.14.0-next-20250404+ #0 NONE [ 148.067472] Tainted: [O]=OOT_MODULE [ 148.067474] Hardware name: Bananapi BPI-R4 2.5GE PoE (DT) [ 148.067476] Workqueue: events_power_efficient phylink_resolve [ 148.067482] Call trace: [ 148.067484] show_stack+0x14/0x1c (C) [ 148.067492] dump_stack_lvl+0x84/0xc0 [ 148.067497] dump_stack+0x14/0x1c [ 148.067500] print_circular_bug+0x330/0x43c [ 148.067505] check_noncircular+0x124/0x134 [ 148.067508] __lock_acquire+0x12b8/0x1ff0 [ 148.067512] lock_acquire+0xf4/0x2d8 [ 148.067516] __mutex_lock+0x90/0x924 [ 148.067521] mutex_lock_nested+0x20/0x28 [ 148.067527] phy_config_inband+0x44/0x90 [ 148.067531] phylink_major_config+0x684/0xa64 [ 148.067535] phylink_resolve+0x24c/0x6a8 [ 148.067539] process_one_work+0x224/0x610 [ 148.067544] worker_thread+0x1b8/0x35c [ 148.067548] kthread+0x11c/0x1e8 [ 148.067552] ret_from_fork+0x10/0x20 Cc: stable(a)vger.kernel.org Fixes: 5fd0f1a02e75 ("net: phylink: add negotiation of in-band capabilities") Signed-off-by: Christian Marangi <ansuelsmth(a)gmail.com> --- drivers/net/phy/phylink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 69ca765485db..4a1edf19dfad 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -2072,8 +2072,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, dev_name(&phy->mdio.dev), phy->drv->name, irq_str); kfree(irq_str); - mutex_lock(&phy->lock); mutex_lock(&pl->state_mutex); + mutex_lock(&phy->lock); pl->phydev = phy; pl->phy_state.interface = interface; pl->phy_state.pause = MLO_PAUSE_NONE; @@ -2115,8 +2115,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, phy_disable_eee(phy); } - mutex_unlock(&pl->state_mutex); mutex_unlock(&phy->lock); + mutex_unlock(&pl->state_mutex); phylink_dbg(pl, "phy: %s setting supported %*pb advertising %*pb\n", -- 2.48.1

2 months, 4 weeks

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

Linux-stable-mirror