Suspected bug in fs/ufs/inode.c - test for < 0 on unsigned sector_t - [2.6.1-rc1-mm1]

Suspected bug in fs/ufs/inode.c - test for < 0 on unsigned sector_t - [2.6.1-rc1-mm1]

[Jesper Juhl]

Hi,

I think I may have found a bug in fs/ufs/inode.c

lines 334 & 335 have this code (in function ufs_getfrag_block) :

	if (fragment < 0)
		goto abort_negative;

fragment is an argument to ufs_getfrag_block of type sector_t
digging a little in include/linux and include/asm reveals sector_t to be
an an unsigned type in all archs (at least as far as I've been able to
determine).

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.

So, since sector_t is unsigned it can never be less than zero, hence the
branch on line 334 will never be taken and the assiciated label
"abort_negative" will never be reached (and it's not referenced anywhere
else).

I don't know enough about UFS to be able to tell if this is actually a
problem or not, but I suspect that it might be.. If it's not a problem,
then the code that tests for (fragment < 0) and the associated code
in abort_negative might as well be removed.

Is this analysis correct?  If it is, can the code simply be removed?


Kind regards,

Jesper Juhl

Re: Suspected bug in fs/ufs/inode.c and other filesystems - test for < 0 on unsigned sector_t - [2.6.1-rc1-mm1]

[Jesper Juhl]

Hmm, the exact same thing as below seems to be going on in

fs/adfs/inode.c		- line 30
fs/befs/linuxvfs.c	- line 135
fs/bfs/file.c		- line 67 (this one's a little different, but the sector_t < 0 check is involved
fs/reiserfs/inode.c	- line 577

should I be worried?  Is this worth digging into any further?
I don't mind doing some intensive fs code studying, but before I commit a
lot of time to this it would be nice with a confirmation from someone who
already knows this stuff that it's worth spending time on... I don't yet
know enough to judge that..


/Jesper Juhl


On Sat, 3 Jan 2004, Jesper Juhl wrote:

>
> Hi,
>
> I think I may have found a bug in fs/ufs/inode.c
>
> lines 334 & 335 have this code (in function ufs_getfrag_block) :
>
> 	if (fragment < 0)
> 		goto abort_negative;
>
> fragment is an argument to ufs_getfrag_block of type sector_t
> digging a little in include/linux and include/asm reveals sector_t to be
> an an unsigned type in all archs (at least as far as I've been able to
> determine).
>
> 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.
>
> So, since sector_t is unsigned it can never be less than zero, hence the
> branch on line 334 will never be taken and the assiciated label
> "abort_negative" will never be reached (and it's not referenced anywhere
> else).
>
> I don't know enough about UFS to be able to tell if this is actually a
> problem or not, but I suspect that it might be.. If it's not a problem,
> then the code that tests for (fragment < 0) and the associated code
> in abort_negative might as well be removed.
>
> Is this analysis correct?  If it is, can the code simply be removed?
>
>
> Kind regards,
>
> Jesper Juhl

Re: Suspected bug in fs/ufs/inode.c - test for < 0 on unsigned sector_t - [2.6.1-rc1-mm1]

[Tim Cambrant]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Sat, Jan 03, 2004 at 11:45:56PM +0100, Jesper Juhl wrote:
> 
> Is this analysis correct?  If it is, can the code simply be removed?

It does seem odd, but indeed, a confirmation from someone with authority
would be nice before digging in to a cleanup-process like this. Try e-mailing
the maintainer of the code directly, since it doesn't seem like anyone
feels like wasting any time on this :)

Nice job on the discovery though, if this is true, these things really should
be removed.

-- 
Tim Cambrant <tim@cambrant.com> 
GPG KeyID 0x59518702
Fingerprint: 14FE 03AE C2D1 072A 87D0  BC4D FA9E 02D8 5951 8702

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Suspected bug in fs/ufs/inode.c - test for < 0 on unsigned se ctor_t - [2.6.1-rc1-mm1]

[Jesper Juhl]

On Sun, 4 Jan 2004, Tim Cambrant wrote:

> On Sat, Jan 03, 2004 at 11:45:56PM +0100, Jesper Juhl wrote:
> >
> > Is this analysis correct?  If it is, can the code simply be removed?
>
> It does seem odd, but indeed, a confirmation from someone with authority
> would be nice before digging in to a cleanup-process like this. Try
> e-mailing
> the maintainer of the code directly, since it doesn't seem like anyone
> feels like wasting any time on this :)
>
I'm glad I'm not the only one who thinks this is odd. I'll try
emailing the maintainers of the filesystems directly and see what response
I get.


> Nice job on the discovery though,

Thank you.


> if this is true, these things really
> should
> be removed.
>
I just want to find out what is actually going on and why that code was
put there in the first place before I start changing/removing anything.
Currently I'm digging on my own but haven't come up with much yet - except
that the same code seems to be used in some other filesystems as well (as
I note in a reply to my initial mail)...
I'll keep digging.


/Jesper Juhl