Commit 60eb3b9c authored by Zhihao Cheng's avatar Zhihao Cheng Committed by Richard Weinberger
Browse files

ubifs: Fix 'ui->dirty' race between do_tmpfile() and writeback work 

'ui->dirty' is not protected by 'ui_mutex' in function do_tmpfile() which
may race with ubifs_write_inode[wb_workfn] to access/update 'ui->dirty',
finally dirty space is released twice.

	open(O_TMPFILE)                wb_workfn
do_tmpfile
  ubifs_budget_space(ino_req = { .dirtied_ino = 1})
  d_tmpfile // mark inode(tmpfile) dirty
  ubifs_jnl_update // without holding tmpfile's ui_mutex
    mark_inode_clean(ui)
      if (ui->dirty)
        ubifs_release_dirty_inode_budget(ui)  // release first time
                                   ubifs_write_inode
				     mutex_lock(&ui->ui_mutex)
                                     ubifs_release_dirty_inode_budget(ui)
				     // release second time
				     mutex_unlock(&ui->ui_mutex)
      ui->dirty = 0

Run generic/476 can reproduce following message easily
(See reproducer in [Link]):

  UBIFS error (ubi0:0 pid 2578): ubifs_assert_failed [ubifs]: UBIFS assert
  failed: c->bi.dd_growth >= 0, in fs/ubifs/budget.c:554
  UBIFS warning (ubi0:0 pid 2578): ubifs_ro_mode [ubifs]: switched to
  read-only mode, error -22
  Workqueue: writeback wb_workfn (flush-ubifs_0_0)
  Call Trace:
    ubifs_ro_mode+0x54/0x60 [ubifs]
    ubifs_assert_failed+0x4b/0x80 [ubifs]
    ubifs_release_budget+0x468/0x5a0 [ubifs]
    ubifs_release_dirty_inode_budget+0x53/0x80 [ubifs]
    ubifs_write_inode+0x121/0x1f0 [ubifs]
    ...
    wb_workfn+0x283/0x7b0

Fix it by holding tmpfile ubifs inode lock during ubifs_jnl_update().
Similar problem exists in whiteout renaming, but previous fix("ubifs:
Rename whiteout atomically") has solved the problem.

Fixes: 474b9370 ("ubifs: Implement O_TMPFILE")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=214765


Signed-off-by: default avatarZhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: default avatarRichard Weinberger <richard@nod.at>
parent 278d9a24

fs/ubifs/dir.c

+30 −30
Original line number Diff line number Diff line
@@ -397,6 +397,32 @@ static struct inode *create_whiteout(struct inode *dir, struct dentry *dentry) 
	return ERR_PTR(err);

}



/**

 * lock_2_inodes - a wrapper for locking two UBIFS inodes.

 * @inode1: first inode

 * @inode2: second inode

 *

 * We do not implement any tricks to guarantee strict lock ordering, because

 * VFS has already done it for us on the @i_mutex. So this is just a simple

 * wrapper function.

 */

static void lock_2_inodes(struct inode *inode1, struct inode *inode2)

{

	mutex_lock_nested(&ubifs_inode(inode1)->ui_mutex, WB_MUTEX_1);

	mutex_lock_nested(&ubifs_inode(inode2)->ui_mutex, WB_MUTEX_2);

}



/**

 * unlock_2_inodes - a wrapper for unlocking two UBIFS inodes.

 * @inode1: first inode

 * @inode2: second inode

 */

static void unlock_2_inodes(struct inode *inode1, struct inode *inode2)

{

	mutex_unlock(&ubifs_inode(inode2)->ui_mutex);

	mutex_unlock(&ubifs_inode(inode1)->ui_mutex);

}



static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,

			 struct dentry *dentry, umode_t mode)

{
@@ -404,7 +430,7 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, 
	struct ubifs_info *c = dir->i_sb->s_fs_info;

	struct ubifs_budget_req req = { .new_ino = 1, .new_dent = 1};

	struct ubifs_budget_req ino_req = { .dirtied_ino = 1 };

	struct ubifs_inode *ui, *dir_ui = ubifs_inode(dir);

	struct ubifs_inode *ui;

	int err, instantiated = 0;

	struct fscrypt_name nm;
@@ -452,18 +478,18 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, 
	instantiated = 1;

	mutex_unlock(&ui->ui_mutex);



	mutex_lock(&dir_ui->ui_mutex);

	lock_2_inodes(dir, inode);

	err = ubifs_jnl_update(c, dir, &nm, inode, 1, 0);

	if (err)

		goto out_cancel;

	mutex_unlock(&dir_ui->ui_mutex);

	unlock_2_inodes(dir, inode);



	ubifs_release_budget(c, &req);



	return 0;



out_cancel:

	mutex_unlock(&dir_ui->ui_mutex);

	unlock_2_inodes(dir, inode);

out_inode:

	make_bad_inode(inode);

	if (!instantiated)
@@ -690,32 +716,6 @@ static int ubifs_dir_release(struct inode *dir, struct file *file) 
	return 0;

}



/**

 * lock_2_inodes - a wrapper for locking two UBIFS inodes.

 * @inode1: first inode

 * @inode2: second inode

 *

 * We do not implement any tricks to guarantee strict lock ordering, because

 * VFS has already done it for us on the @i_mutex. So this is just a simple

 * wrapper function.

 */

static void lock_2_inodes(struct inode *inode1, struct inode *inode2)

{

	mutex_lock_nested(&ubifs_inode(inode1)->ui_mutex, WB_MUTEX_1);

	mutex_lock_nested(&ubifs_inode(inode2)->ui_mutex, WB_MUTEX_2);

}



/**

 * unlock_2_inodes - a wrapper for unlocking two UBIFS inodes.

 * @inode1: first inode

 * @inode2: second inode

 */

static void unlock_2_inodes(struct inode *inode1, struct inode *inode2)

{

	mutex_unlock(&ubifs_inode(inode2)->ui_mutex);

	mutex_unlock(&ubifs_inode(inode1)->ui_mutex);

}



static int ubifs_link(struct dentry *old_dentry, struct inode *dir,

		      struct dentry *dentry)

{