[patch] md: bitmap read_page error

[patch] md: bitmap read_page error

[yang yin]

If the bitmap size is less than one page including super_block and
bitmap and the inode's i_blkbits is also small, when doing the
read_page function call to read the sb_page, it may return a error.
For example, if the device is 12800 chunks, its bitmap file size is
about 1.6KB include the bitmap super block. But the inode i_blkbits
value of the bitmap file is 10,  the read_page will submit 4 bh to
load the sb_page. Because the size of bitmap is only 1.6KB, in the
while loop, the error will ocurr when do bmap operation for the block
2, which will  return 0. Then the bitmap can't be initated because of
ther read sb page fail.

Another error is in the bitmap_init_from_disk function.  Before doing
read_page,. calculating the count value misses the size of super
block. When the bitmap just needs one page, It will read two pages
adding the super block. But at the second read, the count value will
be set to 0, and not all the bitmap will be read from the disk and
some bitmap will missed at the second page.

I give a patch as following:

---------
diff -Nur linux-2.6.19.2.orig/drivers/md/bitmap.c
linux-2.6.19.2/drivers/md/bitmap.c
--- linux-2.6.19.2.orig/drivers/md/bitmap.c     2007-01-11
03:10:37.000000000 +0800
+++ linux-2.6.19.2/drivers/md/bitmap.c  2007-01-20 20:45:32.000000000 +0800
@@ -352,6 +352,7 @@
        struct inode *inode = file->f_dentry->d_inode;
        struct buffer_head *bh;
        sector_t block;
+       loff_t read_size = 0;

        PRINTK("read bitmap file (%dB @ %Lu)\n", (int)PAGE_SIZE,
                        (unsigned long long)index << PAGE_SHIFT);
@@ -371,7 +372,7 @@
        attach_page_buffers(page, bh);
        block = index << (PAGE_SHIFT - inode->i_blkbits);
        while (bh) {
-               if (count == 0)
+               if (count == 0 || (read_size >= (inode->i_size -
(index << PAGE_SHIFT))))
                        bh->b_blocknr = 0;
                else {
                        bh->b_blocknr = bmap(inode, block);
@@ -394,6 +395,7 @@
                        set_buffer_mapped(bh);
                        submit_bh(READ, bh);
                }
+               read_size += (1 << inode->i_blkbits);
                block++;
                bh = bh->b_this_page;
        }
@@ -877,7 +879,7 @@
                        int count;
                        /* unmap the old page, we're done with it */
                        if (index == num_pages-1)
-                               count = bytes - index * PAGE_SIZE;
+                               count = bytes + sizeof(bitmap_super_t)
- index * PAGE_SIZE;
                        else
                                count = PAGE_SIZE;
                        if (index == 0) {


yinyang

________________________________
Tel: (86)10-62600547
Fax: (86)10-6265-7255
Mailing: P. O. Box 2704# Beijing
Postcode: 100080
National Research Centre for High Performance Computer
Institute of Computing Technology,Chinese Academy of Sciences
6,South Kexueyuan Road,
Haidian District, Beijing, China

Re: [patch] md: bitmap read_page error

[Neil Brown]

On Monday January 22, yinyang801120@gmail.com wrote:
> If the bitmap size is less than one page including super_block and
> bitmap and the inode's i_blkbits is also small, when doing the
> read_page function call to read the sb_page, it may return a error.
> For example, if the device is 12800 chunks, its bitmap file size is
> about 1.6KB include the bitmap super block. But the inode i_blkbits
> value of the bitmap file is 10,  the read_page will submit 4 bh to
> load the sb_page. Because the size of bitmap is only 1.6KB, in the
> while loop, the error will ocurr when do bmap operation for the block
> 2, which will  return 0. Then the bitmap can't be initated because of
> ther read sb page fail.
> 
> Another error is in the bitmap_init_from_disk function.  Before doing
> read_page,. calculating the count value misses the size of super
> block. When the bitmap just needs one page, It will read two pages
> adding the super block. But at the second read, the count value will
> be set to 0, and not all the bitmap will be read from the disk and
> some bitmap will missed at the second page.
> 
> I give a patch as following:

Thanks a lot for this.
Rather than checking the file size in read_page, I would like to make
sure the 'count' that is passed in never exceeds the size of the
file.  This should have the same effect.

So this is that patch I plan to submit.

Thanks again,
NeilBrown


------------------
Avoid reading past the end of a bitmap file.

In most cases we check the size of the bitmap file before
reading data from it.  However when reading the superblock,
we always read the first PAGE_SIZE bytes, which might not 
always be appropriate.  So limit that read to the size of the
file if appropriate.

Also, we get the count of available bytes wrong in one place,
so that too can read past the end of the file.

Cc:  "yang yin" <yinyang801120@gmail.com>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./drivers/md/bitmap.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
--- .prev/drivers/md/bitmap.c	2007-01-23 09:44:11.000000000 +1100
+++ ./drivers/md/bitmap.c	2007-01-23 09:44:59.000000000 +1100
@@ -479,9 +479,12 @@ static int bitmap_read_sb(struct bitmap 
 	int err = -EINVAL;
 
 	/* page 0 is the superblock, read it... */
-	if (bitmap->file)
-		bitmap->sb_page = read_page(bitmap->file, 0, bitmap, PAGE_SIZE);
-	else {
+	if (bitmap->file) {
+		loff_t isize = i_size_read(bitmap->file->f_mapping->host);
+		int bytes = isize > PAGE_SIZE ? PAGE_SIZE : isize;
+
+		bitmap->sb_page = read_page(bitmap->file, 0, bitmap, bytes);
+	} else {
 		bitmap->sb_page = read_sb_page(bitmap->mddev, bitmap->offset, 0);
 	}
 	if (IS_ERR(bitmap->sb_page)) {
@@ -877,7 +880,8 @@ static int bitmap_init_from_disk(struct 
 			int count;
 			/* unmap the old page, we're done with it */
 			if (index == num_pages-1)
-				count = bytes - index * PAGE_SIZE;
+				count = bytes + sizeof(bitmap_super_t)
+					- index * PAGE_SIZE;
 			else
 				count = PAGE_SIZE;
 			if (index == 0) {

Re: [patch] md: bitmap read_page error

[yang yin]

I think your patch is not enough to slove the read_page error
completely. I think in the bitmap_init_from_disk we also need to check
the 'count' never exceeds the size of file before calling the
read_page function. How do your think about it.
Thanks your reply.

2007/1/23, Neil Brown <neilb@suse.de>:
> On Monday January 22, yinyang801120@gmail.com wrote:
> > If the bitmap size is less than one page including super_block and
> > bitmap and the inode's i_blkbits is also small, when doing the
> > read_page function call to read the sb_page, it may return a error.
> > For example, if the device is 12800 chunks, its bitmap file size is
> > about 1.6KB include the bitmap super block. But the inode i_blkbits
> > value of the bitmap file is 10,  the read_page will submit 4 bh to
> > load the sb_page. Because the size of bitmap is only 1.6KB, in the
> > while loop, the error will ocurr when do bmap operation for the block
> > 2, which will  return 0. Then the bitmap can't be initated because of
> > ther read sb page fail.
> >
> > Another error is in the bitmap_init_from_disk function.  Before doing
> > read_page,. calculating the count value misses the size of super
> > block. When the bitmap just needs one page, It will read two pages
> > adding the super block. But at the second read, the count value will
> > be set to 0, and not all the bitmap will be read from the disk and
> > some bitmap will missed at the second page.
> >
> > I give a patch as following:
>
> Thanks a lot for this.
> Rather than checking the file size in read_page, I would like to make
> sure the 'count' that is passed in never exceeds the size of the
> file.  This should have the same effect.
>
> So this is that patch I plan to submit.
>
> Thanks again,
> NeilBrown
>
>
> ------------------
> Avoid reading past the end of a bitmap file.
>
> In most cases we check the size of the bitmap file before
> reading data from it.  However when reading the superblock,
> we always read the first PAGE_SIZE bytes, which might not
> always be appropriate.  So limit that read to the size of the
> file if appropriate.
>
> Also, we get the count of available bytes wrong in one place,
> so that too can read past the end of the file.
>
> Cc:  "yang yin" <yinyang801120@gmail.com>
> Signed-off-by: Neil Brown <neilb@suse.de>
>
> ### Diffstat output
>  ./drivers/md/bitmap.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
> --- .prev/drivers/md/bitmap.c   2007-01-23 09:44:11.000000000 +1100
> +++ ./drivers/md/bitmap.c       2007-01-23 09:44:59.000000000 +1100
> @@ -479,9 +479,12 @@ static int bitmap_read_sb(struct bitmap
>         int err = -EINVAL;
>
>         /* page 0 is the superblock, read it... */
> -       if (bitmap->file)
> -               bitmap->sb_page = read_page(bitmap->file, 0, bitmap, PAGE_SIZE);
> -       else {
> +       if (bitmap->file) {
> +               loff_t isize = i_size_read(bitmap->file->f_mapping->host);
> +               int bytes = isize > PAGE_SIZE ? PAGE_SIZE : isize;
> +
> +               bitmap->sb_page = read_page(bitmap->file, 0, bitmap, bytes);
> +       } else {
>                 bitmap->sb_page = read_sb_page(bitmap->mddev, bitmap->offset, 0);
>         }
>         if (IS_ERR(bitmap->sb_page)) {
> @@ -877,7 +880,8 @@ static int bitmap_init_from_disk(struct
>                         int count;
>                         /* unmap the old page, we're done with it */
>                         if (index == num_pages-1)
> -                               count = bytes - index * PAGE_SIZE;
> +                               count = bytes + sizeof(bitmap_super_t)
> +                                       - index * PAGE_SIZE;
>                         else
>                                 count = PAGE_SIZE;
>                         if (index == 0) {
>

Re: [patch] md: bitmap read_page error

[Neil Brown]

On Tuesday January 23, yinyang801120@gmail.com wrote:
> I think your patch is not enough to slove the read_page error
> completely. I think in the bitmap_init_from_disk we also need to check
> the 'count' never exceeds the size of file before calling the
> read_page function. How do your think about it.
> Thanks your reply.

bitmap_init_from_disk already has a test:

	if (file && i_size_read(file->f_mapping->host) < bytes + sizeof(bitmap_super_t)) {
		printk(KERN_INFO "%s: bitmap file too short %lu < %lu\n",
			bmname(bitmap),
			(unsigned long) i_size_read(file->f_mapping->host),
			bytes + sizeof(bitmap_super_t));
		goto out;
	}

so 'bytes' of the bitmap must fit entirely within the file, and count
is set:
			if (index == num_pages-1)
				count = bytes + sizeof(bitmap_super_t)
					- index * PAGE_SIZE;
			else
				count = PAGE_SIZE;
which ensures that it will not go beyond the end of the file.

So I don't think count can ever exceed the size of the file in this
case. 

Can you still see a problem?

Thanks,
NeilBrown