From a2c2a622d41168f9fea2aa3f76b9fbaa88531aac Mon Sep 17 00:00:00 2001 From: Zhen Lei Date: Sat, 8 May 2021 11:33:13 +0800 Subject: ubifs: journal: Fix error return code in ubifs_jnl_write_inode() Fix to return a negative error code from the error handling case instead of 0, as done elsewhere in this function. Fixes: 9ca2d7326444 ("ubifs: Limit number of xattrs per inode") Reported-by: Hulk Robot Signed-off-by: Zhen Lei Signed-off-by: Richard Weinberger --- fs/ubifs/journal.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 2857e64d673d..230717384a38 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -882,6 +882,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) struct ubifs_dent_node *xent, *pxent = NULL; if (ui->xattr_cnt > ubifs_xattr_max_cnt(c)) { + err = -EPERM; ubifs_err(c, "Cannot delete inode, it has too much xattrs!"); goto out_release; } -- cgit v1.2.3 From be076fdf8369f3b4842362c64cd681f3d498f3dd Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 11 May 2021 10:12:00 +0300 Subject: ubifs: fix snprintf() checking The snprintf() function returns the number of characters (not counting the NUL terminator) that it would have printed if we had space. This buffer has UBIFS_DFS_DIR_LEN characters plus one extra for the terminator. Printing UBIFS_DFS_DIR_LEN is okay but anything higher will result in truncation. Thus the comparison needs to be change from == to >. These strings are compile time constants so this patch doesn't affect runtime. Fixes: ae380ce04731 ("UBIFS: lessen the size of debugging info data structure") Signed-off-by: Dan Carpenter Reviewed-by: Alexander Dahl Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/debug.c | 2 +- fs/ubifs/debug.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c index ac2bdba8bb1a..3c0c8eca4d51 100644 --- a/drivers/mtd/ubi/debug.c +++ b/drivers/mtd/ubi/debug.c @@ -511,7 +511,7 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi) n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN + 1, UBI_DFS_DIR_NAME, ubi->ubi_num); - if (n == UBI_DFS_DIR_LEN) { + if (n > UBI_DFS_DIR_LEN) { /* The array size is too small */ return -EINVAL; } diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index 1bbb9fe661b1..fc718f6178f2 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -2824,7 +2824,7 @@ void dbg_debugfs_init_fs(struct ubifs_info *c) n = snprintf(d->dfs_dir_name, UBIFS_DFS_DIR_LEN + 1, UBIFS_DFS_DIR_NAME, c->vi.ubi_num, c->vi.vol_id); - if (n == UBIFS_DFS_DIR_LEN) { + if (n > UBIFS_DFS_DIR_LEN) { /* The array size is too small */ return; } -- cgit v1.2.3 From f4e3634a3b642225a530c292fdb1e8a4007507f5 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 31 May 2021 20:52:09 +0800 Subject: ubifs: Fix races between xattr_{set|get} and listxattr operations UBIFS may occur some problems with concurrent xattr_{set|get} and listxattr operations, such as assertion failure, memory corruption, stale xattr value[1]. Fix it by importing a new rw-lock in @ubifs_inode to serilize write operations on xattr, concurrent read operations are still effective, just like ext4. [1] https://lore.kernel.org/linux-mtd/20200630130438.141649-1-houtao1@huawei.com Fixes: 1e51764a3c2ac05a23 ("UBIFS: add new flash file system") Cc: stable@vger.kernel.org # v2.6+ Signed-off-by: Zhihao Cheng Reviewed-by: Sascha Hauer Signed-off-by: Richard Weinberger --- fs/ubifs/super.c | 1 + fs/ubifs/ubifs.h | 2 ++ fs/ubifs/xattr.c | 44 +++++++++++++++++++++++++++++++++----------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 7b572e1414ba..e279a069a26b 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -275,6 +275,7 @@ static struct inode *ubifs_alloc_inode(struct super_block *sb) memset((void *)ui + sizeof(struct inode), 0, sizeof(struct ubifs_inode) - sizeof(struct inode)); mutex_init(&ui->ui_mutex); + init_rwsem(&ui->xattr_sem); spin_lock_init(&ui->ui_lock); return &ui->vfs_inode; }; diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index b65c599a386a..7e978f421430 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -356,6 +356,7 @@ struct ubifs_gced_idx_leb { * @ui_mutex: serializes inode write-back with the rest of VFS operations, * serializes "clean <-> dirty" state changes, serializes bulk-read, * protects @dirty, @bulk_read, @ui_size, and @xattr_size + * @xattr_sem: serilizes write operations (remove|set|create) on xattr * @ui_lock: protects @synced_i_size * @synced_i_size: synchronized size of inode, i.e. the value of inode size * currently stored on the flash; used only for regular file @@ -409,6 +410,7 @@ struct ubifs_inode { unsigned int bulk_read:1; unsigned int compr_type:2; struct mutex ui_mutex; + struct rw_semaphore xattr_sem; spinlock_t ui_lock; loff_t synced_i_size; loff_t ui_size; diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 6b1e9830b274..1fce27e9b769 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -285,6 +285,7 @@ int ubifs_xattr_set(struct inode *host, const char *name, const void *value, if (!xent) return -ENOMEM; + down_write(&ubifs_inode(host)->xattr_sem); /* * The extended attribute entries are stored in LNC, so multiple * look-ups do not involve reading the flash. @@ -319,6 +320,7 @@ int ubifs_xattr_set(struct inode *host, const char *name, const void *value, iput(inode); out_free: + up_write(&ubifs_inode(host)->xattr_sem); kfree(xent); return err; } @@ -341,18 +343,19 @@ ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf, if (!xent) return -ENOMEM; + down_read(&ubifs_inode(host)->xattr_sem); xent_key_init(c, &key, host->i_ino, &nm); err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); if (err) { if (err == -ENOENT) err = -ENODATA; - goto out_unlock; + goto out_cleanup; } inode = iget_xattr(c, le64_to_cpu(xent->inum)); if (IS_ERR(inode)) { err = PTR_ERR(inode); - goto out_unlock; + goto out_cleanup; } ui = ubifs_inode(inode); @@ -374,7 +377,8 @@ ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf, out_iput: mutex_unlock(&ui->ui_mutex); iput(inode); -out_unlock: +out_cleanup: + up_read(&ubifs_inode(host)->xattr_sem); kfree(xent); return err; } @@ -406,16 +410,21 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) dbg_gen("ino %lu ('%pd'), buffer size %zd", host->i_ino, dentry, size); + down_read(&host_ui->xattr_sem); len = host_ui->xattr_names + host_ui->xattr_cnt; - if (!buffer) + if (!buffer) { /* * We should return the minimum buffer size which will fit a * null-terminated list of all the extended attribute names. */ - return len; + err = len; + goto out_err; + } - if (len > size) - return -ERANGE; + if (len > size) { + err = -ERANGE; + goto out_err; + } lowest_xent_key(c, &key, host->i_ino); while (1) { @@ -437,8 +446,9 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) pxent = xent; key_read(c, &xent->key, &key); } - kfree(pxent); + up_read(&host_ui->xattr_sem); + if (err != -ENOENT) { ubifs_err(c, "cannot find next direntry, error %d", err); return err; @@ -446,6 +456,10 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) ubifs_assert(c, written <= size); return written; + +out_err: + up_read(&host_ui->xattr_sem); + return err; } static int remove_xattr(struct ubifs_info *c, struct inode *host, @@ -504,6 +518,7 @@ int ubifs_purge_xattrs(struct inode *host) ubifs_warn(c, "inode %lu has too many xattrs, doing a non-atomic deletion", host->i_ino); + down_write(&ubifs_inode(host)->xattr_sem); lowest_xent_key(c, &key, host->i_ino); while (1) { xent = ubifs_tnc_next_ent(c, &key, &nm); @@ -523,7 +538,7 @@ int ubifs_purge_xattrs(struct inode *host) ubifs_ro_mode(c, err); kfree(pxent); kfree(xent); - return err; + goto out_err; } ubifs_assert(c, ubifs_inode(xino)->xattr); @@ -535,7 +550,7 @@ int ubifs_purge_xattrs(struct inode *host) kfree(xent); iput(xino); ubifs_err(c, "cannot remove xattr, error %d", err); - return err; + goto out_err; } iput(xino); @@ -544,14 +559,19 @@ int ubifs_purge_xattrs(struct inode *host) pxent = xent; key_read(c, &xent->key, &key); } - kfree(pxent); + up_write(&ubifs_inode(host)->xattr_sem); + if (err != -ENOENT) { ubifs_err(c, "cannot find next direntry, error %d", err); return err; } return 0; + +out_err: + up_write(&ubifs_inode(host)->xattr_sem); + return err; } /** @@ -594,6 +614,7 @@ static int ubifs_xattr_remove(struct inode *host, const char *name) if (!xent) return -ENOMEM; + down_write(&ubifs_inode(host)->xattr_sem); xent_key_init(c, &key, host->i_ino, &nm); err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); if (err) { @@ -618,6 +639,7 @@ static int ubifs_xattr_remove(struct inode *host, const char *name) iput(inode); out_free: + up_write(&ubifs_inode(host)->xattr_sem); kfree(xent); return err; } -- cgit v1.2.3 From 819f9ab430a4478ce519e5cc8ae4de438d8ad4ba Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 31 May 2021 20:52:10 +0800 Subject: ubifs: Remove ui_mutex in ubifs_xattr_get and change_xattr Since ubifs_xattr_get and ubifs_xattr_set cannot being executed parallelly after importing @host_ui->xattr_sem, now we can remove ui_mutex imported by commit ab92a20bce3b4c2 ("ubifs: make ubifs_[get|set]xattr atomic"). @xattr_size, @xattr_names and @xattr_cnt can't be out of protection by @host_ui->mutex yet, they are sill accesed in other places, such as pack_inode() called by ubifs_write_inode() triggered by page-writeback. Signed-off-by: Zhihao Cheng Signed-off-by: Richard Weinberger --- fs/ubifs/xattr.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 1fce27e9b769..e4f193eae4b2 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -208,13 +208,11 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, err = -ENOMEM; goto out_free; } - mutex_lock(&ui->ui_mutex); kfree(ui->data); ui->data = buf; inode->i_size = ui->ui_size = size; old_size = ui->data_len; ui->data_len = size; - mutex_unlock(&ui->ui_mutex); mutex_lock(&host_ui->ui_mutex); host->i_ctime = current_time(host); @@ -362,7 +360,6 @@ ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf, ubifs_assert(c, inode->i_size == ui->data_len); ubifs_assert(c, ubifs_inode(host)->xattr_size > ui->data_len); - mutex_lock(&ui->ui_mutex); if (buf) { /* If @buf is %NULL we are supposed to return the length */ if (ui->data_len > size) { @@ -375,7 +372,6 @@ ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf, err = ui->data_len; out_iput: - mutex_unlock(&ui->ui_mutex); iput(inode); out_cleanup: up_read(&ubifs_inode(host)->xattr_sem); -- cgit v1.2.3 From 07c32de44e67882e66f4f81f78d2a16bb72337e4 Mon Sep 17 00:00:00 2001 From: Zheng Yongjun Date: Fri, 4 Jun 2021 09:45:56 +0800 Subject: ubifs: Fix spelling mistakes Fix some spelling mistakes in comments: withoug ==> without numer ==> number aswell ==> as well referes ==> refers childs ==> children unnecesarry ==> unnecessary Signed-off-by: Zheng Yongjun Reviewed-by: Alexander Dahl Signed-off-by: Richard Weinberger --- fs/ubifs/journal.c | 2 +- fs/ubifs/master.c | 2 +- fs/ubifs/replay.c | 2 +- fs/ubifs/super.c | 2 +- fs/ubifs/tnc_commit.c | 2 +- fs/ubifs/ubifs.h | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 230717384a38..8ea680dba61e 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1432,7 +1432,7 @@ out_free: /** * truncate_data_node - re-compress/encrypt a truncated data node. * @c: UBIFS file-system description object - * @inode: inode which referes to the data node + * @inode: inode which refers to the data node * @block: data block number * @dn: data node to re-compress * @new_len: new length diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c index 0df9a3dd0aaa..7adc37c10b6a 100644 --- a/fs/ubifs/master.c +++ b/fs/ubifs/master.c @@ -37,7 +37,7 @@ int ubifs_compare_master_node(struct ubifs_info *c, void *m1, void *m2) return ret; /* - * Do not compare the embedded HMAC aswell which also must be different + * Do not compare the embedded HMAC as well which also must be different * due to the different common node header. */ behind = hmac_offs + UBIFS_MAX_HMAC_LEN; diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index 382a54c82930..5260d3e531bb 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -296,7 +296,7 @@ static int apply_replay_entry(struct ubifs_info *c, struct replay_entry *r) * @b: second replay entry * * This is a comparios function for 'list_sort()' which compares 2 replay - * entries @a and @b by comparing their sequence numer. Returns %1 if @a has + * entries @a and @b by comparing their sequence number. Returns %1 if @a has * greater sequence number and %-1 otherwise. */ static int replay_entries_cmp(void *priv, const struct list_head *a, diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index e279a069a26b..f0fb25727d96 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -2061,7 +2061,7 @@ const struct super_operations ubifs_super_operations = { * @mode: UBI volume open mode * * The primary method of mounting UBIFS is by specifying the UBI volume - * character device node path. However, UBIFS may also be mounted withoug any + * character device node path. However, UBIFS may also be mounted without any * character device node using one of the following methods: * * o ubiX_Y - mount UBI device number X, volume Y; diff --git a/fs/ubifs/tnc_commit.c b/fs/ubifs/tnc_commit.c index 234be1c4dc87..58c92c96ecef 100644 --- a/fs/ubifs/tnc_commit.c +++ b/fs/ubifs/tnc_commit.c @@ -930,7 +930,7 @@ static int write_index(struct ubifs_info *c) * flag cleared before %COW_ZNODE. Specifically, it matters in * the 'dirty_cow_znode()' function. This is the reason for the * first barrier. Also, we want the bit changes to be seen to - * other threads ASAP, to avoid unnecesarry copying, which is + * other threads ASAP, to avoid unnecessary copying, which is * the reason for the second barrier. */ clear_bit(DIRTY_ZNODE, &znode->flags); diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index 7e978f421430..c38066ce9ab0 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -914,7 +914,7 @@ struct ubifs_budget_req { * @rb: rb-tree node of rb-tree of orphans sorted by inode number * @list: list head of list of orphans in order added * @new_list: list head of list of orphans added since the last commit - * @child_list: list of xattr childs if this orphan hosts xattrs, list head + * @child_list: list of xattr children if this orphan hosts xattrs, list head * if this orphan is a xattr, not used otherwise. * @cnext: next orphan to commit * @dnext: next orphan to delete -- cgit v1.2.3 From a801fcfeef96702fa3f9b22ad56c5eb1989d9221 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Fri, 18 Jun 2021 16:11:03 +0800 Subject: ubifs: Set/Clear I_LINKABLE under i_lock for whiteout inode xfstests-generic/476 reports a warning message as below: WARNING: CPU: 2 PID: 30347 at fs/inode.c:361 inc_nlink+0x52/0x70 Call Trace: do_rename+0x502/0xd40 [ubifs] ubifs_rename+0x8b/0x180 [ubifs] vfs_rename+0x476/0x1080 do_renameat2+0x67c/0x7b0 __x64_sys_renameat2+0x6e/0x90 do_syscall_64+0x66/0xe0 entry_SYSCALL_64_after_hwframe+0x44/0xae Following race case can cause this: rename_whiteout(Thread 1) wb_workfn(Thread 2) ubifs_rename do_rename __writeback_single_inode spin_lock(&inode->i_lock) whiteout->i_state |= I_LINKABLE inode->i_state &= ~dirty; ---- How race happens on i_state: (tmp = whiteout->i_state | I_LINKABLE) (tmp = inode->i_state & ~dirty) (whiteout->i_state = tmp) (inode->i_state = tmp) ---- spin_unlock(&inode->i_lock) inc_nlink(whiteout) WARN_ON(!(inode->i_state & I_LINKABLE)) !!! Fix to add i_lock to avoid i_state update race condition. Fixes: 9e0a1fff8db56ea ("ubifs: Implement RENAME_WHITEOUT") Signed-off-by: Zhihao Cheng Signed-off-by: Richard Weinberger --- fs/ubifs/dir.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 5bd8482e660a..7c61d0ec0159 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -1337,7 +1337,10 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry, goto out_release; } + spin_lock(&whiteout->i_lock); whiteout->i_state |= I_LINKABLE; + spin_unlock(&whiteout->i_lock); + whiteout_ui = ubifs_inode(whiteout); whiteout_ui->data = dev; whiteout_ui->data_len = ubifs_encode_dev(dev, MKDEV(0, 0)); @@ -1430,7 +1433,11 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry, inc_nlink(whiteout); mark_inode_dirty(whiteout); + + spin_lock(&whiteout->i_lock); whiteout->i_state &= ~I_LINKABLE; + spin_unlock(&whiteout->i_lock); + iput(whiteout); } -- cgit v1.2.3