| Message ID | 20231109222251.a62811ebde9b.Ia70a49792c448867fd61b0234e1da507b0f75086@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | debugfs/wifi: locking fixes | expand |
On Sat, 2023-12-02 at 15:37 +0900, Sergey Senozhatsky wrote: > On (23/11/09 22:22), Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > When you take a lock in a debugfs handler but also try > > to remove the debugfs file under that lock, things can > > deadlock since the removal has to wait for all users > > to finish. > > > > Add lockdep annotations in debugfs_file_get()/_put() > > to catch such issues. > > So this triggers when I reset zram device (zsmalloc compiled with > CONFIG_ZSMALLOC_STAT). I shouldn't have put that into the rc, that was more or less an accident. I think I'll do a revert. > debugfs_create_file() and debugfs_remove_recursive() are called > under zram->init_lock, and zsmalloc never calls into zram code. > What I don't really get is where does the > `debugfs::classes -> zram->init_lock` > dependency come from? "debugfs:classes" means a file is being accessed and "classes" is the name, so that's static int zs_stats_size_show(struct seq_file *s, void *v) which uses seq_file, so there we have a seq_file lock. I think eventually the issue is that lockdep isn't telling the various seq_file instances apart, and you have one that's removed under lock (classes) and another one that's taking the lock (reset). Anyway, I'll send a revert, don't think this is ready yet. I was fixing the wireless debugfs lockdep and had used that to debug it. johannes
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 23bdfc126b5e..e499d38b1077 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -103,6 +103,12 @@ int debugfs_file_get(struct dentry *dentry) kfree(fsd); fsd = READ_ONCE(dentry->d_fsdata); } +#ifdef CONFIG_LOCKDEP + fsd->lock_name = kasprintf(GFP_KERNEL, "debugfs:%pd", dentry); + lockdep_register_key(&fsd->key); + lockdep_init_map(&fsd->lockdep_map, fsd->lock_name ?: "debugfs", + &fsd->key, 0); +#endif } /* @@ -119,6 +125,8 @@ int debugfs_file_get(struct dentry *dentry) if (!refcount_inc_not_zero(&fsd->active_users)) return -EIO; + lock_map_acquire_read(&fsd->lockdep_map); + return 0; } EXPORT_SYMBOL_GPL(debugfs_file_get); @@ -136,6 +144,8 @@ void debugfs_file_put(struct dentry *dentry) { struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata); + lock_map_release(&fsd->lockdep_map); + if (refcount_dec_and_test(&fsd->active_users)) complete(&fsd->active_users_drained); } diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 269bad87d552..a4c77aafb77b 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -241,6 +241,14 @@ static void debugfs_release_dentry(struct dentry *dentry) if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) return; + /* check it wasn't an automount or dir */ + if (fsd && fsd->real_fops) { +#ifdef CONFIG_LOCKDEP + lockdep_unregister_key(&fsd->key); + kfree(fsd->lock_name); +#endif + } + kfree(fsd); } @@ -742,6 +750,10 @@ static void __debugfs_file_removed(struct dentry *dentry) fsd = READ_ONCE(dentry->d_fsdata); if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) return; + + lock_map_acquire(&fsd->lockdep_map); + lock_map_release(&fsd->lockdep_map); + if (!refcount_dec_and_test(&fsd->active_users)) wait_for_completion(&fsd->active_users_drained); } diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h index f7c489b5a368..c7d61cfc97d2 100644 --- a/fs/debugfs/internal.h +++ b/fs/debugfs/internal.h @@ -7,6 +7,7 @@ #ifndef _DEBUGFS_INTERNAL_H_ #define _DEBUGFS_INTERNAL_H_ +#include <linux/lockdep.h> struct file_operations; @@ -23,6 +24,11 @@ struct debugfs_fsdata { struct { refcount_t active_users; struct completion active_users_drained; +#ifdef CONFIG_LOCKDEP + struct lockdep_map lockdep_map; + struct lock_class_key key; + char *lock_name; +#endif }; }; };