From cbc898d52cd11b23fd7c8079becbd8be742e78ce Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Fri, 5 Jul 2019 12:05:43 +0100 Subject: [PATCH 01/10] ubifs: Remove redundant assignment to pointer fname The pointer fname is being assigned with a value that is never read because the function returns after the assignment. The assignment is redundant and can be removed. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King Signed-off-by: Richard Weinberger --- fs/ubifs/debug.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index a5f10d79e0dd..e4b52783819d 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -2817,7 +2817,6 @@ void dbg_debugfs_init_fs(struct ubifs_info *c) c->vi.ubi_num, c->vi.vol_id); if (n == UBIFS_DFS_DIR_LEN) { /* The array size is too small */ - fname = UBIFS_DFS_DIR_NAME; return; } From 8596813a117f61db2e74c449ea1436e63d096323 Mon Sep 17 00:00:00 2001 From: Richard Weinberger Date: Thu, 25 Jul 2019 22:46:01 +0200 Subject: [PATCH 02/10] ubi: Don't do anchor move within fastmap area To make sure that Fastmap can use a PEB within the first 64 PEBs, UBI moves blocks away from that area. It uses regular wear-leveling for that job. An anchor move can be triggered if no PEB is free in this area or because of anticipation. In the latter case it can happen that UBI decides to move a block but finds a free PEB within the same area. This case is in vain an increases only erase counters. Catch this case and cancel wear-leveling if this happens. Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/wl.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 949700a9bb4b..3fcdefe2714d 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -710,6 +710,12 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk, if (!e2) goto out_cancel; + /* + * Anchor move within the anchor area is useless. + */ + if (e2->pnum < UBI_FM_MAX_START) + goto out_cancel; + self_check_in_wl_tree(ubi, e1, &ubi->used); rb_erase(&e1->u.rb, &ubi->used); dbg_wl("anchor-move PEB %d to PEB %d", e1->pnum, e2->pnum); From 8615b94f029a4fb4306d3512aaf1c45f5fc24d4b Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Sat, 10 Aug 2019 17:57:41 +0800 Subject: [PATCH 03/10] ubi: ubi_wl_get_peb: Increase the number of attempts while getting PEB Running stress test io_paral (A pressure ubi test in mtd-utils) on an UBI device with fewer PEBs (fastmap enabled) may cause ENOSPC errors and make UBI device read-only, but there are still free PEBs on the UBI device. This problem can be easily reproduced by performing the following steps on a 2-core machine: $ modprobe nandsim first_id_byte=0x20 second_id_byte=0x33 parts=80 $ modprobe ubi mtd="0,0" fm_autoconvert $ ./io_paral /dev/ubi0 We may see the following verbose: (output) [io_paral] update_volume():108: failed to write 380 bytes at offset 95920 of volume 2 [io_paral] update_volume():109: update: 97088 bytes [io_paral] write_thread():227: function pwrite() failed with error 28 (No space left on device) [io_paral] write_thread():229: cannot write 15872 bytes to offs 31744, wrote -1 (dmesg) ubi0 error: ubi_wl_get_peb [ubi]: Unable to get a free PEB from user WL pool ubi0 warning: ubi_eba_write_leb [ubi]: switch to read-only mode CPU: 0 PID: 2027 Comm: io_paral Not tainted 5.3.0-rc2-00001-g5986cd0 #9 ubi0 warning: try_write_vid_and_data [ubi]: failed to write VID header to LEB 2:5, PEB 18 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0 -0-ga698c8995f-prebuilt.qemu.org 04/01/2014 Call Trace: dump_stack+0x85/0xba ubi_eba_write_leb+0xa1e/0xa40 [ubi] vol_cdev_write+0x307/0x520 [ubi] vfs_write+0xfa/0x280 ksys_pwrite64+0xc5/0xe0 __x64_sys_pwrite64+0x22/0x30 do_syscall_64+0xbf/0x440 In function ubi_wl_get_peb, the operation of filling the pool (ubi_update_fastmap) with free PEBs and fetching a free PEB from the pool is not atomic. After thread A filling the pool with free PEB, free PEB may be taken away by thread B. When thread A checks the expression again, the condition is still unsatisfactory. At this time, there may still be free PEBs on UBI that can be filled into the pool. This patch increases the number of attempts to obtain PEB. An extreme case (No free PEBs left after creating test volumes) has been tested on different type of machines for 100 times. The biggest number of attempts are shown below: x86_64 arm64 2-core 4 4 4-core 8 4 8-core 4 4 Signed-off-by: Zhihao Cheng Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/fastmap-wl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c index d9e2e3a6e105..c44c8470247e 100644 --- a/drivers/mtd/ubi/fastmap-wl.c +++ b/drivers/mtd/ubi/fastmap-wl.c @@ -196,7 +196,7 @@ static int produce_free_peb(struct ubi_device *ubi) */ int ubi_wl_get_peb(struct ubi_device *ubi) { - int ret, retried = 0; + int ret, attempts = 0; struct ubi_fm_pool *pool = &ubi->fm_pool; struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; @@ -221,12 +221,12 @@ int ubi_wl_get_peb(struct ubi_device *ubi) if (pool->used == pool->size) { spin_unlock(&ubi->wl_lock); - if (retried) { + attempts++; + if (attempts == 10) { ubi_err(ubi, "Unable to get a free PEB from user WL pool"); ret = -ENOSPC; goto out; } - retried = 1; up_read(&ubi->fm_eba_sem); ret = produce_free_peb(ubi); if (ret < 0) { From ce4d8b16e64d062f1c6271a0068dbbbba2dee620 Mon Sep 17 00:00:00 2001 From: Wenwen Wang Date: Mon, 19 Aug 2019 22:03:46 -0500 Subject: [PATCH 04/10] ubifs: Fix memory leak in read_znode() error path In read_znode(), the indexing node 'idx' is allocated by kmalloc(). However, it is not deallocated in the following execution if ubifs_node_check_hash() fails, leading to a memory leak bug. To fix this issue, free 'idx' before returning the error. Fixes: 16a26b20d2af ("ubifs: authentication: Add hashes to index nodes") Signed-off-by: Wenwen Wang Signed-off-by: Richard Weinberger --- fs/ubifs/tnc_misc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ubifs/tnc_misc.c b/fs/ubifs/tnc_misc.c index 6f293f662d98..49cb34c3f324 100644 --- a/fs/ubifs/tnc_misc.c +++ b/fs/ubifs/tnc_misc.c @@ -284,6 +284,7 @@ static int read_znode(struct ubifs_info *c, struct ubifs_zbranch *zzbr, err = ubifs_node_check_hash(c, idx, zzbr->hash); if (err) { ubifs_bad_hash(c, idx, zzbr->hash, lnum, offs); + kfree(idx); return err; } From 7992e00469c485f108a7f0da08be34b0fa441f79 Mon Sep 17 00:00:00 2001 From: Wenwen Wang Date: Mon, 19 Aug 2019 22:24:50 -0500 Subject: [PATCH 05/10] ubifs: Fix memory leak in __ubifs_node_verify_hmac error path In __ubifs_node_verify_hmac(), 'hmac' is allocated through kmalloc(). However, it is not deallocated in the following execution if ubifs_node_calc_hmac() fails, leading to a memory leak bug. To fix this issue, free 'hmac' before returning the error. Fixes: 49525e5eecca ("ubifs: Add helper functions for authentication support") Signed-off-by: Wenwen Wang Signed-off-by: Richard Weinberger --- fs/ubifs/auth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c index d9af2de9084a..8cdbd53d780c 100644 --- a/fs/ubifs/auth.c +++ b/fs/ubifs/auth.c @@ -479,8 +479,10 @@ int __ubifs_node_verify_hmac(const struct ubifs_info *c, const void *node, return -ENOMEM; err = ubifs_node_calc_hmac(c, node, len, ofs_hmac, hmac); - if (err) + if (err) { + kfree(hmac); return err; + } err = crypto_memneq(hmac, node + ofs_hmac, hmac_len); From 9163e0184bd7d5f779934d34581843f699ad2ffd Mon Sep 17 00:00:00 2001 From: Wenwen Wang Date: Mon, 19 Aug 2019 23:44:24 -0500 Subject: [PATCH 06/10] ubifs: Fix memory leak bug in alloc_ubifs_info() error path In ubifs_mount(), 'c' is allocated through kzalloc() in alloc_ubifs_info(). However, it is not deallocated in the following execution if ubifs_fill_super() fails, leading to a memory leak bug. To fix this issue, free 'c' before going to the 'out_deact' label. Fixes: 1e51764a3c2a ("UBIFS: add new flash file system") Signed-off-by: Wenwen Wang Signed-off-by: Richard Weinberger --- fs/ubifs/super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 8c1d571334bc..fe9231fc4702 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -2256,8 +2256,10 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags, } } else { err = ubifs_fill_super(sb, data, flags & SB_SILENT ? 1 : 0); - if (err) + if (err) { + kfree(c); goto out_deact; + } /* We do not support atime */ sb->s_flags |= SB_ACTIVE; if (IS_ENABLED(CONFIG_UBIFS_ATIME_SUPPORT)) From e46131b9fd31155a552dea598e7d915b64c5c04c Mon Sep 17 00:00:00 2001 From: Richard Weinberger Date: Sun, 1 Sep 2019 22:32:05 +0200 Subject: [PATCH 07/10] ubi: block: Warn if volume size is not multiple of 512 If volume size is not a multiple of 512, ubi block cuts off the last bytes of an volume since the block layer works on 512 byte sectors. This can happen especially on NOR flash with minimal io size of 1. To avoid unpleasant surprises, print a warning. Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/block.c | 43 +++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c index 6025398955a2..e1a2ae21dfd3 100644 --- a/drivers/mtd/ubi/block.c +++ b/drivers/mtd/ubi/block.c @@ -345,15 +345,36 @@ static const struct blk_mq_ops ubiblock_mq_ops = { .init_request = ubiblock_init_request, }; +static int calc_disk_capacity(struct ubi_volume_info *vi, u64 *disk_capacity) +{ + u64 size = vi->used_bytes >> 9; + + if (vi->used_bytes % 512) { + pr_warn("UBI: block: volume size is not a multiple of 512, " + "last %llu bytes are ignored!\n", + vi->used_bytes - (size << 9)); + } + + if ((sector_t)size != size) + return -EFBIG; + + *disk_capacity = size; + + return 0; +} + int ubiblock_create(struct ubi_volume_info *vi) { struct ubiblock *dev; struct gendisk *gd; - u64 disk_capacity = vi->used_bytes >> 9; + u64 disk_capacity; int ret; - if ((sector_t)disk_capacity != disk_capacity) - return -EFBIG; + ret = calc_disk_capacity(vi, &disk_capacity); + if (ret) { + return ret; + } + /* Check that the volume isn't already handled */ mutex_lock(&devices_mutex); if (find_dev_nolock(vi->ubi_num, vi->vol_id)) { @@ -507,7 +528,8 @@ int ubiblock_remove(struct ubi_volume_info *vi) static int ubiblock_resize(struct ubi_volume_info *vi) { struct ubiblock *dev; - u64 disk_capacity = vi->used_bytes >> 9; + u64 disk_capacity; + int ret; /* * Need to lock the device list until we stop using the device, @@ -520,11 +542,16 @@ static int ubiblock_resize(struct ubi_volume_info *vi) mutex_unlock(&devices_mutex); return -ENODEV; } - if ((sector_t)disk_capacity != disk_capacity) { + + ret = calc_disk_capacity(vi, &disk_capacity); + if (ret) { mutex_unlock(&devices_mutex); - dev_warn(disk_to_dev(dev->gd), "the volume is too big (%d LEBs), cannot resize", - vi->size); - return -EFBIG; + if (ret == -EFBIG) { + dev_warn(disk_to_dev(dev->gd), + "the volume is too big (%d LEBs), cannot resize", + vi->size); + } + return ret; } mutex_lock(&dev->dev_mutex); From f2538f999345405f7d2e1194c0c8efa4e11f7b3a Mon Sep 17 00:00:00 2001 From: Jia-Ju Bai Date: Wed, 24 Jul 2019 10:46:58 +0800 Subject: [PATCH 08/10] jffs2: Fix possible null-pointer dereferences in jffs2_add_frag_to_fragtree() In jffs2_add_frag_to_fragtree(), there is an if statement on line 223 to check whether "this" is NULL: if (this) When "this" is NULL, it is used at several places, such as on line 249: if (this->node) and on line 260: if (newfrag->ofs > this->ofs) Thus possible null-pointer dereferences may occur. To fix these bugs, -EINVAL is returned when "this" is NULL. These bugs are found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai Signed-off-by: Richard Weinberger --- fs/jffs2/nodelist.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/jffs2/nodelist.c b/fs/jffs2/nodelist.c index b86c78d178c6..021a4a2190ee 100644 --- a/fs/jffs2/nodelist.c +++ b/fs/jffs2/nodelist.c @@ -226,7 +226,7 @@ static int jffs2_add_frag_to_fragtree(struct jffs2_sb_info *c, struct rb_root *r lastend = this->ofs + this->size; } else { dbg_fragtree2("lookup gave no frag\n"); - lastend = 0; + return -EINVAL; } /* See if we ran off the end of the fragtree */ From 61b875e88a04fd626e68c657400f237af8afa95e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 24 Jul 2019 09:15:25 +0200 Subject: [PATCH 09/10] jffs2: Remove jffs2_gc_fetch_page and jffs2_gc_release_page Merge these two helpers into the only callers to get rid of some amazingly bad calling conventions. Suggested-by: Al Viro Signed-off-by: Christoph Hellwig Signed-off-by: Richard Weinberger --- fs/jffs2/fs.c | 27 --------------------------- fs/jffs2/gc.c | 21 +++++++++++++-------- fs/jffs2/os-linux.h | 3 --- 3 files changed, 13 insertions(+), 38 deletions(-) diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c index 8a20ddd25f2d..a3193c0f3a9b 100644 --- a/fs/jffs2/fs.c +++ b/fs/jffs2/fs.c @@ -678,33 +678,6 @@ struct jffs2_inode_info *jffs2_gc_fetch_inode(struct jffs2_sb_info *c, return JFFS2_INODE_INFO(inode); } -unsigned char *jffs2_gc_fetch_page(struct jffs2_sb_info *c, - struct jffs2_inode_info *f, - unsigned long offset, - unsigned long *priv) -{ - struct inode *inode = OFNI_EDONI_2SFFJ(f); - struct page *pg; - - pg = read_cache_page(inode->i_mapping, offset >> PAGE_SHIFT, - jffs2_do_readpage_unlock, inode); - if (IS_ERR(pg)) - return (void *)pg; - - *priv = (unsigned long)pg; - return kmap(pg); -} - -void jffs2_gc_release_page(struct jffs2_sb_info *c, - unsigned char *ptr, - unsigned long *priv) -{ - struct page *pg = (void *)*priv; - - kunmap(pg); - put_page(pg); -} - static int jffs2_flash_setup(struct jffs2_sb_info *c) { int ret = 0; diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c index 9ed0f26cf023..373b3b7c9f44 100644 --- a/fs/jffs2/gc.c +++ b/fs/jffs2/gc.c @@ -1165,12 +1165,13 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era struct jffs2_inode_info *f, struct jffs2_full_dnode *fn, uint32_t start, uint32_t end) { + struct inode *inode = OFNI_EDONI_2SFFJ(f); struct jffs2_full_dnode *new_fn; struct jffs2_raw_inode ri; uint32_t alloclen, offset, orig_end, orig_start; int ret = 0; unsigned char *comprbuf = NULL, *writebuf; - unsigned long pg; + struct page *page; unsigned char *pg_ptr; memset(&ri, 0, sizeof(ri)); @@ -1325,15 +1326,18 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era * end up here trying to GC the *same* page that jffs2_write_begin() is * trying to write out, read_cache_page() will not deadlock. */ mutex_unlock(&f->sem); - pg_ptr = jffs2_gc_fetch_page(c, f, start, &pg); - mutex_lock(&f->sem); - - if (IS_ERR(pg_ptr)) { + page = read_cache_page(inode->i_mapping, start >> PAGE_SHIFT, + jffs2_do_readpage_unlock, inode); + if (IS_ERR(page)) { pr_warn("read_cache_page() returned error: %ld\n", - PTR_ERR(pg_ptr)); - return PTR_ERR(pg_ptr); + PTR_ERR(page)); + mutex_lock(&f->sem); + return PTR_ERR(page); } + pg_ptr = kmap(page); + mutex_lock(&f->sem); + offset = start; while(offset < orig_end) { uint32_t datalen; @@ -1396,6 +1400,7 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era } } - jffs2_gc_release_page(c, pg_ptr, &pg); + kunmap(page); + put_page(page); return ret; } diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h index bd3d5f0ddc34..f4895dda26a3 100644 --- a/fs/jffs2/os-linux.h +++ b/fs/jffs2/os-linux.h @@ -183,9 +183,6 @@ unsigned char *jffs2_gc_fetch_page(struct jffs2_sb_info *c, struct jffs2_inode_info *f, unsigned long offset, unsigned long *priv); -void jffs2_gc_release_page(struct jffs2_sb_info *c, - unsigned char *pg, - unsigned long *priv); void jffs2_flash_cleanup(struct jffs2_sb_info *c); From 6a379f67454a3c740671ed6c7793b76ffecef50b Mon Sep 17 00:00:00 2001 From: Wenwen Wang Date: Mon, 19 Aug 2019 16:55:04 -0500 Subject: [PATCH 10/10] jffs2: Fix memory leak in jffs2_scan_eraseblock() error path In jffs2_scan_eraseblock(), 'sumptr' is allocated through kmalloc() if 'sumlen' is larger than 'buf_size'. However, it is not deallocated in the following execution if jffs2_fill_scan_buf() fails, leading to a memory leak bug. To fix this issue, free 'sumptr' before returning the error. Signed-off-by: Wenwen Wang Signed-off-by: Richard Weinberger --- fs/jffs2/scan.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c index 90431dd613b8..5f7e284e0df3 100644 --- a/fs/jffs2/scan.c +++ b/fs/jffs2/scan.c @@ -527,8 +527,11 @@ static int jffs2_scan_eraseblock (struct jffs2_sb_info *c, struct jffs2_eraseblo err = jffs2_fill_scan_buf(c, sumptr, jeb->offset + c->sector_size - sumlen, sumlen - buf_len); - if (err) + if (err) { + if (sumlen > buf_size) + kfree(sumptr); return err; + } } }