Bug in fs/reiserfs/file.c

Bug in fs/reiserfs/file.c

[Neil Brown]

In fs/reiserfs/file.c, in reiserfs_file_write, at line 1400 in
2.6.16-rc2-mm1 we have

		size_t blocks_to_allocate;	/* how much blocks we need to allocate for this iteration */

size_t is an unsigned type.

Later (line 1467) we have code like:

		blocks_to_allocate =
		    reiserfs_prepare_file_region_for_write(inode, pos,
							   num_pages,
							   write_bytes,
							   prepared_pages);
		if (blocks_to_allocate < 0) {
			res = blocks_to_allocate;
			reiserfs_release_claimed_blocks(inode->i_sb,
							num_pages <<
							(PAGE_CACHE_SHIFT -
							 inode->i_blkbits));
			break;
		}


Spot the bug.... reiserfs_prepare_file_region_for_write can return a
negative error status, but blocks_to_allocate won't store it, and
things go wrong.

The actual result if reiserfs_prepare_file_region_for_write returns
negative is that a subsequent call to 
			    reiserfs_allocate_blocks_for_region(&th, inode, pos,
								num_pages,
								write_bytes,
								prepared_pages,
								blocks_to_allocate);
trys to kmalloc an enormous amount of memory
	allocated_blocks = kmalloc((blocks_to_allocate + will_prealloc) *
				   sizeof(b_blocknr_t), GFP_NOFS);

and fails so
		if (res) {
			reiserfs_unprepare_pages(prepared_pages, num_pages);
			break;
		}

which tries to unlock the pages in prepared_pages. But
reiserfs_prepare_file_region_for_write didn't leave any locked pages
in their (due to it's failure) and try_to_free_buffers BUGs out.

The "obvious" fix it to change the 'size_t' to 'ssize_t', but I'll
leave to to reiserfs-dev to create and submit a patch....


As an aside, 
  info gcc
tells me that '-W' will cause a warning when

        * An unsigned value is compared against zero with `<' or `<='.

It doesn't :-(

NeilBrown

Re: Bug in fs/reiserfs/file.c

[Neil Brown]

On Tuesday February 28, neilb@suse.de wrote:
> 
> 
> As an aside, 
>   info gcc
> tells me that '-W' will cause a warning when
> 
>         * An unsigned value is compared against zero with `<' or `<='.
> 
> It doesn't :-(

Actually, it does.  However Linux isn't compiled with "-W".
I'm experimenting with

diff ./Makefile~current~ ./Makefile
--- ./Makefile~current~	2006-02-28 14:44:21.000000000 +1100
+++ ./Makefile	2006-02-28 14:44:51.000000000 +1100
@@ -306,7 +306,7 @@ LINUXINCLUDE    := -Iinclude \
 
 CPPFLAGS        := -D__KERNEL__ $(LINUXINCLUDE)
 
-CFLAGS 		:= -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
+CFLAGS 		:= -W -Wall -Wno-unused -Wno-sign-compare -Wundef -Wstrict-prototypes -Wno-trigraphs \
 	  	   -fno-strict-aliasing -fno-common \
 		   -ffreestanding
 AFLAGS		:= -D__ASSEMBLY__



and it reports the following.

I think all of these are harmless except the file.c:1472 one (I
think), but it probably wouldn't hurt to tidy them up.

NeilBrown


  GEN    /home/src/mm-i386/Makefile
scripts/kconfig/conf -s arch/i386/Kconfig
#
# using defaults found in .config
#
  SPLIT   include/linux/autoconf.h -> include/config/*
  Using /home/src/mm as source for kernel
  GEN    /home/src/mm-i386/Makefile
  CHK     include/linux/version.h
  CC      fs/reiserfs/bitmap.o
  CC      fs/reiserfs/do_balan.o
  CC      fs/reiserfs/namei.o
/home/src/mm/fs/reiserfs/namei.c: In function `reiserfs_lookup':
/home/src/mm/fs/reiserfs/namei.c:370: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c: In function `reiserfs_create':
/home/src/mm/fs/reiserfs/namei.c:629: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c:673: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c: In function `reiserfs_mknod':
/home/src/mm/fs/reiserfs/namei.c:704: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c:752: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c: In function `reiserfs_mkdir':
/home/src/mm/fs/reiserfs/namei.c:783: warning: empty body in an if-statement
/home/src/mm/fs/reiserfs/namei.c:838: warning: empty body in an if-statement
  CC      fs/reiserfs/inode.o
/home/src/mm/fs/reiserfs/inode.c: In function `reiserfs_get_block':
/home/src/mm/fs/reiserfs/inode.c:630: warning: comparison of unsigned expression < 0 is always false
/home/src/mm/fs/reiserfs/inode.c:942: warning: comparison of unsigned expression < 0 is always false
  CC      fs/reiserfs/file.o
/home/src/mm/fs/reiserfs/file.c: In function `reiserfs_file_write':
/home/src/mm/fs/reiserfs/file.c:1472: warning: comparison of unsigned expression < 0 is always false
  CC      fs/reiserfs/dir.o
  CC      fs/reiserfs/fix_node.o
/home/src/mm/fs/reiserfs/fix_node.c: In function `create_virtual_node':
/home/src/mm/fs/reiserfs/fix_node.c:196: warning: empty body in an else-statement
  CC      fs/reiserfs/super.o
/home/src/mm/fs/reiserfs/super.c:687: warning: missing initializer
/home/src/mm/fs/reiserfs/super.c:687: warning: (near initialization for `logging_mode[3].clrmask')
/home/src/mm/fs/reiserfs/super.c:694: warning: missing initializer
/home/src/mm/fs/reiserfs/super.c:694: warning: (near initialization for `barrier_mode[2].clrmask')
/home/src/mm/fs/reiserfs/super.c: In function `reiserfs_parse_options':
/home/src/mm/fs/reiserfs/super.c:892: warning: missing initializer
/home/src/mm/fs/reiserfs/super.c:892: warning: (near initialization for `opts[9].arg_required')
/home/src/mm/fs/reiserfs/super.c:910: warning: missing initializer
/home/src/mm/fs/reiserfs/super.c:910: warning: (near initialization for `opts[25].arg_required')
  CC      fs/reiserfs/prints.o
  CC      fs/reiserfs/objectid.o
  CC      fs/reiserfs/lbalance.o
  CC      fs/reiserfs/ibalance.o
  CC      fs/reiserfs/stree.o
  CC      fs/reiserfs/hashes.o
  CC      fs/reiserfs/tail_conversion.o
  CC      fs/reiserfs/journal.o
/home/src/mm/fs/reiserfs/journal.c: In function `journal_read':
/home/src/mm/fs/reiserfs/journal.c:2322: warning: comparison of unsigned expression >= 0 is always true
  CC      fs/reiserfs/resize.o
  CC      fs/reiserfs/item_ops.o
  CC      fs/reiserfs/ioctl.o
  CC      fs/reiserfs/procfs.o
  LD      fs/reiserfs/reiserfs.o
  LD      fs/reiserfs/built-in.o

Re: Bug in fs/reiserfs/file.c

[Hans Reiser]

Thanks Neil, I'll have vs evaluate and fix this.

Hans

Neil Brown wrote:

>In fs/reiserfs/file.c, in reiserfs_file_write, at line 1400 in
>2.6.16-rc2-mm1 we have
>
>		size_t blocks_to_allocate;	/* how much blocks we need to allocate for this iteration */
>
>size_t is an unsigned type.
>
>Later (line 1467) we have code like:
>
>		blocks_to_allocate =
>		    reiserfs_prepare_file_region_for_write(inode, pos,
>							   num_pages,
>							   write_bytes,
>							   prepared_pages);
>		if (blocks_to_allocate < 0) {
>			res = blocks_to_allocate;
>			reiserfs_release_claimed_blocks(inode->i_sb,
>							num_pages <<
>							(PAGE_CACHE_SHIFT -
>							 inode->i_blkbits));
>			break;
>		}
>
>
>Spot the bug.... reiserfs_prepare_file_region_for_write can return a
>negative error status, but blocks_to_allocate won't store it, and
>things go wrong.
>
>The actual result if reiserfs_prepare_file_region_for_write returns
>negative is that a subsequent call to 
>			    reiserfs_allocate_blocks_for_region(&th, inode, pos,
>								num_pages,
>								write_bytes,
>								prepared_pages,
>								blocks_to_allocate);
>trys to kmalloc an enormous amount of memory
>	allocated_blocks = kmalloc((blocks_to_allocate + will_prealloc) *
>				   sizeof(b_blocknr_t), GFP_NOFS);
>
>and fails so
>		if (res) {
>			reiserfs_unprepare_pages(prepared_pages, num_pages);
>			break;
>		}
>
>which tries to unlock the pages in prepared_pages. But
>reiserfs_prepare_file_region_for_write didn't leave any locked pages
>in their (due to it's failure) and try_to_free_buffers BUGs out.
>
>The "obvious" fix it to change the 'size_t' to 'ssize_t', but I'll
>leave to to reiserfs-dev to create and submit a patch....
>
>
>As an aside, 
>  info gcc
>tells me that '-W' will cause a warning when
>
>        * An unsigned value is compared against zero with `<' or `<='.
>
>It doesn't :-(
>
>NeilBrown
>
>
>  
>