Messages in this thread | | | Date | Tue, 9 Aug 2022 19:05:37 -0400 | Subject | Re: [PATCH] mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock | From | Waiman Long <> |
| |
On 8/9/22 18:25, Roman Gushchin wrote: > On Tue, Aug 09, 2022 at 04:59:01PM -0400, Waiman Long wrote: >> A circular locking problem is reported by lockdep due to the following >> circular locking dependency. >> >> +--> cpu_hotplug_lock --> slab_mutex --> kn->active#126 --+ >> | | >> +---------------------------------------------------------+ >> >> One way to break this circular locking chain is to avoid holding >> cpu_hotplug_lock and slab_mutex while deleting the kobject in >> sysfs_slab_unlink() which should be equivalent to doing a write_lock >> and write_unlock pair of the kn->active virtual lock. >> >> Since the kobject structures are not protected by slab_mutex or the >> cpu_hotplug_lock, we can certainly release those locks before doing >> the delete operation. >> >> Move sysfs_slab_unlink() and sysfs_slab_release() to the newly >> created kmem_cache_release() and call it outside the slab_mutex & >> cpu_hotplug_lock critical sections. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> mm/slab_common.c | 48 +++++++++++++++++++++++++++++++----------------- >> 1 file changed, 31 insertions(+), 17 deletions(-) >> >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index 17996649cfe3..9274fb03563e 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -392,6 +392,30 @@ kmem_cache_create(const char *name, unsigned int size, unsigned int align, >> } >> EXPORT_SYMBOL(kmem_cache_create); >> >> +#ifdef SLAB_SUPPORTS_SYSFS >> +/* >> + * For a given kmem_cache, kmem_cache_destroy() should only be called >> + * once or there will be a use-after-free problem. The actual deletion >> + * and release of the kobject does not need slab_mutex or cpu_hotplug_lock >> + * protection. So they are now done without holding those locks. >> + */ >> +static void kmem_cache_release(struct kmem_cache *s, bool workfn) >> +{ >> + if (!workfn) >> + sysfs_slab_unlink(s); >> + >> + if (workfn || !(s->flags & SLAB_TYPESAFE_BY_RCU)) >> + sysfs_slab_release(s); >> + else >> + schedule_work(&slab_caches_to_rcu_destroy_work); >> +} >> +#else >> +static inline void kmem_cache_release(struct kmem_cache *s, bool workfn) >> +{ >> + slab_kmem_cache_release(s); >> +} >> +#endif >> + >> static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) >> { >> LIST_HEAD(to_destroy); >> @@ -418,11 +442,7 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) >> list_for_each_entry_safe(s, s2, &to_destroy, list) { >> debugfs_slab_release(s); >> kfence_shutdown_cache(s); >> -#ifdef SLAB_SUPPORTS_SYSFS >> - sysfs_slab_release(s); >> -#else >> - slab_kmem_cache_release(s); >> -#endif >> + kmem_cache_release(s, true); > Hi Waiman! > > As I understand, with SLAB_SUPPORTS_SYSFS kmem_cache_release() can effectively call > into itself: first it's called with workfn == false from shutdown_cache() and > then optionally it's scheduled to call itself from a work context with > workfn == true just to call sysfs_slab_release(). Is it right? > > If !SLAB_SUPPORTS_SYSFS, shutdown_cache() optionally adds kmem_cache to the > slab_caches_to_rcu_destroy list and calls kmem_cache_release(s, false) == > slab_kmem_cache_release(). How it's then removed from the list? > > Overall the patch is a bit hard to follow (not like this code was easy to read > before, so can't blame the patch). But I wonder if it will make things simpler > to decouple kmem_cache_release(workfn == true) and kmem_cache_release(workfn == false) > into 2 different helpers? Or at least add a bold comment on how things are supposed > to work. > > Thanks!
You are right. I agree that it can be hard to read. Simpler is always better. Will post a v2 with the change suggested.
Thanks, Longman
| |