Messages in this thread | | | Date | Wed, 10 Aug 2022 11:34:58 +0200 | Subject | Re: [PATCH] mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock | From | Vlastimil Babka <> |
| |
On 8/9/22 22:59, 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 --+ > | | > +---------------------------------------------------------+
This sounded familiar and I've found a thread from January:
https://lore.kernel.org/all/388098b2c03fbf0a732834fc01b2d875c335bc49.1642170196.git.lucien.xin@gmail.com/
But that seemed to be specific to RHEL-8 RT kernel and not reproduced with mainline. Is it different this time? Can you share the splats?
> 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); > } > } > > @@ -437,20 +457,10 @@ static int shutdown_cache(struct kmem_cache *s) > list_del(&s->list); > > if (s->flags & SLAB_TYPESAFE_BY_RCU) { > -#ifdef SLAB_SUPPORTS_SYSFS > - sysfs_slab_unlink(s); > -#endif > list_add_tail(&s->list, &slab_caches_to_rcu_destroy); > - schedule_work(&slab_caches_to_rcu_destroy_work); > } else { > kfence_shutdown_cache(s); > debugfs_slab_release(s); > -#ifdef SLAB_SUPPORTS_SYSFS > - sysfs_slab_unlink(s); > - sysfs_slab_release(s); > -#else > - slab_kmem_cache_release(s); > -#endif > } > > return 0; > @@ -465,14 +475,16 @@ void slab_kmem_cache_release(struct kmem_cache *s) > > void kmem_cache_destroy(struct kmem_cache *s) > { > + int refcnt; > + > if (unlikely(!s) || !kasan_check_byte(s)) > return; > > cpus_read_lock(); > mutex_lock(&slab_mutex); > > - s->refcount--; > - if (s->refcount) > + refcnt = --s->refcount; > + if (refcnt) > goto out_unlock; > > WARN(shutdown_cache(s), > @@ -481,6 +493,8 @@ void kmem_cache_destroy(struct kmem_cache *s) > out_unlock: > mutex_unlock(&slab_mutex); > cpus_read_unlock(); > + if (!refcnt) > + kmem_cache_release(s, false); > } > EXPORT_SYMBOL(kmem_cache_destroy); >
| |