| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8f689011-4f35-41a1-a4cb-b81c833533f7@suse.com>
Date: Tue, 24 Jun 2025 08:38:05 +0800
From: Heming Zhao <heming.zhao@...e.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
Mark Fasheh <mark@...heh.com>, Joel Becker <jlbec@...lplan.org>,
Joseph Qi <joseph.qi@...ux.alibaba.com>, ocfs2-devel@...ts.linux.dev,
LKML <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] ocfs2: embed actual values into ocfs2_sysfile_lock_key
names
Hello,
Just from your code logic, the switch is unnecessary, and converting the input parameter into an "unsigned int" type is sufficient.
e.g.: (not test)
unsigned int type = args->fi_sysfile_type;
if (args->fi_sysfile_type != 0)
lockdep_set_class(&inode->i_rwsem,
&ocfs2_sysfile_lock_key[type]);
Thanks
- Heming
On 6/23/25 22:54, Tetsuo Handa wrote:
> Since lockdep_set_class() uses stringified key name via macro, calling
> lockdep_set_class() with an array causes lockdep warning messages to
> report variable name than actual index number.
>
> Change ocfs2_init_locked_inode() to pass actual index number for better
> readability of lockdep reports. This patch does not change behavior.
>
>
> Before:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[args->fi_sysfile_type] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
>
> *** DEADLOCK ***
>
> After:
>
> Chain exists of:
> &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE] --> jbd2_handle --> &oi->ip_xattr_sem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&oi->ip_xattr_sem);
> lock(jbd2_handle);
> lock(&oi->ip_xattr_sem);
> lock(&ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
>
> *** DEADLOCK ***
>
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> ---
> fs/ocfs2/inode.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 12e5d1f73325..14bf440ea4df 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -50,8 +50,6 @@ struct ocfs2_find_inode_args
> unsigned int fi_sysfile_type;
> };
>
> -static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> -
> static int ocfs2_read_locked_inode(struct inode *inode,
> struct ocfs2_find_inode_args *args);
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque);
> @@ -250,14 +248,77 @@ static int ocfs2_find_actor(struct inode *inode, void *opaque)
> static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> {
> struct ocfs2_find_inode_args *args = opaque;
> +#ifdef CONFIG_LOCKDEP
> + static struct lock_class_key ocfs2_sysfile_lock_key[NUM_SYSTEM_INODES];
> static struct lock_class_key ocfs2_quota_ip_alloc_sem_key,
> ocfs2_file_ip_alloc_sem_key;
> +#endif
>
> inode->i_ino = args->fi_ino;
> OCFS2_I(inode)->ip_blkno = args->fi_blkno;
> - if (args->fi_sysfile_type != 0)
> +#ifdef CONFIG_LOCKDEP
> + switch (args->fi_sysfile_type) {
> + case BAD_BLOCK_SYSTEM_INODE:
> + break;
> + case GLOBAL_INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case SLOT_MAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[SLOT_MAP_SYSTEM_INODE]);
> + break;
> + case HEARTBEAT_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[HEARTBEAT_SYSTEM_INODE]);
> + break;
> + case GLOBAL_BITMAP_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GLOBAL_BITMAP_SYSTEM_INODE]);
> + break;
> + case USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + case ORPHAN_DIR_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[ORPHAN_DIR_SYSTEM_INODE]);
> + break;
> + case EXTENT_ALLOC_SYSTEM_INODE:
> lockdep_set_class(&inode->i_rwsem,
> - &ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
> + &ocfs2_sysfile_lock_key[EXTENT_ALLOC_SYSTEM_INODE]);
> + break;
> + case INODE_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[INODE_ALLOC_SYSTEM_INODE]);
> + break;
> + case JOURNAL_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[JOURNAL_SYSTEM_INODE]);
> + break;
> + case LOCAL_ALLOC_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_ALLOC_SYSTEM_INODE]);
> + break;
> + case TRUNCATE_LOG_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[TRUNCATE_LOG_SYSTEM_INODE]);
> + break;
> + case LOCAL_USER_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_USER_QUOTA_SYSTEM_INODE]);
> + break;
> + case LOCAL_GROUP_QUOTA_SYSTEM_INODE:
> + lockdep_set_class(&inode->i_rwsem,
> + &ocfs2_sysfile_lock_key[LOCAL_GROUP_QUOTA_SYSTEM_INODE]);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown sysfile type %d\n", args->fi_sysfile_type);
> + }
> if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
> args->fi_sysfile_type == LOCAL_USER_QUOTA_SYSTEM_INODE ||
> @@ -267,6 +328,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> else
> lockdep_set_class(&OCFS2_I(inode)->ip_alloc_sem,
> &ocfs2_file_ip_alloc_sem_key);
> +#endif
>
> return 0;
> }
Powered by blists - more mailing lists