jffs2: fix lockdep warning

PATCH In review Helmut Grohne 3 participants 10 messages Apr 27, 2018 – Dec 16, 2018

Subsystems (2)

Memory technology devices (MTD) Journalling flash file system V2 (JFFS2)

lore

Share & tools

lore

Helmut Grohne

author

2018-04-27 10:31 UTC #0 lore raw

cover jffs2: fix lockdep warning

This patch series essentially fixes a false positive lockdep warning. The issue is known since 2012, referenced and fixed in the third patch. The other two patches subtly change the locking semantics on order to make the third patch feasible.

I would like thank Peter Zijlstra for his precise guiding on where to look. His first guess on what is going nailed it.

Helmut Grohne (3):
jffs2: avoid unnecessarily taking f->sem
jffs2: jffs2_iget defer f->sem acquisition
jffs2: put directories f->sem on a separate lockdep class

 fs/jffs2/fs.c        | 16 +++++++++++++++-
 fs/jffs2/readinode.c |  4 +---
 fs/jffs2/super.c     |  3 +++
 3 files changed, 19 insertions(+), 4 deletions(-)

Signature · 1 line

2.11.0

Helmut Grohne

author

2018-04-27 10:31 UTC #1 lore raw

PATCH 1/3 jffs2: avoid unnecessarily taking f->sem

jffs2_xattr_do_crccheck_inode creates a temporary struct jffs2_inode_info for handling it to jffs2_do_read_inode_internal. Formerly, jffs2_do_read_inode_internal released f->sem in the error path and that was why it had to be initialized, taken and released. As part of 7aaea7605c0 ("jffs2: fix unbalanced locking") locking was moved out of it.

In the relevant jffs2_xattr_do_crccheck_inode we own the only reference to f as we allocate it with kmalloc. Since nothing else has knowledge of it, there is no need to lock it. In particular, that saves us from thinking about the correct locking class.

We still need to initialize the mutex as jffs2_do_clear_inode insists on acquiring it.

Signed-off-by Helmut Grohne <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 fs/jffs2/readinode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/jffs2/readinode.c +1 -3

index 389ea53ea487..a6718fbfe1fd 100644
@@ -1382,12 +1382,10 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
 	if (!f)
 		return -ENOMEM;
 
-	mutex_init(&f->sem);
-	mutex_lock(&f->sem);
 	f->inocache = ic;
 
 	ret = jffs2_do_read_inode_internal(c, f, &n);
-	mutex_unlock(&f->sem);
+	mutex_init(&f->sem);
 	jffs2_do_clear_inode(c, f);
 	jffs2_xattr_do_crccheck_inode(c, ic);
 	kfree (f);

Signature · 1 line

2.11.0

Richard Weinberger

Reviewed-by on v1 1/3

2018-12-16 23:44 UTC #2 lore raw

Re: [PATCH 1/3] jffs2: avoid unnecessarily taking f->sem

Quoted 13 lines · Helmut Grohne

jffs2_xattr_do_crccheck_inode creates a temporary struct jffs2_inode_info for handling it to jffs2_do_read_inode_internal. Formerly, jffs2_do_read_inode_internal released f->sem in the error path and that was why it had to be initialized, taken and released. As part of 7aaea7605c0 ("jffs2: fix unbalanced locking") locking was moved out of it.
In the relevant jffs2_xattr_do_crccheck_inode we own the only reference to f as we allocate it with kmalloc. Since nothing else has knowledge of it, there is no need to lock it. In particular, that saves us from thinking about the correct locking class.

Quoted 5 lines

We still need to initialize the mutex as jffs2_do_clear_inode insists on acquiring it.
Signed-off-by Helmut Grohne <[email protected]>
Cc Peter Zijlstra <[email protected]>

Reviewed-by Richard Weinberger <[email protected]>

Signature · 2 lines

Thanks,
//richard

Helmut Grohne

author

2018-04-27 10:31 UTC #3 lore raw

PATCH 2/3 jffs2: jffs2_iget defer f->sem acquisition

In reply to Helmut Grohne

At the point where f->sem is taken, we know that we have a locked I_NEW inode. Since jffs2_iget is the only function processing I_NEW inodes and iget_locked blocks on I_NEW inodes, we know that we are the only user of the inode until we unlock_new_inode it. Since 7aaea7605c0 ("jffs2: fix unbalanced locking"), jffs2_do_read_inode no longer touches f->sem and does not make the inode visible to others either. Thus we can call it without f->sem acquired. Nextup, inode->i_mode is protected until unlock_new_inode.

After deferring the locking, we can decide upon the proper lock class depending on inode->i_mode.

Signed-off-by Helmut Grohne <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 fs/jffs2/fs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/jffs2/fs.c +3 -1

index eab04eca95a3..89a10b398d00 100644
@@ -270,13 +270,15 @@ struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
 	c = JFFS2_SB_INFO(inode->i_sb);
 
 	jffs2_init_inode_info(f);
-	mutex_lock(&f->sem);
 
 	ret = jffs2_do_read_inode(c, f, inode->i_ino, &latest_node);
 	if (ret)
 		goto error;
 
 	inode->i_mode = jemode_to_cpu(latest_node.mode);
+
+	mutex_lock(&f->sem);
+
 	i_uid_write(inode, je16_to_cpu(latest_node.uid));
 	i_gid_write(inode, je16_to_cpu(latest_node.gid));
 	inode->i_size = je32_to_cpu(latest_node.isize);

Signature · 1 line

2.11.0

Richard Weinberger

Reviewed-by on v1 2/3

2018-12-16 23:42 UTC #4 lore raw

Re: [PATCH 2/3] jffs2: jffs2_iget defer f->sem acquisition

Quoted 10 lines · Helmut Grohne

At the point where f->sem is taken, we know that we have a locked I_NEW inode. Since jffs2_iget is the only function processing I_NEW inodes and iget_locked blocks on I_NEW inodes, we know that we are the only user of the inode until we unlock_new_inode it. Since 7aaea7605c0 ("jffs2: fix unbalanced locking"), jffs2_do_read_inode no longer touches f->sem and does not make the inode visible to others either. Thus we can call it without f->sem acquired. Nextup, inode->i_mode is protected until unlock_new_inode.

Quoted 5 lines

After deferring the locking, we can decide upon the proper lock class depending on inode->i_mode.
Signed-off-by Helmut Grohne <[email protected]>
Cc Peter Zijlstra <[email protected]>

Reviewed-by Richard Weinberger <[email protected]>

Signature · 2 lines

Thanks,
//richard

Helmut Grohne

author

2018-04-27 10:31 UTC #5 lore raw

PATCH 3/3 jffs2: put directories f->sem on a separate lockdep class

In reply to Helmut Grohne

We get a lockdep warning if we read a directory (e.g. ls) and fault a page on a regular file:

[  181.206207] mmaptest/392 is trying to acquire lock:
[  181.211066]  (&f->sem){+.+.}, at: [<c01ff7d8>] jffs2_readpage+0x30/0x5c
[  181.217663]
[  181.217663] but task is already holding lock:
[  181.223477]  (&mm->mmap_sem){++++}, at: [<c04928e8>] do_page_fault+0xa8/0x454
[  181.230596]
[  181.230596] which lock already depends on the new lock.
[  181.230596]
[  181.238755]
[  181.238755] the existing dependency chain (in reverse order) is:
[  181.246219]
[  181.246219] -> #1 (&mm->mmap_sem){++++}:
[  181.251614]        __might_fault+0x90/0xc0
[  181.255694]        filldir+0xa4/0x1f4
[  181.259334]        jffs2_readdir+0xd8/0x1d4
[  181.263499]        iterate_dir+0x78/0x128
[  181.267490]        SyS_getdents+0x8c/0x12c
[  181.271576]        ret_fast_syscall+0x0/0x28
[  181.275818]
[  181.275818] -> #0 (&f->sem){+.+.}:
[  181.280690]        lock_acquire+0xfc/0x2b0
[  181.284763]        __mutex_lock+0x8c/0xb0c
[  181.288842]        mutex_lock_nested+0x2c/0x34
[  181.293272]        jffs2_readpage+0x30/0x5c
[  181.297438]        filemap_fault+0x600/0x73c
[  181.301690]        __do_fault+0x28/0xb8
[  181.305510]        handle_mm_fault+0x854/0xec0
[  181.309937]        do_page_fault+0x384/0x454
[  181.314188]        do_DataAbort+0x4c/0xc8
[  181.318181]        __dabt_usr+0x44/0x60
[  181.321999]        0xb6dffb02

Like with inode->i_rwsem, we recognize that this can never result in a dead lock, because we cannot page fault directory inodes and we cannot call getdents on non-directories. Thus we need different lockdep classes depending on the mode. This mirrors what lockdep_annotate_inode_mutex_key does to inodes.

Link https://www.spinics.net/lists/linux-rt-users/msg07644.html

Signed-off-by Helmut Grohne <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 fs/jffs2/fs.c    | 12 ++++++++++++
 fs/jffs2/super.c |  3 +++
 2 files changed, 15 insertions(+)

fs/jffs2/fs.c +12 -0

index 89a10b398d00..1717c464a21a 100644
@@ -26,6 +26,8 @@
 #include <linux/crc32.h>
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_dir_key;
+
 static int jffs2_flash_setup(struct jffs2_sb_info *c);
 
 int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)

@@ -277,6 +279,11 @@ struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
 
 	inode->i_mode = jemode_to_cpu(latest_node.mode);
 
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	if (S_ISDIR(inode->i_mode))
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_dir_key);
 	mutex_lock(&f->sem);
 
 	i_uid_write(inode, je16_to_cpu(latest_node.uid));

@@ -439,6 +446,11 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
 
 	f = JFFS2_INODE_INFO(inode);
 	jffs2_init_inode_info(f);
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	if (S_ISDIR(mode))
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_dir_key);
 	mutex_lock(&f->sem);
 
 	memset(ri, 0, sizeof(*ri));

fs/jffs2/super.c +3 -0

index 87bdf0f4cba1..f338a4f4bec5 100644
@@ -30,6 +30,8 @@
 #include "compr.h"
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_key;
+
 static void jffs2_put_super(struct super_block *);
 
 static struct kmem_cache *jffs2_inode_cachep;

@@ -60,6 +62,7 @@ static void jffs2_i_init_once(void *foo)
 	struct jffs2_inode_info *f = foo;
 
 	mutex_init(&f->sem);
+	lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key);
 	inode_init_once(&f->vfs_inode);
 }

Signature · 1 line

2.11.0

David Woodhouse

maintainer

2018-04-27 12:54 UTC #6 lore raw

Re: [PATCH 3/3] jffs2: put directories f->sem on a separate lockdep class

Quoted 9 lines · Helmut Grohne

fs/jffs2/super.c +0 -0
@@ -60,6 +62,7 @@ static void jffs2_i_init_once(void *foo)
         struct jffs2_inode_info *f = foo;
mutex_init(&f->sem);
+ lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key); inode_init_once(&f->vfs_inode);
}

The point in init_once is that inode structures can get reused. So if it gets used the first time for a directory, then re-used for a file, nothing ever sets the lockdep class back to jffs2_inode_info_sem_key.

Other than that, this all looks good; thanks!

Helmut Grohne

author

2018-04-30 10:31 UTC #7 lore raw

PATCH v2 1/1 jffs2: put directories f->sem on a separate lockdep class

We get a lockdep warning if we read a directory (e.g. ls) and fault a page on a regular file:

[  181.206207] mmaptest/392 is trying to acquire lock:
[  181.211066]  (&f->sem){+.+.}, at: [<c01ff7d8>] jffs2_readpage+0x30/0x5c
[  181.217663]
[  181.217663] but task is already holding lock:
[  181.223477]  (&mm->mmap_sem){++++}, at: [<c04928e8>] do_page_fault+0xa8/0x454
[  181.230596]
[  181.230596] which lock already depends on the new lock.
[  181.230596]
[  181.238755]
[  181.238755] the existing dependency chain (in reverse order) is:
[  181.246219]
[  181.246219] -> #1 (&mm->mmap_sem){++++}:
[  181.251614]        __might_fault+0x90/0xc0
[  181.255694]        filldir+0xa4/0x1f4
[  181.259334]        jffs2_readdir+0xd8/0x1d4
[  181.263499]        iterate_dir+0x78/0x128
[  181.267490]        SyS_getdents+0x8c/0x12c
[  181.271576]        ret_fast_syscall+0x0/0x28
[  181.275818]
[  181.275818] -> #0 (&f->sem){+.+.}:
[  181.280690]        lock_acquire+0xfc/0x2b0
[  181.284763]        __mutex_lock+0x8c/0xb0c
[  181.288842]        mutex_lock_nested+0x2c/0x34
[  181.293272]        jffs2_readpage+0x30/0x5c
[  181.297438]        filemap_fault+0x600/0x73c
[  181.301690]        __do_fault+0x28/0xb8
[  181.305510]        handle_mm_fault+0x854/0xec0
[  181.309937]        do_page_fault+0x384/0x454
[  181.314188]        do_DataAbort+0x4c/0xc8
[  181.318181]        __dabt_usr+0x44/0x60
[  181.321999]        0xb6dffb02

Link https://www.spinics.net/lists/linux-rt-users/msg07644.html

Signed-off-by Helmut Grohne <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
I have reworked only this last patch of the series to avoid sending three extra mails. If you prefer the full series, I can resend it.

Changing lock classes after using a lock is not a common thing to do, yet this patch does exactly that. Thus I opted for performing two extra tests to gain more confidence in the correctness of this approach:

* I tried adding a BUG_ON(!mutex_is_locked(&f->sem)) to before the lock class change to validate that the mutex on the I_NEW inode cannot be held. This call did not error out in my tests.

* I tried adding a mutex_clear(&f->sem) to jffs2_do_clear_inode and reinitialized it before setting the mutex class to validate that it really isn't used in the relevant time frame. This did not cause lockdep to complain in my tests.

Changes in v2:
- Initialize the lock class on every inode initialization rather than just the initial one in jffs2_i_init_once. (Thanks to David Woodhouse for pointing at this issue.)
- Properly wrap the commit message to make checkpatch.pl fully happy.

---
 fs/jffs2/fs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/jffs2/fs.c +13 -0

index 89a10b398d00..9bc3cd204732 100644
@@ -26,6 +26,9 @@
 #include <linux/crc32.h>
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_key,
+	 jffs2_inode_info_sem_dir_key;
+
 static int jffs2_flash_setup(struct jffs2_sb_info *c);
 
 int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)

@@ -277,6 +280,11 @@ struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
 
 	inode->i_mode = jemode_to_cpu(latest_node.mode);
 
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	lockdep_set_class(&f->sem, S_ISDIR(inode->i_mode) ?
+		&jffs2_inode_info_sem_dir_key : &jffs2_inode_info_sem_key);
 	mutex_lock(&f->sem);
 
 	i_uid_write(inode, je16_to_cpu(latest_node.uid));

@@ -439,6 +447,11 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
 
 	f = JFFS2_INODE_INFO(inode);
 	jffs2_init_inode_info(f);
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	lockdep_set_class(&f->sem, S_ISDIR(inode->i_mode) ?
+		&jffs2_inode_info_sem_dir_key : &jffs2_inode_info_sem_key);
 	mutex_lock(&f->sem);
 
 	memset(ri, 0, sizeof(*ri));

Signature · 1 line

2.11.0

Richard Weinberger

2018-12-16 10:45 UTC #8 lore raw

Re: [PATCH v2] jffs2: put directories f->sem on a separate lockdep class

Quoted 100 lines · Helmut Grohne

We get a lockdep warning if we read a directory (e.g. ls) and fault a page on a regular file:
[  181.206207] mmaptest/392 is trying to acquire lock:
[  181.211066]  (&f->sem){+.+.}, at: [<c01ff7d8>] jffs2_readpage+0x30/0x5c
[  181.217663]
[  181.217663] but task is already holding lock:
[  181.223477]  (&mm->mmap_sem){++++}, at: [<c04928e8>] do_page_fault+0xa8/0x454
[  181.230596]
[  181.230596] which lock already depends on the new lock.
[  181.230596]
[  181.238755]
[  181.238755] the existing dependency chain (in reverse order) is:
[  181.246219]
[  181.246219] -> #1 (&mm->mmap_sem){++++}:
[  181.251614]        __might_fault+0x90/0xc0
[  181.255694]        filldir+0xa4/0x1f4
[  181.259334]        jffs2_readdir+0xd8/0x1d4
[  181.263499]        iterate_dir+0x78/0x128
[  181.267490]        SyS_getdents+0x8c/0x12c
[  181.271576]        ret_fast_syscall+0x0/0x28
[  181.275818]
[  181.275818] -> #0 (&f->sem){+.+.}:
[  181.280690]        lock_acquire+0xfc/0x2b0
[  181.284763]        __mutex_lock+0x8c/0xb0c
[  181.288842]        mutex_lock_nested+0x2c/0x34
[  181.293272]        jffs2_readpage+0x30/0x5c
[  181.297438]        filemap_fault+0x600/0x73c
[  181.301690]        __do_fault+0x28/0xb8
[  181.305510]        handle_mm_fault+0x854/0xec0
[  181.309937]        do_page_fault+0x384/0x454
[  181.314188]        do_DataAbort+0x4c/0xc8
[  181.318181]        __dabt_usr+0x44/0x60
[  181.321999]        0xb6dffb02
Like with inode->i_rwsem, we recognize that this can never result in a dead lock, because we cannot page fault directory inodes and we cannot call getdents on non-directories. Thus we need different lockdep classes depending on the mode. This mirrors what lockdep_annotate_inode_mutex_key does to inodes.
Link https://www.spinics.net/lists/linux-rt-users/msg07644.html
Signed-off-by Helmut Grohne <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
I have reworked only this last patch of the series to avoid sending three extra mails. If you prefer the full series, I can resend it.
Changing lock classes after using a lock is not a common thing to do, yet this patch does exactly that. Thus I opted for performing two extra tests to gain more confidence in the correctness of this approach:
* I tried adding a BUG_ON(!mutex_is_locked(&f->sem)) to before the lock class change to validate that the mutex on the I_NEW inode cannot be held. This call did not error out in my tests.
* I tried adding a mutex_clear(&f->sem) to jffs2_do_clear_inode and reinitialized it before setting the mutex class to validate that it really isn't used in the relevant time frame. This did not cause lockdep to complain in my tests.
Changes in v2:
- Initialize the lock class on every inode initialization rather than just the initial one in jffs2_i_init_once. (Thanks to David Woodhouse for pointing at this issue.)
- Properly wrap the commit message to make checkpatch.pl fully happy.
---
 fs/jffs2/fs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
fs/jffs2/fs.c +8 -0
index 89a10b398d00..9bc3cd204732 100644
@@ -26,6 +26,9 @@
 #include <linux/crc32.h>
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_key,
+        jffs2_inode_info_sem_dir_key;
+
 static int jffs2_flash_setup(struct jffs2_sb_info *c);
 
 int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)
@@ -277,6 +280,11 @@ struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
 
        inode->i_mode = jemode_to_cpu(latest_node.mode);
 
+       /* Mirror lock class of inode->i_rwsem, see
+        * lockdep_annotate_inode_mutex_key.
+        */
+       lockdep_set_class(&f->sem, S_ISDIR(inode->i_mode) ?
+               &jffs2_inode_info_sem_dir_key : &jffs2_inode_info_sem_key);
        mutex_lock(&f->sem);
 
        i_uid_write(inode, je16_to_cpu(latest_node.uid));
@@ -439,6 +447,11 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
 

fs/jffs2/fs.c struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r · line 457

Quoted 10 lines

fs/jffs2/fs.c +5 -0

        f = JFFS2_INODE_INFO(inode);
        jffs2_init_inode_info(f);
+       /* Mirror lock class of inode->i_rwsem, see
+        * lockdep_annotate_inode_mutex_key.
+        */
+       lockdep_set_class(&f->sem, S_ISDIR(inode->i_mode) ?
+               &jffs2_inode_info_sem_dir_key : &jffs2_inode_info_sem_key);
        mutex_lock(&f->sem);
 
        memset(ri, 0, sizeof(*ri));

I'm currently going through old jffs2 patches and found this one. It looks correct and sane, therefore I plan to merge it via the MTD tree.

David, if you have objections, please let me know. :-)

Signature · 2 lines

Thanks,
//richard

Richard Weinberger

2018-12-16 23:40 UTC #9 lore raw

Re: [PATCH v2] jffs2: put directories f->sem on a separate lockdep class

In reply to Helmut Grohne

fs/jffs2/fs.c struct inode *jffs2_iget(struct super_block *sb, unsigned long ino) · line 287

Quoted 5 lines · Helmut Grohne

fs/jffs2/fs.c +5 -0

+       /* Mirror lock class of inode->i_rwsem, see
+        * lockdep_annotate_inode_mutex_key.
+        */
+       lockdep_set_class(&f->sem, S_ISDIR(inode->i_mode) ?
+               &jffs2_inode_info_sem_dir_key : &jffs2_inode_info_sem_key);

Please use a plain if/else branch.
lockdep_set_class() uses the second parameter verbatim in its reports.

Signature · 2 lines

Thanks,
//richard