From ffb6b7e4fdef715061859651fe46cd27afc6acec Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Tue, 12 May 2009 15:43:44 +0300 Subject: UBI: fix races in I/O debugging checks When paranoid checs are enabled, the 'io_paral' test from the 'mtd-utils' package fails. The symptoms are: UBI error: paranoid_check_all_ff: flash region at PEB 3973:512, length 15872 does not contain all 0xFF bytes UBI error: paranoid_check_all_ff: paranoid check failed for PEB 3973 UBI: hex dump of the 512-16384 region It turned out to be a bug in the checking function. Suppose there are 2 tasks - A and B. Task A is the wear-levelling working ('wear_leveling_worker()'). It is reading the VID header to find which LEB this PEB belongs to. Say, task A is reading header of PEB X. Suppose PEB X is unmapped, and has no VID header. Task B is trying to write to PEB X. Task A: in 'ubi_io_read_vid_hdr()': reads the VID header from PEB X. The read data contain all 0xFF bytes. Task B: writes VID header and some data to PEB X Task A: assumes PEB X is empty, calls 'paranoid_check_all_ff()', which fails. The solution for this problem is to make 'paranoid_check_all_ff()' re-read the VID header, re-check it, and only if it is not there, check the rest. This now implemented by the 'paranoid_check_empty()' function. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/io.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 73 insertions(+), 7 deletions(-) (limited to 'drivers/mtd/ubi/io.c') diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index fe81039f2a7..ac6604aeb72 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -100,6 +100,7 @@ static int paranoid_check_vid_hdr(const struct ubi_device *ubi, int pnum, const struct ubi_vid_hdr *vid_hdr); static int paranoid_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len); +static int paranoid_check_empty(struct ubi_device *ubi, int pnum); #else #define paranoid_check_not_bad(ubi, pnum) 0 #define paranoid_check_peb_ec_hdr(ubi, pnum) 0 @@ -107,6 +108,7 @@ static int paranoid_check_all_ff(struct ubi_device *ubi, int pnum, int offset, #define paranoid_check_peb_vid_hdr(ubi, pnum) 0 #define paranoid_check_vid_hdr(ubi, pnum, vid_hdr) 0 #define paranoid_check_all_ff(ubi, pnum, offset, len) 0 +#define paranoid_check_empty(ubi, pnum) 0 #endif /** @@ -670,11 +672,6 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum, if (read_err != -EBADMSG && check_pattern(ec_hdr, 0xFF, UBI_EC_HDR_SIZE)) { /* The physical eraseblock is supposedly empty */ - - /* - * The below is just a paranoid check, it has to be - * compiled out if paranoid checks are disabled. - */ err = paranoid_check_all_ff(ubi, pnum, 0, ubi->peb_size); if (err) @@ -955,8 +952,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum, * The below is just a paranoid check, it has to be * compiled out if paranoid checks are disabled. */ - err = paranoid_check_all_ff(ubi, pnum, ubi->leb_start, - ubi->leb_size); + err = paranoid_check_empty(ubi, pnum); if (err) return err > 0 ? UBI_IO_BAD_VID_HDR : err; @@ -1280,4 +1276,74 @@ error: return err; } +/** + * paranoid_check_empty - whether a PEB is empty. + * @ubi: UBI device description object + * @pnum: the physical eraseblock number to check + * + * This function makes sure PEB @pnum is empty, which means it contains only + * %0xFF data bytes. Returns zero if the PEB is empty, %1 if not, and a + * negative error code in case of failure. + * + * Empty PEBs have the EC header, and do not have the VID header. The caller of + * this function should have already made sure the PEB does not have the VID + * header. However, this function re-checks that, because it is possible that + * the header and data has already been written to the PEB. + * + * Let's consider a possible scenario. Suppose there are 2 tasks - A and B. + * Task A is in 'wear_leveling_worker()'. It is reading VID header of PEB X to + * find which LEB it corresponds to. PEB X is currently unmapped, and has no + * VID header. Task B is trying to write to PEB X. + * + * Task A: in 'ubi_io_read_vid_hdr()': reads the VID header from PEB X. The + * read data contain all 0xFF bytes; + * Task B: writes VID header and some data to PEB X; + * Task A: assumes PEB X is empty, calls 'paranoid_check_empty()'. And if we + * do not re-read the VID header, and do not cancel the checking if it + * is there, we fail. + */ +static int paranoid_check_empty(struct ubi_device *ubi, int pnum) +{ + int err, offs = ubi->vid_hdr_aloffset, len = ubi->vid_hdr_alsize; + size_t read; + uint32_t magic; + const struct ubi_vid_hdr *vid_hdr; + + mutex_lock(&ubi->dbg_buf_mutex); + err = ubi->mtd->read(ubi->mtd, offs, len, &read, ubi->dbg_peb_buf); + if (err && err != -EUCLEAN) { + ubi_err("error %d while reading %d bytes from PEB %d:%d, " + "read %zd bytes", err, len, pnum, offs, read); + goto error; + } + + vid_hdr = ubi->dbg_peb_buf; + magic = be32_to_cpu(vid_hdr->magic); + if (magic == UBI_VID_HDR_MAGIC) + /* The PEB contains VID header, so it is not empty */ + goto out; + + err = check_pattern(ubi->dbg_peb_buf, 0xFF, len); + if (err == 0) { + ubi_err("flash region at PEB %d:%d, length %d does not " + "contain all 0xFF bytes", pnum, offs, len); + goto fail; + } + +out: + mutex_unlock(&ubi->dbg_buf_mutex); + return 0; + +fail: + ubi_err("paranoid check failed for PEB %d", pnum); + ubi_msg("hex dump of the %d-%d region", offs, offs + len); + print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, + ubi->dbg_peb_buf, len, 1); + err = 1; +error: + ubi_dbg_dump_stack(); + mutex_unlock(&ubi->dbg_buf_mutex); + return err; +} + #endif /* CONFIG_MTD_UBI_DEBUG_PARANOID */ -- cgit v1.2.3-70-g09d2 From 815bc5f8fe516f55291aef90f2142073821e7a9c Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Mon, 8 Jun 2009 19:28:18 +0300 Subject: UBI: fix multiple spelling typos Some of the typos were indicated by Adrian Hunter, some by 'aspell'. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/eba.c | 2 +- drivers/mtd/ubi/io.c | 2 +- drivers/mtd/ubi/ubi.h | 4 ++-- drivers/mtd/ubi/wl.c | 14 +++++++------- 4 files changed, 11 insertions(+), 11 deletions(-) (limited to 'drivers/mtd/ubi/io.c') diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c index 632b95f3ff3..b6565561218 100644 --- a/drivers/mtd/ubi/eba.c +++ b/drivers/mtd/ubi/eba.c @@ -951,7 +951,7 @@ write_error: * physical eraseblock @to. The @vid_hdr buffer may be changed by this * function. Returns: * o %0 in case of success; - * o %MOVE_CANCEL_RACE, %MOVE_TARGET_WR_ERR, or %MOVE_CANCEL_BITFLIPS; + * o %MOVE_CANCEL_RACE, %MOVE_TARGET_WR_ERR, %MOVE_CANCEL_BITFLIPS, etc; * o a negative error code in case of failure. */ int ubi_eba_copy_leb(struct ubi_device *ubi, int from, int to, diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index ac6604aeb72..effaff28bab 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -899,7 +899,7 @@ bad: * o %UBI_IO_BITFLIPS if the CRC is correct, but bit-flips were detected * and corrected by the flash driver; this is harmless but may indicate that * this eraseblock may become bad soon; - * o %UBI_IO_BAD_VID_HRD if the volume identifier header is corrupted (a CRC + * o %UBI_IO_BAD_VID_HDR if the volume identifier header is corrupted (a CRC * error detected); * o %UBI_IO_PEB_FREE if the physical eraseblock is free (i.e., there is no VID * header there); diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index 82da62bde41..70ce48b95b6 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -333,8 +333,8 @@ struct ubi_wl_entry; * protected from the wear-leveling worker) * @pq_head: protection queue head * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, - * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works and - * @erroneous_peb_count fields + * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, + * @erroneous, and @erroneous_peb_count fields * @move_mutex: serializes eraseblock moves * @work_sem: synchronizes the WL worker with use tasks * @wl_scheduled: non-zero if the wear-leveling was scheduled diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index acb5520f7f3..2b247230061 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -83,7 +83,7 @@ * used. The former state corresponds to the @wl->free tree. The latter state * is split up on several sub-states: * o the WL movement is allowed (@wl->used tree); - * o the WL movement is disallowed (@wl->erroneous) becouse the PEB is + * o the WL movement is disallowed (@wl->erroneous) because the PEB is * erroneous - e.g., there was a read error; * o the WL movement is temporarily prohibited (@wl->pq queue); * o scrubbing is needed (@wl->scrub tree). @@ -744,8 +744,8 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk, * given, so we have a situation when it has not yet * had a chance to write it, because it was preempted. * So add this PEB to the protection queue so far, - * because presubably more data will be written there - * (including the missin VID header), and then we'll + * because presumably more data will be written there + * (including the missing VID header), and then we'll * move it. */ dbg_wl("PEB %d has no VID header", e1->pnum); @@ -790,8 +790,8 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk, * not switch to R/O mode in this case, and give the * upper layers a possibility to recover from this, * e.g. by unmapping corresponding LEB. Instead, just - * put thie PEB to the @ubi->erroneus list to prevent - * UBI from trying to move the over and over again. + * put this PEB to the @ubi->erroneous list to prevent + * UBI from trying to move it over and over again. */ if (ubi->erroneous_peb_count > ubi->max_erroneous) { ubi_err("too many erroneous eraseblocks (%d)", @@ -1045,7 +1045,7 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk, /* * If this is not %-EIO, we have no idea what to do. Scheduling * this physical eraseblock for erasure again would cause - * errors again and again. Well, lets switch to RO mode. + * errors again and again. Well, lets switch to R/O mode. */ goto out_ro; } @@ -1161,7 +1161,7 @@ retry: rb_erase(&e->u.rb, &ubi->erroneous); ubi->erroneous_peb_count -= 1; ubi_assert(ubi->erroneous_peb_count >= 0); - /* Erronious PEBs should be tortured */ + /* Erroneous PEBs should be tortured */ torture = 1; } else { err = prot_queue_del(ubi, e->pnum); -- cgit v1.2.3-70-g09d2