* Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested @ 2004-01-05 23:20 Jesper Juhl 2004-01-06 8:51 ` Hans Reiser 0 siblings, 1 reply; 20+ messages in thread From: Jesper Juhl @ 2004-01-05 23:20 UTC (permalink / raw) To: Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson Cc: linux-kernel Hi, First of all, please let me explain the large number of recipients. I initially posted a mail about this issue to lkml asking for confirmation that what I'd found really was a bug. I got no reply appart from a single suggestion that I attempt to email the maintainers of the filesystems in question directly - this is what I'm now doing. I've included lkml as CC in case someone there should know something but missed the original mail. The problem is what seems (to me, but I could be dead wrong) to be invalid use of variables of type sector_t. Let me use fs/ufs/inode.c as the example : on lines 334 & 335 there is this code if (fragment < 0) goto abort_negative; fragment being of type sector_t which (as far as I've been able to find out) is always an unsigned datatype. That means that it can never be < 0 and thus the branch will never be taken and the code in abort_negative never executed. Looks like a bug to me. I initially discovered this in fs/ufs/inode.c line 334 (this is all releative to 2.6.1-rc1-mm1 btw), but when I started looking for it elsewere I found the same (or similar) issue in fs/adfs/inode.c line 30, fs/befs/linuxvfs.c line 135, fs/bfs/file.c line 67 & fs/reiserfs/inode.c line 577 When I went digging for the actual type of sector_t I found the following: include/linux/types.h defines sector_t as typedef unsigned long sector_t; and in include/asm* sector_t is defined as : asm-sh/types.h:typedef u64 sector_t; asm-x86_64/types.h:typedef u64 sector_t; asm-h8300/types.h:typedef u64 sector_t; asm-i386/types.h:typedef u64 sector_t; asm-mips/types.h:typedef u64 sector_t; asm-s390/types.h:typedef u64 sector_t; asm-ppc/types.h:typedef u64 sector_t; all unsigned types. Have I missed a location where sector_t is defined as a signed type? The naive approach would be to simply remove the test for < 0 and the associated code in abort_negative. But since I don't know enough about file system internals to know why that test was put in there in the first place (and I've been unable to deduce it from reading the surrounding code) I need your help. - Why is that code there? - Can it simply be removed (seems like what you'd want if it will never execute)? - Can you point me at some relevant documentation I should read in order to discover the ansver for myself? I've so far been reading the affected .c files themselves and the files in Documentation/filesystems/ , but I can't seem to find anything even remotely related to sector_t's being negative... - Am I missing something obvious? I want to thank you in advance for your time - I hope I'm not wasting it. Kind regards, Jesper Juhl ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-05 23:20 Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested Jesper Juhl @ 2004-01-06 8:51 ` Hans Reiser 2004-01-06 11:28 ` Jesper Juhl 0 siblings, 1 reply; 20+ messages in thread From: Hans Reiser @ 2004-01-06 8:51 UTC (permalink / raw) To: Jesper Juhl Cc: Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita Jesper Juhl wrote: > > >The problem is what seems (to me, but I could be dead wrong) to be invalid >use of variables of type sector_t. > > > thanks. nikita, please clean. -- Hans ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 8:51 ` Hans Reiser @ 2004-01-06 11:28 ` Jesper Juhl 2004-01-06 17:46 ` Mike Fedyk 0 siblings, 1 reply; 20+ messages in thread From: Jesper Juhl @ 2004-01-06 11:28 UTC (permalink / raw) To: Hans Reiser Cc: Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita On Tue, 6 Jan 2004, Hans Reiser wrote: > Jesper Juhl wrote: > > > > > > >The problem is what seems (to me, but I could be dead wrong) to be invalid > >use of variables of type sector_t. > > > > > > > thanks. nikita, please clean. > In case the proper fix is to just get rid of the impossible branches and the unreachable code, I've invluded patches below that does this for the various filesystems. Seems to compile and behave fine (against 2.6.1-rc1-mm2).. --- linux-2.6.1-rc1-mm2-orig/fs/ufs/inode.c 2003-12-31 05:46:41.000000000 +0100 +++ linux-2.6.1-rc1-mm2/fs/ufs/inode.c 2004-01-06 12:12:29.000000000 +0100 @@ -331,8 +331,6 @@ static int ufs_getfrag_block (struct ino lock_kernel(); UFSD(("ENTER, ino %lu, fragment %u\n", inode->i_ino, fragment)) - if (fragment < 0) - goto abort_negative; if (fragment > ((UFS_NDADDR + uspi->s_apb + uspi->s_2apb + uspi->s_3apb) << uspi->s_fpbshift)) @@ -393,10 +391,6 @@ abort: unlock_kernel(); return err; -abort_negative: - ufs_warning(sb, "ufs_get_block", "block < 0"); - goto abort; - abort_too_big: ufs_warning(sb, "ufs_get_block", "block > big"); goto abort; --- linux-2.6.1-rc1-mm2-orig/fs/adfs/inode.c 2003-12-31 05:46:54.000000000 +0100 +++ linux-2.6.1-rc1-mm2/fs/adfs/inode.c 2004-01-06 12:13:08.000000000 +0100 @@ -27,9 +27,6 @@ int adfs_get_block(struct inode *inode, sector_t block, struct buffer_head *bh, int create) { - if (block < 0) - goto abort_negative; - if (!create) { if (block >= inode->i_blocks) goto abort_toobig; @@ -42,10 +39,6 @@ adfs_get_block(struct inode *inode, sect /* don't support allocation of blocks yet */ return -EIO; -abort_negative: - adfs_error(inode->i_sb, "block %d < 0", block); - return -EIO; - abort_toobig: return 0; } --- linux-2.6.1-rc1-mm2-orig/fs/befs/linuxvfs.c 2003-12-31 05:47:11.000000000 +0100 +++ linux-2.6.1-rc1-mm2/fs/befs/linuxvfs.c 2004-01-06 12:35:27.000000000 +0100 @@ -132,13 +132,6 @@ befs_get_block(struct inode *inode, sect befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld", inode->i_ino, block); - if (block < 0) { - befs_error(sb, "befs_get_block() was asked for a block " - "number less than zero: block %ld in inode %lu", - block, inode->i_ino); - return -EIO; - } - if (create) { befs_error(sb, "befs_get_block() was asked to write to " "block %ld in inode %lu", block, inode->i_ino); --- linux-2.6.1-rc1-mm2-orig/fs/bfs/file.c 2003-12-31 05:46:40.000000000 +0100 +++ linux-2.6.1-rc1-mm2/fs/bfs/file.c 2004-01-06 12:15:20.000000000 +0100 @@ -64,7 +64,7 @@ static int bfs_get_block(struct inode * struct bfs_inode_info *bi = BFS_I(inode); struct buffer_head *sbh = info->si_sbh; - if (block < 0 || block > info->si_blocks) + if (block > info->si_blocks) return -EIO; phys = bi->i_sblock + block; --- linux-2.6.1-rc1-mm2-orig/fs/reiserfs/inode.c 2004-01-06 01:33:08.000000000 +0100 +++ linux-2.6.1-rc1-mm2/fs/reiserfs/inode.c 2004-01-06 12:16:16.000000000 +0100 @@ -574,11 +574,6 @@ int reiserfs_get_block (struct inode * i th.t_trans_id = 0 ; version = get_inode_item_key_version (inode); - if (block < 0) { - reiserfs_write_unlock(inode->i_sb); - return -EIO; - } - if (!file_capable (inode, block)) { reiserfs_write_unlock(inode->i_sb); return -EFBIG; Kind regards, Jesper Juhl ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 11:28 ` Jesper Juhl @ 2004-01-06 17:46 ` Mike Fedyk 2004-01-06 21:35 ` Oleg Drokin 2004-01-06 22:43 ` Jesper Juhl 0 siblings, 2 replies; 20+ messages in thread From: Mike Fedyk @ 2004-01-06 17:46 UTC (permalink / raw) To: Jesper Juhl Cc: Hans Reiser, Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita On Tue, Jan 06, 2004 at 12:28:34PM +0100, Jesper Juhl wrote: > --- linux-2.6.1-rc1-mm2-orig/fs/reiserfs/inode.c 2004-01-06 01:33:08.000000000 +0100 > +++ linux-2.6.1-rc1-mm2/fs/reiserfs/inode.c 2004-01-06 12:16:16.000000000 +0100 > @@ -574,11 +574,6 @@ int reiserfs_get_block (struct inode * i > th.t_trans_id = 0 ; > version = get_inode_item_key_version (inode); > > - if (block < 0) { > - reiserfs_write_unlock(inode->i_sb); > - return -EIO; > - } > - Did you check the locking after this is removed? Maybe after the sector_t merges, this code covered a case that is left open now... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 17:46 ` Mike Fedyk @ 2004-01-06 21:35 ` Oleg Drokin 2004-01-06 22:25 ` Mike Fedyk 2004-01-06 23:37 ` Hans Reiser 2004-01-06 22:43 ` Jesper Juhl 1 sibling, 2 replies; 20+ messages in thread From: Oleg Drokin @ 2004-01-06 21:35 UTC (permalink / raw) To: linux-kernel, mfedyk Hello! Mike Fedyk <mfedyk@matchmail.com> wrote: MF> On Tue, Jan 06, 2004 at 12:28:34PM +0100, Jesper Juhl wrote: >> --- linux-2.6.1-rc1-mm2-orig/fs/reiserfs/inode.c 2004-01-06 01:33:08.000000000 +0100 >> +++ linux-2.6.1-rc1-mm2/fs/reiserfs/inode.c 2004-01-06 12:16:16.000000000 +0100 >> @@ -574,11 +574,6 @@ int reiserfs_get_block (struct inode * i >> th.t_trans_id = 0 ; >> version = get_inode_item_key_version (inode); >> >> - if (block < 0) { >> - reiserfs_write_unlock(inode->i_sb); >> - return -EIO; >> - } >> - MF> Did you check the locking after this is removed? Locking should be fine. As I remember, we take this lock near reiserfs_get_block() entrance, and then drop it on exit. MF> Maybe after the sector_t merges, this code covered a case that is left open MF> now... This code was never executing anyway. Bye, Oleg ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 21:35 ` Oleg Drokin @ 2004-01-06 22:25 ` Mike Fedyk 2004-01-06 23:37 ` Hans Reiser 1 sibling, 0 replies; 20+ messages in thread From: Mike Fedyk @ 2004-01-06 22:25 UTC (permalink / raw) To: Oleg Drokin; +Cc: linux-kernel On Tue, Jan 06, 2004 at 11:35:10PM +0200, Oleg Drokin wrote: > Hello! > > Mike Fedyk <mfedyk@matchmail.com> wrote: > MF> Did you check the locking after this is removed? > > Locking should be fine. > As I remember, we take this lock near reiserfs_get_block() entrance, > and then drop it on exit. > Ok, I just wondered if the sector_t merge overlooked this part, maybe it also changed the locking in this area because of that. > MF> Maybe after the sector_t merges, this code covered a case that is left open > MF> now... > > This code was never executing anyway. Right. Thanks, Mike ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 21:35 ` Oleg Drokin 2004-01-06 22:25 ` Mike Fedyk @ 2004-01-06 23:37 ` Hans Reiser 2004-01-06 23:53 ` Oleg Drokin 1 sibling, 1 reply; 20+ messages in thread From: Hans Reiser @ 2004-01-06 23:37 UTC (permalink / raw) To: Oleg Drokin; +Cc: linux-kernel, mfedyk, Jesper Juhl Oleg Drokin wrote: > > >This code was never executing anyway. > > > Oleg, I thought you ran a script for finding dead code last fall or summer? Any idea why it didn't find this and gcc did? Or did you only run it on reiser4? -- Hans ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 23:37 ` Hans Reiser @ 2004-01-06 23:53 ` Oleg Drokin 2004-01-07 9:26 ` Hans Reiser 0 siblings, 1 reply; 20+ messages in thread From: Oleg Drokin @ 2004-01-06 23:53 UTC (permalink / raw) To: Hans Reiser; +Cc: linux-kernel, mfedyk, Jesper Juhl Hello! On Wed, Jan 07, 2004 at 02:37:20AM +0300, Hans Reiser wrote: > >This code was never executing anyway. > Oleg, I thought you ran a script for finding dead code last fall or > summer? Any idea why it didn't find this and gcc did? Or did you only > run it on reiser4? Actually I found this dead code back then (with gcc as well), though it was not looked all that serious. I think I decided we may want to have that just in case sector_t will become signed oneday or something like that. (in 2.4 the block type is still signed long, for example). As for why gcc is finding this, but scripts (e.g. smatch) do not is because scripts generally know nothing about variable types, so they cannot tell this comparison was always false (and since gcc can do this for long time already, there is no point in implementing it in scripts anyway). Bye, Oleg ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 23:53 ` Oleg Drokin @ 2004-01-07 9:26 ` Hans Reiser 2004-01-07 10:01 ` Oleg Drokin 0 siblings, 1 reply; 20+ messages in thread From: Hans Reiser @ 2004-01-07 9:26 UTC (permalink / raw) To: Oleg Drokin; +Cc: linux-kernel, mfedyk, Jesper Juhl Oleg Drokin wrote: > > >As for why gcc is finding this, but scripts (e.g. smatch) do not is because >scripts generally know nothing about variable types, so they cannot tell >this comparison was always false (and since gcc can do this for long time >already, there is no point in implementing it in scripts anyway). > > can we get gcc to issue us a warning? there might be other stuff lurking around also.... -- Hans ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 9:26 ` Hans Reiser @ 2004-01-07 10:01 ` Oleg Drokin 2004-01-07 11:00 ` Hans Reiser ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Oleg Drokin @ 2004-01-07 10:01 UTC (permalink / raw) To: Hans Reiser; +Cc: linux-kernel, mfedyk, Jesper Juhl Hello! On Wed, Jan 07, 2004 at 12:26:09PM +0300, Hans Reiser wrote: > >As for why gcc is finding this, but scripts (e.g. smatch) do not is because > >scripts generally know nothing about variable types, so they cannot tell > >this comparison was always false (and since gcc can do this for long time > >already, there is no point in implementing it in scripts anyway). > can we get gcc to issue us a warning? there might be other stuff > lurking around also.... If you add -W switch to CFLAGS, you'd get A LOT of more warnings. Also just reading manpage on gcc around description of that flag will give you a list of options to individually turn on certain check types. Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by default, I think. Bye, Oleg ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 10:01 ` Oleg Drokin @ 2004-01-07 11:00 ` Hans Reiser 2004-01-07 12:08 ` Oleg Drokin 2004-01-07 17:45 ` Mike Fedyk 2004-01-13 16:26 ` Adrian Bunk 2 siblings, 1 reply; 20+ messages in thread From: Hans Reiser @ 2004-01-07 11:00 UTC (permalink / raw) To: Oleg Drokin Cc: linux-kernel, mfedyk, Jesper Juhl, Reiserfs developers mail-list, grev Oleg Drokin wrote: >Hello! > >On Wed, Jan 07, 2004 at 12:26:09PM +0300, Hans Reiser wrote: > > >>>As for why gcc is finding this, but scripts (e.g. smatch) do not is because >>>scripts generally know nothing about variable types, so they cannot tell >>>this comparison was always false (and since gcc can do this for long time >>>already, there is no point in implementing it in scripts anyway). >>> >>> >>can we get gcc to issue us a warning? there might be other stuff >>lurking around also.... >> >> > >If you add -W switch to CFLAGS, you'd get A LOT of more warnings. >Also just reading manpage on gcc around description of that flag will >give you a list of options to individually turn on certain check types. >Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by >default, I think. > >Bye, > Oleg > > > > Sigh, this means that not one member of our team bothered to compile with -W and cleanup things that were found? Sad. This is what happens when project leaders like me spend more of their time on funding proposals than code tweaking..... Elena, please do so, for both V3 and V4, and send a proposed patch to cleanup what gets complained of. -- Hans ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 11:00 ` Hans Reiser @ 2004-01-07 12:08 ` Oleg Drokin 2004-01-07 12:17 ` Jesper Juhl 0 siblings, 1 reply; 20+ messages in thread From: Oleg Drokin @ 2004-01-07 12:08 UTC (permalink / raw) To: Hans Reiser Cc: linux-kernel, mfedyk, Jesper Juhl, Reiserfs developers mail-list, grev Hello! On Wed, Jan 07, 2004 at 02:00:35PM +0300, Hans Reiser wrote: > >>can we get gcc to issue us a warning? there might be other stuff > >>lurking around also.... > >If you add -W switch to CFLAGS, you'd get A LOT of more warnings. > >Also just reading manpage on gcc around description of that flag will > >give you a list of options to individually turn on certain check types. > >Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by > >default, I think. > Sigh, this means that not one member of our team bothered to compile > with -W and cleanup things that were found? Sad. This is what happens Well, I was doing these sorts of stuff and cleaning all stuff that I thought was important enough. Both on kernel code and progs code. Also reiser4progs include -W switch by default (and -Werror as well) I think. At least that was the case back in September. Bye, Oleg ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 12:08 ` Oleg Drokin @ 2004-01-07 12:17 ` Jesper Juhl 2004-01-07 12:27 ` Oleg Drokin 0 siblings, 1 reply; 20+ messages in thread From: Jesper Juhl @ 2004-01-07 12:17 UTC (permalink / raw) To: Oleg Drokin Cc: Hans Reiser, linux-kernel, mfedyk, Reiserfs developers mail-list, grev On Wed, 7 Jan 2004, Oleg Drokin wrote: > Hello! > > On Wed, Jan 07, 2004 at 02:00:35PM +0300, Hans Reiser wrote: > > > >>can we get gcc to issue us a warning? there might be other stuff > > >>lurking around also.... > > >If you add -W switch to CFLAGS, you'd get A LOT of more warnings. > > >Also just reading manpage on gcc around description of that flag will > > >give you a list of options to individually turn on certain check types. > > >Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by > > >default, I think. > > Sigh, this means that not one member of our team bothered to compile > > with -W and cleanup things that were found? Sad. This is what happens > > Well, I was doing these sorts of stuff and cleaning all stuff that I thought > was important enough. > This is what I'm currently doing with all new -mm kernels. There's a lot of signed vs unsigned comparison all over the kernel as well as unsigned values being compared to negative values, missing initializers, and a lot of other minor stuff. I'm slowly trying to clean up what I find... I'm most likely not going to be able to clean it /all/ up, and it's a slow process for me, but I'll be posting patches whenever I think I've got something cleaned up correctly. - Jesper Juhl ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 12:17 ` Jesper Juhl @ 2004-01-07 12:27 ` Oleg Drokin 0 siblings, 0 replies; 20+ messages in thread From: Oleg Drokin @ 2004-01-07 12:27 UTC (permalink / raw) To: Jesper Juhl Cc: Hans Reiser, linux-kernel, mfedyk, Reiserfs developers mail-list, grev Hello! On Wed, Jan 07, 2004 at 01:17:09PM +0100, Jesper Juhl wrote: > > > >>can we get gcc to issue us a warning? there might be other stuff > > > >>lurking around also.... > > > >If you add -W switch to CFLAGS, you'd get A LOT of more warnings. > > > >Also just reading manpage on gcc around description of that flag will > > > >give you a list of options to individually turn on certain check types. > > > >Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by > > > >default, I think. > > > Sigh, this means that not one member of our team bothered to compile > > > with -W and cleanup things that were found? Sad. This is what happens > > Well, I was doing these sorts of stuff and cleaning all stuff that I thought > > was important enough. > This is what I'm currently doing with all new -mm kernels. There's a lot > of signed vs unsigned comparison all over the kernel as well as unsigned > values being compared to negative values, missing initializers, and a lot > of other minor stuff. Well, some of this "minor" stuff was hiding major bugs, as I remember. > I'm slowly trying to clean up what I find... Great! Bye, Oleg ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 10:01 ` Oleg Drokin 2004-01-07 11:00 ` Hans Reiser @ 2004-01-07 17:45 ` Mike Fedyk 2004-01-13 16:26 ` Adrian Bunk 2 siblings, 0 replies; 20+ messages in thread From: Mike Fedyk @ 2004-01-07 17:45 UTC (permalink / raw) To: Oleg Drokin; +Cc: Hans Reiser, linux-kernel, Jesper Juhl On Wed, Jan 07, 2004 at 12:01:13PM +0200, Oleg Drokin wrote: > Hello! > > On Wed, Jan 07, 2004 at 12:26:09PM +0300, Hans Reiser wrote: > > >As for why gcc is finding this, but scripts (e.g. smatch) do not is because > > >scripts generally know nothing about variable types, so they cannot tell > > >this comparison was always false (and since gcc can do this for long time > > >already, there is no point in implementing it in scripts anyway). > > can we get gcc to issue us a warning? there might be other stuff > > lurking around also.... > > If you add -W switch to CFLAGS, you'd get A LOT of more warnings. > Also just reading manpage on gcc around description of that flag will > give you a list of options to individually turn on certain check types. > Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by > default, I think. That was suse using a version of gcc pulled from cvs, not a released version of 3.3.x IIRC. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-07 10:01 ` Oleg Drokin 2004-01-07 11:00 ` Hans Reiser 2004-01-07 17:45 ` Mike Fedyk @ 2004-01-13 16:26 ` Adrian Bunk 2 siblings, 0 replies; 20+ messages in thread From: Adrian Bunk @ 2004-01-13 16:26 UTC (permalink / raw) To: Oleg Drokin; +Cc: Hans Reiser, linux-kernel, mfedyk, Jesper Juhl On Wed, Jan 07, 2004 at 12:01:13PM +0200, Oleg Drokin wrote: > Hello! > > On Wed, Jan 07, 2004 at 12:26:09PM +0300, Hans Reiser wrote: > > >As for why gcc is finding this, but scripts (e.g. smatch) do not is because > > >scripts generally know nothing about variable types, so they cannot tell > > >this comparison was always false (and since gcc can do this for long time > > >already, there is no point in implementing it in scripts anyway). > > can we get gcc to issue us a warning? there might be other stuff > > lurking around also.... > > If you add -W switch to CFLAGS, you'd get A LOT of more warnings. > Also just reading manpage on gcc around description of that flag will > give you a list of options to individually turn on certain check types. > Also gcc 3.3 have this sort of " unsigned < 0 | unsigned > 0" checks on by > default, I think. IIRC this was only in prerelease versions of gcc 3.3 (including one SuSE ships). For released version of gcc you need -Wsign-compare. > Bye, > Oleg cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 17:46 ` Mike Fedyk 2004-01-06 21:35 ` Oleg Drokin @ 2004-01-06 22:43 ` Jesper Juhl 2004-01-06 22:58 ` Mike Fedyk 2004-01-06 23:26 ` Hans Reiser 1 sibling, 2 replies; 20+ messages in thread From: Jesper Juhl @ 2004-01-06 22:43 UTC (permalink / raw) To: Mike Fedyk Cc: Hans Reiser, Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita On Tue, 6 Jan 2004, Mike Fedyk wrote: > On Tue, Jan 06, 2004 at 12:28:34PM +0100, Jesper Juhl wrote: > > --- linux-2.6.1-rc1-mm2-orig/fs/reiserfs/inode.c 2004-01-06 01:33:08.000000000 +0100 > > +++ linux-2.6.1-rc1-mm2/fs/reiserfs/inode.c 2004-01-06 12:16:16.000000000 +0100 > > @@ -574,11 +574,6 @@ int reiserfs_get_block (struct inode * i > > th.t_trans_id = 0 ; > > version = get_inode_item_key_version (inode); > > > > - if (block < 0) { > > - reiserfs_write_unlock(inode->i_sb); > > - return -EIO; > > - } > > - > > Did you check the locking after this is removed? > reiserfs_write_unlock(inode->i_sb); is called at the beginning of the function, and as far as I can tell it's matched by a call to reiserfs_write_unlock(inode->i_sb); at every potential return point in the function, and I see no other locks being taken. Besides, since if (block < 0) will never be true and reiserfs_write_unlock(inode->i_sb); return -EIO; will never execute in any case, locking should behave identical to what it did before removing the code. Locking /seems/ OK to me in this function. Also, I did a build of fs/reiserfs/ both with and without the above patch, and then did a disassemble of inode.o (objdump -d) and compared the generated code for reiserfs_get_block , and the generated code is byte-for-byte identical in both cases, which means that gcc realizes that the if() statement will never execute and optimizes it away in any case. This is a further indication that removing the code should be safe. And it seems to me that any code that assumes that a sector_t value can be negative is either broken or obsolete. > Maybe after the sector_t merges, this code covered a case that is left open > now... > I'm not familliar with those "sector_t merges" you are refering to, but I found some mention of a 64bit sector_t merge in the 2.5.x kernel Changelogs, so I downloaded the 2.5.10 kernel source (first reference to sector_t I found was in the 2.5.11 changelog) and took a look at how sector_t used to be defined. It seems that it was an unsigned value even back then. Has sector_t ever been signed? /Jesper Juhl ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 22:43 ` Jesper Juhl @ 2004-01-06 22:58 ` Mike Fedyk 2004-01-06 23:26 ` Hans Reiser 1 sibling, 0 replies; 20+ messages in thread From: Mike Fedyk @ 2004-01-06 22:58 UTC (permalink / raw) To: Jesper Juhl Cc: Hans Reiser, Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita On Tue, Jan 06, 2004 at 11:43:40PM +0100, Jesper Juhl wrote: > reiserfs_write_unlock(inode->i_sb); is called at the beginning of the > function, and as far as I can tell it's matched by a call to > reiserfs_write_unlock(inode->i_sb); at every potential return point in the > function, and I see no other locks being taken. OK, good. > Besides, since if (block < 0) will never be true and > reiserfs_write_unlock(inode->i_sb); > return -EIO; > will never execute in any case, locking should behave identical to what it > did before removing the code. > Locking /seems/ OK to me in this function. > > Also, I did a build of fs/reiserfs/ both with and without the above patch, > and then did a disassemble of inode.o (objdump -d) and compared the > generated code for reiserfs_get_block , and the generated code is > byte-for-byte identical in both cases, which means that gcc realizes that > the if() statement will never execute and optimizes it away in any case. I'm not talking about before and afte your patch, I'm talking about before and after the sector_t patch (presumably for the large block device (gt 2TB) support). > I'm not familliar with those "sector_t merges" you are refering to, but I > found some mention of a 64bit sector_t merge in the 2.5.x kernel > Changelogs, so I downloaded the 2.5.10 kernel source (first reference to > sector_t I found was in the 2.5.11 changelog) and took a look at how > sector_t used to be defined. It seems that it was an unsigned value even > back then. > Has sector_t ever been signed? Really? Interesting. Then maybe this is from ported 2.2 code? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 22:43 ` Jesper Juhl 2004-01-06 22:58 ` Mike Fedyk @ 2004-01-06 23:26 ` Hans Reiser 2004-01-07 0:46 ` Jesper Juhl 1 sibling, 1 reply; 20+ messages in thread From: Hans Reiser @ 2004-01-06 23:26 UTC (permalink / raw) To: Jesper Juhl Cc: Mike Fedyk, Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita Jesper Juhl wrote: > > >Also, I did a build of fs/reiserfs/ both with and without the above patch, >and then did a disassemble of inode.o (objdump -d) and compared the >generated code for reiserfs_get_block , and the generated code is >byte-for-byte identical in both cases, which means that gcc realizes that >the if() statement will never execute and optimizes it away in any case. > > I think you have done too much work.;-) Thanks though. The only reason we are slow in processing your patch and forwarding it to Linus is that the Russian Christmas is today.... -- Hans ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested 2004-01-06 23:26 ` Hans Reiser @ 2004-01-07 0:46 ` Jesper Juhl 0 siblings, 0 replies; 20+ messages in thread From: Jesper Juhl @ 2004-01-07 0:46 UTC (permalink / raw) To: Hans Reiser Cc: Mike Fedyk, Tigran A. Aivazian, Hans Reiser, Daniel Pirkl, Russell King, Will Dyson, linux-kernel, nikita On Wed, 7 Jan 2004, Hans Reiser wrote: > Jesper Juhl wrote: > > > > >Also, I did a build of fs/reiserfs/ both with and without the above patch, > >and then did a disassemble of inode.o (objdump -d) and compared the > >generated code for reiserfs_get_block , and the generated code is > >byte-for-byte identical in both cases, which means that gcc realizes that > >the if() statement will never execute and optimizes it away in any case. > > > > > I think you have done too much work.;-) > > Thanks though. > You are very welcome. I'm having great fun reading the code, looking for potential problems, testing stuff etc.. I'm enjoying myself (and learning along the way, which is good :). > The only reason we are slow in processing your patch and forwarding it > to Linus is that the Russian Christmas is today.... > Enjoy :-) /Jesper Juhl ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2004-01-13 16:26 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2004-01-05 23:20 Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested Jesper Juhl 2004-01-06 8:51 ` Hans Reiser 2004-01-06 11:28 ` Jesper Juhl 2004-01-06 17:46 ` Mike Fedyk 2004-01-06 21:35 ` Oleg Drokin 2004-01-06 22:25 ` Mike Fedyk 2004-01-06 23:37 ` Hans Reiser 2004-01-06 23:53 ` Oleg Drokin 2004-01-07 9:26 ` Hans Reiser 2004-01-07 10:01 ` Oleg Drokin 2004-01-07 11:00 ` Hans Reiser 2004-01-07 12:08 ` Oleg Drokin 2004-01-07 12:17 ` Jesper Juhl 2004-01-07 12:27 ` Oleg Drokin 2004-01-07 17:45 ` Mike Fedyk 2004-01-13 16:26 ` Adrian Bunk 2004-01-06 22:43 ` Jesper Juhl 2004-01-06 22:58 ` Mike Fedyk 2004-01-06 23:26 ` Hans Reiser 2004-01-07 0:46 ` Jesper Juhl
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.