[PATCH] jffs2: prevent xattr_sem -> fs_reclaim deadlock

[PATCH] jffs2: prevent xattr_sem -> fs_reclaim deadlock

[Shuai Liu]

do_jffs2_setxattr() and several other xattr paths allocate memory
(e.g. jffs2_alloc_xattr_datum() via kmem_cache_alloc() inside
create_xattr_datum()) and write to flash while holding
c->xattr_sem, using GFP_KERNEL.  Such allocations may enter direct
reclaim and take fs_reclaim, establishing the
xattr_sem -> fs_reclaim dependency.

kswapd, on the other hand, holds fs_reclaim while shrinking
dcaches and inodes: shrink_slab() -> prune_dcache_sb() ->
dentry_kill() -> evict() -> jffs2_do_clear_inode() ->
jffs2_xattr_delete_inode() takes down_write(&c->xattr_sem),
giving the reverse fs_reclaim -> xattr_sem dependency.  The
resulting lock cycle can deadlock:

    CPU0                    CPU1
    ----                    ----
    lock(xattr_sem)         lock(fs_reclaim)
    lock(fs_reclaim)        lock(xattr_sem)

syzbot reports exactly this circular dependency with lockdep [1]:
kswapd0 acquiring xattr_sem in jffs2_xattr_delete_inode() while
holding fs_reclaim, with the recorded chain showing
do_jffs2_setxattr() allocating (jffs2_alloc_xattr_datum() ->
kmem_cache_alloc() -> fs_reclaim_acquire) while holding
xattr_sem.

Fix it by wrapping every critical section that holds c->xattr_sem
and can allocate memory or write flash with
memalloc_nofs_save()/memalloc_nofs_restore(), so allocations made
under xattr_sem never enter direct reclaim.  This cuts the
xattr_sem -> fs_reclaim edge; reclaim contexts taking xattr_sem
remain harmless.  The covered paths are:

  - jffs2_xattr_delete_inode(): delete_xattr_ref() writes flash
  - check_xattr_ref_inode(): load_xattr_datum() allocates,
    delete_xattr_ref() writes flash
  - the write-lock upgrade sections of jffs2_listxattr() and
    do_jffs2_getxattr(): load_xattr_datum() allocates
  - do_jffs2_setxattr(): create_xattr_datum() allocates,
    save_xattr_ref() writes flash (both lock sections)
  - jffs2_garbage_collect_xattr_datum() /
    jffs2_garbage_collect_xattr_ref(): jffs2_reserve_space_gc()
    may trigger GC, save_xattr_datum()/save_xattr_ref() write
    flash
  - jffs2_verify_xattr(): do_verify_xattr_datum() reads flash

jffs2_xattr_free_inode() is deliberately left alone: it only
frees memory and cannot allocate.

A related but distinct cycle between fs_reclaim and &f->sem
(jffs2_do_read_inode() allocates while holding f->sem during
iget) also exists; it is a separate issue not addressed here.

Verified with the syzbot reproducer under QEMU (mtdram + JFFS2 +
OverlayFS, CONFIG_PROVE_LOCKING=y): without this patch lockdep
reports the xattr_sem <-> fs_reclaim cycle reproduced above;
with this patch that warning no longer appears.

[1] https://syzkaller.appspot.com/bug?id=eefefd759ef3b48ddad4da086ff8f4b9194576f9

Reported-by: syzbot+fdfd8fdf716c90baef0f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fdfd8fdf716c90baef0f
Signed-off-by: Shuai Liu <847807867@qq.com>
---
 fs/jffs2/xattr.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
index 6b4f68593c29..7b873fe2e08d 100644
--- a/fs/jffs2/xattr.c
+++ b/fs/jffs2/xattr.c
@@ -15,6 +15,7 @@
 
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/sched/mm.h>
 #include <linux/fs.h>
 #include <linux/time.h>
 #include <linux/pagemap.h>
@@ -598,10 +599,20 @@ void jffs2_xattr_delete_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache
 	/* It's called from jffs2_evict_inode() on inode removing.
 	   When an inode with XATTR is removed, those XATTRs must be removed. */
 	struct jffs2_xattr_ref *ref, *_ref;
+	unsigned int nofs;
 
 	if (!ic || ic->pino_nlink > 0)
 		return;
 
+	/*
+	 * delete_xattr_ref() below writes flash nodes.  This function runs
+	 * from inode eviction, i.e. memory reclaim may be waiting for this
+	 * lock.  Keep allocations under xattr_sem out of direct reclaim so
+	 * that holders of xattr_sem never wait on fs_reclaim -- otherwise
+	 * reclaim contexts taking xattr_sem while holding fs_reclaim can
+	 * deadlock with the setxattr() paths doing the reverse.
+	 */
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
 	for (ref = ic->xref; ref; ref = _ref) {
 		_ref = ref->next;
@@ -609,6 +620,7 @@ void jffs2_xattr_delete_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache
 	}
 	ic->xref = NULL;
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 }
 
 void jffs2_xattr_free_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *ic)
@@ -638,10 +650,12 @@ static int check_xattr_ref_inode(struct jffs2_sb_info *c, struct jffs2_inode_cac
 	 * one will be removed.
 	 */
 	struct jffs2_xattr_ref *ref, *cmp, **pref, **pcmp;
+	unsigned int nofs;
 	int rc = 0;
 
 	if (likely(ic->flags & INO_FLAGS_XATTR_CHECKED))
 		return 0;
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
  retry:
 	rc = 0;
@@ -683,6 +697,7 @@ static int check_xattr_ref_inode(struct jffs2_sb_info *c, struct jffs2_inode_cac
 	ic->flags |= INO_FLAGS_XATTR_CHECKED;
  out:
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 
 	return rc;
 }
@@ -973,6 +988,7 @@ ssize_t jffs2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	const char *prefix;
 	ssize_t prefix_len, len, rc;
 	int retry = 0;
+	unsigned int nofs = 0;
 
 	rc = check_xattr_ref_inode(c, ic);
 	if (unlikely(rc))
@@ -990,6 +1006,7 @@ ssize_t jffs2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 				retry = 1;
 				up_read(&c->xattr_sem);
 				down_write(&c->xattr_sem);
+				nofs = memalloc_nofs_save();
 				goto retry;
 			} else {
 				rc = load_xattr_datum(c, xd);
@@ -1027,6 +1044,7 @@ ssize_t jffs2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		up_read(&c->xattr_sem);
 	} else {
 		up_write(&c->xattr_sem);
+		memalloc_nofs_restore(nofs);
 	}
 	return rc;
 }
@@ -1040,6 +1058,7 @@ int do_jffs2_getxattr(struct inode *inode, int xprefix, const char *xname,
 	struct jffs2_xattr_datum *xd;
 	struct jffs2_xattr_ref *ref, **pref;
 	int rc, retry = 0;
+	unsigned int nofs = 0;
 
 	rc = check_xattr_ref_inode(c, ic);
 	if (unlikely(rc))
@@ -1059,6 +1078,7 @@ int do_jffs2_getxattr(struct inode *inode, int xprefix, const char *xname,
 				retry = 1;
 				up_read(&c->xattr_sem);
 				down_write(&c->xattr_sem);
+				nofs = memalloc_nofs_save();
 				goto retry;
 			} else {
 				rc = load_xattr_datum(c, xd);
@@ -1089,6 +1109,7 @@ int do_jffs2_getxattr(struct inode *inode, int xprefix, const char *xname,
 		up_read(&c->xattr_sem);
 	} else {
 		up_write(&c->xattr_sem);
+		memalloc_nofs_restore(nofs);
 	}
 	return rc;
 }
@@ -1102,6 +1123,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
 	struct jffs2_xattr_datum *xd;
 	struct jffs2_xattr_ref *ref, *newref, **pref;
 	uint32_t length, request;
+	unsigned int nofs;
 	int rc;
 
 	rc = check_xattr_ref_inode(c, ic);
@@ -1120,6 +1142,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
 	}
 
 	/* Find existing xattr */
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
  retry:
 	for (ref=ic->xref, pref=&ic->xref; ref; pref=&ref->next, ref=ref->next) {
@@ -1189,6 +1212,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
 		JFFS2_WARNING("jffs2_reserve_space()=%d, request=%u\n", rc, request);
 		unrefer_xattr_datum(c, xd);
 		up_write(&c->xattr_sem);
+		memalloc_nofs_restore(nofs);
 		return rc;
 	}
 	if (ref)
@@ -1206,6 +1230,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
 	}
  out:
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 	jffs2_complete_reservation(c);
 	return rc;
 }
@@ -1226,8 +1251,10 @@ int jffs2_garbage_collect_xattr_datum(struct jffs2_sb_info *c, struct jffs2_xatt
 				      struct jffs2_raw_node_ref *raw)
 {
 	uint32_t totlen, length, old_ofs;
+	unsigned int nofs;
 	int rc = 0;
 
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
 	if (xd->node != raw)
 		goto out;
@@ -1255,6 +1282,7 @@ int jffs2_garbage_collect_xattr_datum(struct jffs2_sb_info *c, struct jffs2_xatt
 	if (!rc)
 		jffs2_mark_node_obsolete(c, raw);
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 	return rc;
 }
 
@@ -1262,8 +1290,10 @@ int jffs2_garbage_collect_xattr_ref(struct jffs2_sb_info *c, struct jffs2_xattr_
 				    struct jffs2_raw_node_ref *raw)
 {
 	uint32_t totlen, length, old_ofs;
+	unsigned int nofs;
 	int rc = 0;
 
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
 	BUG_ON(!ref->node);
 
@@ -1289,6 +1319,7 @@ int jffs2_garbage_collect_xattr_ref(struct jffs2_sb_info *c, struct jffs2_xattr_
 	if (!rc)
 		jffs2_mark_node_obsolete(c, raw);
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 	return rc;
 }
 
@@ -1298,8 +1329,10 @@ int jffs2_verify_xattr(struct jffs2_sb_info *c)
 	struct jffs2_eraseblock *jeb;
 	struct jffs2_raw_node_ref *raw;
 	uint32_t totlen;
+	unsigned int nofs;
 	int rc;
 
+	nofs = memalloc_nofs_save();
 	down_write(&c->xattr_sem);
 	list_for_each_entry_safe(xd, _xd, &c->xattr_unchecked, xindex) {
 		rc = do_verify_xattr_datum(c, xd);
@@ -1322,6 +1355,7 @@ int jffs2_verify_xattr(struct jffs2_sb_info *c)
 		spin_unlock(&c->erase_completion_lock);
 	}
 	up_write(&c->xattr_sem);
+	memalloc_nofs_restore(nofs);
 	return list_empty(&c->xattr_unchecked) ? 1 : 0;
 }
 
-- 
2.43.0