From 8b229c76765816796eec7ccd428f03bd8de8b525 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 25 Mar 2011 18:33:57 +0200 Subject: [PATCH 01/10] UBIFS: do not read flash unnecessarily This fix makes the 'dbg_check_old_index()' function return immediately if debugging is disabled, instead of executing incorrect 'goto out' which causes UBIFS to: 1. Allocate memory 2. Read the flash On every commit. OK, we do not commit that often, but it is still silly to do unneeded I/O anyway. Credits to coverity for spotting this silly issue. Signed-off-by: Artem Bityutskiy Cc: stable@kernel.org --- fs/ubifs/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c index b148fbc80f8d..1bd01ded7123 100644 --- a/fs/ubifs/commit.c +++ b/fs/ubifs/commit.c @@ -577,7 +577,7 @@ int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot) size_t sz; if (!(ubifs_chk_flags & UBIFS_CHK_OLD_IDX)) - goto out; + return 0; INIT_LIST_HEAD(&list); From 54acbaaa523ca0bd284a18f67ad213c379679e86 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 25 Mar 2011 19:09:54 +0200 Subject: [PATCH 02/10] UBIFS: fix oops on error path in read_pnode Thanks to coverity which spotted that UBIFS will oops if 'kmalloc()' in 'read_pnode()' fails and we dereference a NULL 'pnode' pointer when we 'goto out'. Signed-off-by: Artem Bityutskiy Cc: stable@kernel.org --- fs/ubifs/lpt.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c index 72775d35b99e..ef5155e109a2 100644 --- a/fs/ubifs/lpt.c +++ b/fs/ubifs/lpt.c @@ -1270,10 +1270,9 @@ static int read_pnode(struct ubifs_info *c, struct ubifs_nnode *parent, int iip) lnum = branch->lnum; offs = branch->offs; pnode = kzalloc(sizeof(struct ubifs_pnode), GFP_NOFS); - if (!pnode) { - err = -ENOMEM; - goto out; - } + if (!pnode) + return -ENOMEM; + if (lnum == 0) { /* * This pnode was not written which just means that the LEB From c88ac00c5af70c2a0741da14b22cdcf8507ddd92 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Tue, 29 Mar 2011 09:45:21 +0300 Subject: [PATCH 03/10] UBIFS: fix assertion warnings This patch fixes UBIFS assertion warnings like: UBIFS assert failed in ubifs_leb_unmap at 135 (pid 29365) Pid: 29365, comm: integck Tainted: G I 2.6.37-ubi-2.6+ #34 Call Trace: [] ubifs_lpt_init+0x95e/0x9ee [ubifs] [] ubifs_remount_fs+0x2c7/0x762 [ubifs] [] do_remount_sb+0xb6/0x101 [] ? do_mount+0x191/0x78e [] do_mount+0x258/0x78e [] ? alloc_pages_current+0xa2/0xc5 [] sys_mount+0x83/0xbd [] system_call_fastpath+0x16/0x1b They happen when we re-mount from R/O mode to R/W mode. While re-mounting, we write to the media, but we still have the c->ro_mount flag set. The fix is very simple - just clear the flag before starting re-mounting R/W. These warnings are caused by the following commit: 2ef13294d29bcfb306e0d360f1b97f37b647b0c0 For -stable guys: this bug was introduced in 2.6.38, this is materieal for 2.6.38-stable. Signed-off-by: Artem Bityutskiy Cc: stable@kernel.org [2.6.38] --- fs/ubifs/super.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 6ddd9973e681..c75f6133206c 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -1568,6 +1568,7 @@ static int ubifs_remount_rw(struct ubifs_info *c) mutex_lock(&c->umount_mutex); dbg_save_space_info(c); c->remounting_rw = 1; + c->ro_mount = 0; err = check_free_space(c); if (err) @@ -1676,13 +1677,13 @@ static int ubifs_remount_rw(struct ubifs_info *c) } dbg_gen("re-mounted read-write"); - c->ro_mount = 0; c->remounting_rw = 0; err = dbg_check_space_info(c); mutex_unlock(&c->umount_mutex); return err; out: + c->ro_mount = 1; vfree(c->orph_buf); c->orph_buf = NULL; if (c->bgt) { From 81354de3d8691c2dedcc686cd2c167819ff0df10 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 30 Mar 2011 11:18:54 +0300 Subject: [PATCH 04/10] UBIFS: do not select KALLSYMS_ALL All UBIFS needs is to make sure we stacktraces when UBIFS debugging is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL is not necessary. Moreover, Randy Dunlap reported that UBIFS causes the following Kconfig dependency warning: warning: (UBIFS_FS_DEBUG && LOCKDEP && LATENCYTOP) selects KALLSYMS_ALL which has unmet direct dependencies (DEBUG_KERNEL && KALLSYMS) The reason is that KALLSYMS_ALL requires DEBUG_KERNEL and KALLSYMS, so ideally, to select KALLSYMS_ALL we'd need to select DEBUG_KERNEL and KALLSYMS first. This seems to be too much to select. The easiest way to go is to forget about KALLSYMS_ALL and just select KALLSYMS when UBIFS debugging is enabled - that should be enough for stackdumps. Reported-by: Randy Dunlap Signed-off-by: Artem Bityutskiy Acked-by: Randy Dunlap --- fs/ubifs/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig index d7440904be17..f8b0160da2da 100644 --- a/fs/ubifs/Kconfig +++ b/fs/ubifs/Kconfig @@ -47,7 +47,7 @@ config UBIFS_FS_DEBUG bool "Enable debugging support" depends on UBIFS_FS select DEBUG_FS - select KALLSYMS_ALL + select KALLSYMS help This option enables UBIFS debugging support. It makes sure various assertions, self-checks, debugging messages and test modes are compiled From cc6a86b950d69cfe542ee0d0ff30790152936a00 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 1 Apr 2011 10:10:52 +0300 Subject: [PATCH 05/10] UBIFS: unify error path dbg_debugfs_init_fs This is just a small clean-up patch which simlifies and unifies the error path in the dbg_debugfs_init_fs(). We have common error path for all failure cases in this function except of the very first case. And this patch makes the first failure case use the same error path as the other cases by using the 'fname' and 'dent' variables. Signed-off-by: Artem Bityutskiy Acked-by: Phil Carmody --- fs/ubifs/debug.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index f25a7339f800..304cc6e1c84b 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -2806,13 +2806,11 @@ int dbg_debugfs_init_fs(struct ubifs_info *c) struct ubifs_debug_info *d = c->dbg; sprintf(d->dfs_dir_name, "ubi%d_%d", c->vi.ubi_num, c->vi.vol_id); - d->dfs_dir = debugfs_create_dir(d->dfs_dir_name, dfs_rootdir); - if (IS_ERR(d->dfs_dir)) { - err = PTR_ERR(d->dfs_dir); - ubifs_err("cannot create \"%s\" debugfs directory, error %d\n", - d->dfs_dir_name, err); + fname = d->dfs_dir_name; + dent = debugfs_create_dir(fname, dfs_rootdir); + if (IS_ERR(dent)) goto out; - } + d->dfs_dir = dent; fname = "dump_lprops"; dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops); @@ -2835,11 +2833,11 @@ int dbg_debugfs_init_fs(struct ubifs_info *c) return 0; out_remove: + debugfs_remove_recursive(d->dfs_dir); +out: err = PTR_ERR(dent); ubifs_err("cannot create \"%s\" debugfs directory, error %d\n", fname, err); - debugfs_remove_recursive(d->dfs_dir); -out: return err; } From 95169535113073993a3ed97ecc21831657f42a80 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 1 Apr 2011 10:16:17 +0300 Subject: [PATCH 06/10] UBIFS: fix error path in dbg_debugfs_init_fs The debug interface is substandard and on error returns either NULL or an error code packed in the pointer. So using "IS_ERR" for the pointers returned by debugfs function is incorrect. Instead, we should use IS_ERR_OR_NULL. This path is an improved vestion of the original patch from Phil Carmody. Reported-by: Phil Carmody Signed-off-by: Artem Bityutskiy Acked-by: Phil Carmody --- fs/ubifs/debug.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index 304cc6e1c84b..645737769c98 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -2808,25 +2808,25 @@ int dbg_debugfs_init_fs(struct ubifs_info *c) sprintf(d->dfs_dir_name, "ubi%d_%d", c->vi.ubi_num, c->vi.vol_id); fname = d->dfs_dir_name; dent = debugfs_create_dir(fname, dfs_rootdir); - if (IS_ERR(dent)) + if (IS_ERR_OR_NULL(dent)) goto out; d->dfs_dir = dent; fname = "dump_lprops"; dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops); - if (IS_ERR(dent)) + if (IS_ERR_OR_NULL(dent)) goto out_remove; d->dfs_dump_lprops = dent; fname = "dump_budg"; dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops); - if (IS_ERR(dent)) + if (IS_ERR_OR_NULL(dent)) goto out_remove; d->dfs_dump_budg = dent; fname = "dump_tnc"; dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops); - if (IS_ERR(dent)) + if (IS_ERR_OR_NULL(dent)) goto out_remove; d->dfs_dump_tnc = dent; @@ -2835,7 +2835,7 @@ int dbg_debugfs_init_fs(struct ubifs_info *c) out_remove: debugfs_remove_recursive(d->dfs_dir); out: - err = PTR_ERR(dent); + err = dent ? PTR_ERR(dent) : -ENODEV; ubifs_err("cannot create \"%s\" debugfs directory, error %d\n", fname, err); return err; From 7da6443aca9be29c6948dcbd636ad50154d0bc0c Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Mon, 4 Apr 2011 17:16:39 +0300 Subject: [PATCH 07/10] UBIFS: fix debugging failure in dbg_check_space_info This patch fixes a debugging failure with which looks like this: UBIFS error (pid 32313): dbg_check_space_info: free space changed from 6019344 to 6022654 The reason for this failure is described in the comment this patch adds to the code. But in short - 'c->freeable_cnt' may be different before and after re-mounting, and this is normal. So the debugging code should make sure that free space calculations do not depend on 'c->freeable_cnt'. A similar issue has been reported here: http://lists.infradead.org/pipermail/linux-mtd/2011-April/034647.html This patch should fix it. For the -stable guys: this patch is only relevant for kernels 2.6.30 onwards. Signed-off-by: Artem Bityutskiy Cc: stable@kernel.org [2.6.30+] --- fs/ubifs/debug.c | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index 645737769c98..004d3745dc45 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -972,11 +972,39 @@ void dbg_dump_index(struct ubifs_info *c) void dbg_save_space_info(struct ubifs_info *c) { struct ubifs_debug_info *d = c->dbg; - - ubifs_get_lp_stats(c, &d->saved_lst); + int freeable_cnt; spin_lock(&c->space_lock); + memcpy(&d->saved_lst, &c->lst, sizeof(struct ubifs_lp_stats)); + + /* + * We use a dirty hack here and zero out @c->freeable_cnt, because it + * affects the free space calculations, and UBIFS might not know about + * all freeable eraseblocks. Indeed, we know about freeable eraseblocks + * only when we read their lprops, and we do this only lazily, upon the + * need. So at any given point of time @c->freeable_cnt might be not + * exactly accurate. + * + * Just one example about the issue we hit when we did not zero + * @c->freeable_cnt. + * 1. The file-system is mounted R/O, c->freeable_cnt is %0. We save the + * amount of free space in @d->saved_free + * 2. We re-mount R/W, which makes UBIFS to read the "lsave" + * information from flash, where we cache LEBs from various + * categories ('ubifs_remount_fs()' -> 'ubifs_lpt_init()' + * -> 'lpt_init_wr()' -> 'read_lsave()' -> 'ubifs_lpt_lookup()' + * -> 'ubifs_get_pnode()' -> 'update_cats()' + * -> 'ubifs_add_to_cat()'). + * 3. Lsave contains a freeable eraseblock, and @c->freeable_cnt + * becomes %1. + * 4. We calculate the amount of free space when the re-mount is + * finished in 'dbg_check_space_info()' and it does not match + * @d->saved_free. + */ + freeable_cnt = c->freeable_cnt; + c->freeable_cnt = 0; d->saved_free = ubifs_get_free_space_nolock(c); + c->freeable_cnt = freeable_cnt; spin_unlock(&c->space_lock); } @@ -993,12 +1021,15 @@ int dbg_check_space_info(struct ubifs_info *c) { struct ubifs_debug_info *d = c->dbg; struct ubifs_lp_stats lst; - long long avail, free; + long long free; + int freeable_cnt; spin_lock(&c->space_lock); - avail = ubifs_calc_available(c, c->min_idx_lebs); + freeable_cnt = c->freeable_cnt; + c->freeable_cnt = 0; + free = ubifs_get_free_space_nolock(c); + c->freeable_cnt = freeable_cnt; spin_unlock(&c->space_lock); - free = ubifs_get_free_space(c); if (free != d->saved_free) { ubifs_err("free space changed from %lld to %lld", From 3efe509070e3d27e6d5dbc4bf8588e9453e9b949 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 18 Mar 2011 18:11:42 +0200 Subject: [PATCH 08/10] UBI: check if we are in RO mode in the erase routine 'do_sync_erase()' has to check whether we are in R/O mode before erasing the PEB. This patch adds the check and while on it, adds an assertion which validates the 'pnum' argument, as well as removes a check which is always true because it has already been done few lines before. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/io.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index eededf94f5a6..e347cc4388ed 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -344,6 +344,12 @@ static int do_sync_erase(struct ubi_device *ubi, int pnum) wait_queue_head_t wq; dbg_io("erase PEB %d", pnum); + ubi_assert(pnum >= 0 && pnum < ubi->peb_count); + + if (ubi->ro_mode) { + ubi_err("read-only mode"); + return -EROFS; + } retry: init_waitqueue_head(&wq); @@ -390,7 +396,7 @@ retry: if (err) return err; - if (ubi_dbg_is_erase_failure() && !err) { + if (ubi_dbg_is_erase_failure()) { dbg_err("cannot erase PEB %d (emulated)", pnum); return -EIO; } From 6e5133cc757912e7ba2bfbbfb384667707f45ec3 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 25 Mar 2011 18:48:59 +0200 Subject: [PATCH 09/10] UBI: do not compare array with NULL Coverity spotted that UBI debugging code tries to compare an array and NULL, which obviously makes little sense. Kill this check. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/vmt.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c index b79e0dea3632..366eb70219a6 100644 --- a/drivers/mtd/ubi/vmt.c +++ b/drivers/mtd/ubi/vmt.c @@ -790,11 +790,6 @@ static int paranoid_check_volume(struct ubi_device *ubi, int vol_id) goto fail; } - if (!vol->name) { - ubi_err("NULL volume name"); - goto fail; - } - n = strnlen(vol->name, vol->name_len + 1); if (n != vol->name_len) { ubi_err("bad name_len %lld", n); From 6bef0b67474d71e0d6484cbabcc87657a1176d8d Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 30 Mar 2011 11:27:08 +0300 Subject: [PATCH 10/10] UBI: do not select KALLSYMS_ALL All UBI needs is to make sure we stacktraces when UBI debugging is enabled. It is enough to select KALLSYMS for this, KALLSYMS_ALL is not necessary. And the current Kconfig line we have: select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL is just too complex to be sane and right. But this "if" part there is needed to prevent "unmet direct dependency" warnings, because KALLSYMS_ALL depends on KALLSYMS and DEBUG_KERNEL, so we cannot just select KALLSYMS_ALL. Anyway, this feels messy, and we do not seem to really need KALLSYMS_ALL, so select KALLSYMS instead. Signed-off-by: Artem Bityutskiy Acked-by: Randy Dunlap --- drivers/mtd/ubi/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig index 6abeb4f13403..4dcc752a0c0b 100644 --- a/drivers/mtd/ubi/Kconfig +++ b/drivers/mtd/ubi/Kconfig @@ -56,7 +56,7 @@ config MTD_UBI_DEBUG bool "UBI debugging" depends on SYSFS select DEBUG_FS - select KALLSYMS_ALL if KALLSYMS && DEBUG_KERNEL + select KALLSYMS help This option enables UBI debugging.