[PATCH v3 0/3] jffs2: fix lockdep warning

[PATCH v3 0/3] jffs2: fix lockdep warning

[Helmut Grohne]

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. I would also like to thank David
Woodhouse and Richard Weinberger for their reviews.

Changes in v3:
 - Avoid ?: operator (requested by Richard Weinberger).
 - Add Reviewed-by tags from Richard Weinberger.
 - Rebase patch series on v4.20-rc7.
 - Update my email address.

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.

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        | 21 ++++++++++++++++++++-
 fs/jffs2/readinode.c |  4 +---
 2 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.11.0

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

[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.

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

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Richard Weinberger <richard@nod.at>
---
 fs/jffs2/readinode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 389ea53ea487..a6718fbfe1fd 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -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);
-- 
2.11.0

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

[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 <helmut.grohne@intenta.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Richard Weinberger <richard@nod.at>
---
 fs/jffs2/fs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index eab04eca95a3..89a10b398d00 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -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);
-- 
2.11.0

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

[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 <helmut.grohne@intenta.de>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 fs/jffs2/fs.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 89a10b398d00..d4d691488a19 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -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,13 @@ 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);
+	else
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key);
 	mutex_lock(&f->sem);
 
 	i_uid_write(inode, je16_to_cpu(latest_node.uid));
@@ -439,6 +449,13 @@ 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(inode->i_mode))
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_dir_key);
+	else
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key);
 	mutex_lock(&f->sem);
 
 	memset(ri, 0, sizeof(*ri));
-- 
2.11.0