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
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
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
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...
Hello!
Mike Fedyk <[email protected]> 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
On Tue, Jan 06, 2004 at 11:35:10PM +0200, Oleg Drokin wrote:
> Hello!
>
> Mike Fedyk <[email protected]> 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
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
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?
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
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
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
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
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
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
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
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
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
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
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.
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