[PATCH 0/4] mtd-utils: integck improvements

[PATCH 0/4] mtd-utils: integck improvements

[Elie De Brauwer]

Hello all,

The past couple of days I've been doing some NAND stress testing on an
i.MX28 platform. So far my observation is that at a certain point 
integck writes a buffer, then tries to verify this data, concludes the
buffer is not what it was written and fails. However the file it saves
(and the contents on flash) appear to be the correct data, hence the 
standard integck wasn't very useful in assisting troubleshooting this 
issue (which only appears once every couple of hours). 

So to step you through the patches:

'verify the operation after datastructure update':

At first integck failed with a similar dump:
<quote>
integck:     File Data:
integck:         Offset: 0  Size: 196  Seed: 5999877  R.Off: 0
integck:         Offset: 196  Size: 33  Seed: 4160795  R.Off: 0
integck:         Offset: 229  Size: 1252  Seed: 8070052  R.Off: 0
integck:         Offset: 1481  Size: 612  Seed: 4160795  R.Off: 1285
integck:         Offset: 2093  Size: 6  Seed: 6946586  R.Off: 0
integck:         Offset: 2099  Size: 536  Seed: 4160795  R.Off: 1903
integck:         Offset: 2635  Size: 1562  Seed: 9845455  R.Off: 0
integck:         Offset: 4197  Size: 80  Seed: 702818  R.Off: 0
integck:         Offset: 4277  Size: 115  Seed: 9845455  R.Off: 1642
integck:     9 writes
integck:     ============================================
integck:     Write Info:
integck:         Offset: 826  Size: 357  Seed: 5908448  R.Off: 0
integck:         Offset: 4197  Size: 80  Seed: 702818  R.Off: 0
...
</quote>
And I would expect the file data listing to include at offset 826 something
with a size of 357 and a seed of 5908448. Clearly it is not there (which
is already extremely confusing). The point is that file_write_info first 
updates the raw_write, then verifies the data (passing the new write) 
and only after that updates the write structure. But in file_check_data
only the newly written data is verified (passed as an argument) whilst 
the save_file() function to dump the file uses the raw_writes to recreate
the written data (while raw_writes is only updated after after this check
would have succeeded). Several lines to say that in this patch the verify
only gets called _after_ the datastructures are updated. 

'file_check_data update to dump the buffer':

See my problem description above, the point is that integck in file_check_data
reads a buffer, and then checks if the data is correct,  it will do a 
seek(0), and reread from the same fd. The point is that in the scenario I 
observed integck failed (due to a buffer mismatch) but the it saved (and 
what was in flash) was actually correct. So I modified this function to
dump the buffers to stderr at the moment an error is found.

'path buffer overflow':

In the problem above I've spend several hours waiting for the issue to 
appear, only to had the 'luck' that it was found in a file whose name was
256 bytes in length, resulting in the write to fail. Closer examination 
showed that the buffer to store the path was 256 bytes in length, but this
buffer also includes /tmp and the read/write suffix and should be able to
contain a filename which is up to 255 bytes (NAME_MAX in linux/limits.h)
in size which is a bad fit. So that array is modified to FILENAME_MAX
(stdio_lim.h) and some checking is added to truncate the filename should
it cause an overflow.

The following log shows the first patch in action (see the correct seed), 
and shows why this third patch is needed:
<quote>
integck:     File Data:
integck:         Offset: 0  Size: 1  Seed: 5008310  R.Off: 0
integck:     1 writes
integck:     ============================================
integck:     Write Info:
integck:         Offset: 0  Size: 1  Seed: 5008310  R.Off: 0
integck:         Offset: 0  Size: 1  Seed: 8246352  R.Off: 0
integck:         Offset: 0  Size: 1  Seed: 5078796  R.Off: 0
integck:         Offset: 0  Size: 1  Seed: 2267087  R.Off: 0
integck:         Offset: 0  Size: 1  Seed: 3602680  R.Off: 0
integck:     5 writes or truncations
integck:     ============================================
integck: Saving /tmp/yqcnfygfitaatyeyvffrguegcdttamcnyhowhgieljfuxfipiljsjcbluaeaghwyinkggommsbwnmvekihgnwgiibccpbwfrpxuxwkmnyghnutrudienngxwgorudbskedaaekiuiyqksfazrwzfwbfhzjjqoiulebtlpbfiuffmsnguqkjzqjqizimsmhbqqagaebjdhqwmzdxghiavtcxubegawlgtvstuqurkurpnrckjfkgostdtpg.integ.sav.readn
integck: error!: condition 'w_fd != -1' failed in save_file() at integck.c:1445
integck: error 36 (File name too long)
</quote>

'typo fixes':

Something trivial to end with. 

my 2 cents, feedback welcome but keep me in cc since I'm not on the list.
E.



Elie De Brauwer (4):
  integck.c: Only verify the operation after all datastructures have
    been updated
  integck.c: rework file_check_data to immediately dump the buffer
    containing the errors
  integck.c: Fix buffer overflow in save_file, avoid possible failure
    to write buffers when the filename length is equal to max_name_len
  Typo fixes: avaiable -> available and priortiry ->  priority

 mkfs.jffs2.1                       |   10 +++---
 mkfs.jffs2.c                       |    4 +--
 tests/fs-tests/integrity/integck.c |   61 +++++++++++++++++++++++-------------
 3 files changed, 47 insertions(+), 28 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] integck.c: Only verify the operation after all datastructures have been updated

[Elie De Brauwer]

Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
---
 tests/fs-tests/integrity/integck.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
index 087a18b..2c6ffea 100644
--- a/tests/fs-tests/integrity/integck.c
+++ b/tests/fs-tests/integrity/integck.c
@@ -960,9 +960,6 @@ static void file_write_info(struct file_info *file, int fd, off_t offset,
 	w->random_seed = seed;
 	file->raw_writes = w;
 
-	if (args.verify_ops && !args.power_cut_mode)
-		file_check_data(file, fd, new_write);
-
 	/* Insert it into file->writes */
 	inserted = 0;
 	end = offset + size;
@@ -1028,6 +1025,9 @@ static void file_write_info(struct file_info *file, int fd, off_t offset,
 	/* Update file length */
 	if (end > file->length)
 		file->length = end;
+
+	if (args.verify_ops && !args.power_cut_mode)
+		file_check_data(file, fd, new_write);
 }
 
 /* Randomly select offset and and size to write in a file */
-- 
1.7.10.4

[PATCH 2/4] integck.c: rework file_check_data to immediately dump the buffer containing the errors

[Elie De Brauwer]

Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
---
 tests/fs-tests/integrity/integck.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
index 2c6ffea..5ea3642 100644
--- a/tests/fs-tests/integrity/integck.c
+++ b/tests/fs-tests/integrity/integck.c
@@ -1502,7 +1502,8 @@ static void file_check_data(struct file_info *file, int fd,
 {
 	size_t remains, block, i;
 	off_t r;
-	char buf[IO_BUFFER_SIZE];
+	unsigned char read_buf[IO_BUFFER_SIZE];
+	unsigned char check_buf[IO_BUFFER_SIZE];
 	unsigned int seed = w->random_seed;
 
 	if (args.power_cut_mode && !file->clean)
@@ -1517,17 +1518,28 @@ static void file_check_data(struct file_info *file, int fd,
 			block = IO_BUFFER_SIZE;
 		else
 			block = remains;
-		CHECK(read(fd, buf, block) == block);
-		for (i = 0; i < block; ++i) {
-			char c = (char)rand_r(&seed);
-			if (buf[i] != c) {
-				errmsg("file_check_data failed at %zu checking "
-				       "data at %llu size %zu", w->size - remains + i,
-					(unsigned long long)w->offset, w->size);
-				file_info_display(file);
-				save_file(fd, file);
-			}
-			CHECK(buf[i] == c);
+		CHECK(read(fd, read_buf, block) == block);
+		for (i = 0; i < block; ++i)
+			check_buf[i] = (char)rand_r(&seed);
+
+		if (memcmp(check_buf, read_buf, block) != 0) {
+			errmsg("file_check_data failed, dumping "
+				"data at offset %llu size %zu",
+				(unsigned long long)w->offset, w->size);
+
+			fprintf (stderr, "Read data:\n");
+			for (r = 0; r < block; ++r)
+				fprintf(stderr, "%02x%c",
+					read_buf[r], ((r+1)%16)?' ':'\n');
+			fprintf(stderr, "\nExpected data:\n");
+			for (r = 0; r < block; ++r)
+				fprintf(stderr, "%02x%c",
+					check_buf[r], ((r+1)%16)?' ':'\n');
+			fprintf(stderr, " \n");
+
+			file_info_display(file);
+			save_file(fd, file);
+			CHECK(0);
 		}
 		remains -= block;
 	}
-- 
1.7.10.4

[PATCH 3/4] integck.c: Fix buffer overflow in save_file, avoid possible failure to write buffers when the filename length is equal to max_name_len

[Elie De Brauwer]

Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
---
 tests/fs-tests/integrity/integck.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
index 5ea3642..ee37a0d 100644
--- a/tests/fs-tests/integrity/integck.c
+++ b/tests/fs-tests/integrity/integck.c
@@ -32,11 +32,11 @@
 #include <assert.h>
 #include <mntent.h>
 #include <execinfo.h>
+#include <bits/stdio_lim.h>
 #include <sys/mman.h>
 #include <sys/vfs.h>
 #include <sys/mount.h>
 #include <sys/statvfs.h>
-#include <linux/fs.h>
 
 #define PROGRAM_VERSION "1.1"
 #define PROGRAM_NAME "integck"
@@ -1433,12 +1433,17 @@ static void save_file(int fd, struct file_info *file)
 	int w_fd;
 	struct write_info *w;
 	char buf[IO_BUFFER_SIZE];
-	char name[256];
+	char name[FILENAME_MAX];
+        const char * read_suffix = ".integ.sav.read";
+        const char * write_suffix = ".integ.sav.written";
+        size_t fname_len = strlen(get_file_name(file));
 
 	/* Open file to save contents to */
 	strcpy(name, "/tmp/");
-	strcat(name, get_file_name(file));
-	strcat(name, ".integ.sav.read");
+	if (fname_len + strlen(read_suffix) > fsinfo.max_name_len)
+		fname_len = fsinfo.max_name_len - strlen(read_suffix);
+	strncat(name, get_file_name(file), fname_len);
+	strcat(name, read_suffix);
 	normsg("Saving %sn", name);
 	w_fd = open(name, O_CREAT | O_WRONLY, 0777);
 	CHECK(w_fd != -1);
@@ -1457,8 +1462,10 @@ static void save_file(int fd, struct file_info *file)
 
 	/* Open file to save contents to */
 	strcpy(name, "/tmp/");
-	strcat(name, get_file_name(file));
-	strcat(name, ".integ.sav.written");
+	if (fname_len + strlen(write_suffix) > fsinfo.max_name_len)
+		fname_len = fsinfo.max_name_len - strlen(write_suffix);
+	strncat(name, get_file_name(file), fname_len);
+	strcat(name, write_suffix);
 	normsg("Saving %s", name);
 	w_fd = open(name, O_CREAT | O_WRONLY, 0777);
 	CHECK(w_fd != -1);
-- 
1.7.10.4

[PATCH 4/4] Typo fixes: avaiable -> available and priortiry -> priority

[Elie De Brauwer]

Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
---
 mkfs.jffs2.1 |   10 +++++-----
 mkfs.jffs2.c |    4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/mkfs.jffs2.1 b/mkfs.jffs2.1
index 47a3ac7..7c57ddc 100644
--- a/mkfs.jffs2.1
+++ b/mkfs.jffs2.1
@@ -105,7 +105,7 @@ or the present directory, if the
 option is not specified.
 
 Each block of the files to be placed into the file system image
-are compressed using one of the avaiable compressors depending
+are compressed using one of the available compressors depending
 on the selected compression mode.
 
 File systems are created with the same endianness as the host,
@@ -209,21 +209,21 @@ successful one. The alternatives are:
 .B -x, --disable-compressor=NAME
 Disable a compressor. Use
 .B -L
-to see the list of the avaiable compressors and their default states.
+to see the list of the available compressors and their default states.
 .TP
 .B -X, --enable-compressor=NAME
 Enable a compressor. Use
 .B -L
-to see the list of the avaiable compressors and their default states.
+to see the list of the available compressors and their default states.
 .TP
 .B -y, --compressor-priority=PRIORITY:NAME
 Set the priority of a compressor. Use
 .B -L
-to see the list of the avaiable compressors and their default priority.
+to see the list of the available compressors and their default priority.
 Priorities are used by priority compression mode.
 .TP
 .B -L, --list-compressors
-Show the list of the avaiable compressors and their states.
+Show the list of the available compressors and their states.
 .TP
 .B -t, --test-compression
 Call decompress after every compress - and compare the result with the original data -, and
diff --git a/mkfs.jffs2.c b/mkfs.jffs2.c
index a33a0d6..7ade078 100644
--- a/mkfs.jffs2.c
+++ b/mkfs.jffs2.c
@@ -1396,14 +1396,14 @@ static const char helptext[] =
 "                          page size (default: 4KiB)\n"
 "  -e, --eraseblock=SIZE   Use erase block size SIZE (default: 64KiB)\n"
 "  -c, --cleanmarker=SIZE  Size of cleanmarker (default 12)\n"
-"  -m, --compr-mode=MODE   Select compression mode (default: priortiry)\n"
+"  -m, --compr-mode=MODE   Select compression mode (default: priority)\n"
 "  -x, --disable-compressor=COMPRESSOR_NAME\n"
 "                          Disable a compressor\n"
 "  -X, --enable-compressor=COMPRESSOR_NAME\n"
 "                          Enable a compressor\n"
 "  -y, --compressor-priority=PRIORITY:COMPRESSOR_NAME\n"
 "                          Set the priority of a compressor\n"
-"  -L, --list-compressors  Show the list of the avaiable compressors\n"
+"  -L, --list-compressors  Show the list of the available compressors\n"
 "  -t, --test-compression  Call decompress and compare with the original (for test)\n"
 "  -n, --no-cleanmarkers   Don't add a cleanmarker to every eraseblock\n"
 "  -o, --output=FILE       Output to FILE (default: stdout)\n"
-- 
1.7.10.4

Re: [PATCH 0/4] mtd-utils: integck improvements

[Artem Bityutskiy]

On Fri, 2013-03-01 at 19:37 +0100, Elie De Brauwer wrote:
> Hello all,
> 
> The past couple of days I've been doing some NAND stress testing on an
> i.MX28 platform. So far my observation is that at a certain point 
> integck writes a buffer, then tries to verify this data, concludes the
> buffer is not what it was written and fails. However the file it saves
> (and the contents on flash) appear to be the correct data, hence the 
> standard integck wasn't very useful in assisting troubleshooting this 
> issue (which only appears once every couple of hours). 

Hi, first of all - thanks for the hard work and the fixes. This test was
use on early days when we were developing UBIFS, and was extremely
useful for us. But we mostly used it with nandsim.

When UBIFS became stable, I add ed the "power cut emulation" support
there, and this is probably when I broke the report printing
functionality.

The test is not very cleanly written, and it is a bit of spaghetti,
which did not scare you off, and for which you get my kudos :-)

I've applied and pushed all your patches. I only briefly looked at them,
they looked OK. And I only compile-tested them. I just have not time
right now for more thorough review, and I forgot how integck works, and
your patches looked very trustworthy, so I just pushed them out.

I added your description to the commit messages. This e-mail will be
lost in the archives, but the commit messages will stay in the git
history and may be useful. Keep this in mind - put useful descriptive
comment so that they stay in the commit message, do not put all of them
to the cover letter.

I also amended few broken indentations - we indent using tabs, as a
convention. We do use spaces to fine-tune the indentation in tricky
places, but the basic style is using tabs.

Thanks!

-- 
Best Regards,
Artem Bityutskiy