Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

ubifs: Check @c->dirty_[n|p]n_cnt and @c->nroot state under @c->lp_mutex

The checking of @c->nroot->flags and @c->dirty_[n|p]n_cnt in function
nothing_to_commit() is not atomic, which could be raced with modifying
of lpt, for example:
P1 P2 P3
run_gc
ubifs_garbage_collect
do_commit
ubifs_return_leb
ubifs_lpt_lookup_dirty
dirty_cow_nnode
do_commit
nothing_to_commit
if (test_bit(DIRTY_CNODE, &c->nroot->flags)
// false
test_and_set_bit(DIRTY_CNODE, &nnode->flags)
c->dirty_nn_cnt += 1
ubifs_assert(c, c->dirty_nn_cnt == 0)
// false !

Fetch a reproducer in Link:
UBIFS error (ubi0:0 pid 2747): ubifs_assert_failed
UBIFS assert failed: c->dirty_pn_cnt == 0, in fs/ubifs/commit.c
Call Trace:
ubifs_ro_mode+0x58/0x70 [ubifs]
ubifs_assert_failed+0x6a/0x90 [ubifs]
do_commit+0x5b7/0x930 [ubifs]
ubifs_run_commit+0xc6/0x1a0 [ubifs]
ubifs_sync_fs+0xd8/0x110 [ubifs]
sync_filesystem+0xb4/0x120
do_syscall_64+0x6f/0x140

Fix it by checking @c->dirty_[n|p]n_cnt and @c->nroot state with
@c->lp_mutex locked.

Fixes: 944fdef52ca9 ("UBIFS: do not start the commit if there is nothing to commit")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218162
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: Richard Weinberger <richard@nod.at>

authored by

Zhihao Cheng and committed by
Richard Weinberger
c07a4dab 2ba5b489

+12 -1
+12 -1
fs/ubifs/commit.c
··· 70 70 return 0; 71 71 72 72 /* 73 + * Increasing @c->dirty_pn_cnt/@c->dirty_nn_cnt and marking 74 + * nnodes/pnodes as dirty in run_gc() could race with following 75 + * checking, which leads inconsistent states between @c->nroot 76 + * and @c->dirty_pn_cnt/@c->dirty_nn_cnt, holding @c->lp_mutex 77 + * to avoid that. 78 + */ 79 + mutex_lock(&c->lp_mutex); 80 + /* 73 81 * Even though the TNC is clean, the LPT tree may have dirty nodes. For 74 82 * example, this may happen if the budgeting subsystem invoked GC to 75 83 * make some free space, and the GC found an LEB with only dirty and 76 84 * free space. In this case GC would just change the lprops of this 77 85 * LEB (by turning all space into free space) and unmap it. 78 86 */ 79 - if (c->nroot && test_bit(DIRTY_CNODE, &c->nroot->flags)) 87 + if (c->nroot && test_bit(DIRTY_CNODE, &c->nroot->flags)) { 88 + mutex_unlock(&c->lp_mutex); 80 89 return 0; 90 + } 81 91 82 92 ubifs_assert(c, atomic_long_read(&c->dirty_zn_cnt) == 0); 83 93 ubifs_assert(c, c->dirty_pn_cnt == 0); 84 94 ubifs_assert(c, c->dirty_nn_cnt == 0); 95 + mutex_unlock(&c->lp_mutex); 85 96 86 97 return 1; 87 98 }