[PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Jesper Juhl]

'arg' is unsigned so it can never be less than zero, so testing for that 
is pointless and also generates a warning when building with gcc -W. This 
patch eliminates the pointless check.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

--- linux-2.6.12-rc2-mm3-orig/fs/fcntl.c	2005-04-11 21:20:50.000000000 +0200
+++ linux-2.6.12-rc2-mm3/fs/fcntl.c	2005-04-15 03:03:00.000000000 +0200
@@ -308,7 +308,7 @@ static long do_fcntl(int fd, unsigned in
 		break;
 	case F_SETSIG:
 		/* arg == 0 restores default behaviour. */
-		if (arg < 0 || arg > _NSIG) {
+		if (arg > _NSIG) {
 			break;
 		}
 		err = 0;

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Matthew Wilcox]

On Fri, Apr 15, 2005 at 03:07:42AM +0200, Jesper Juhl wrote:
> 'arg' is unsigned so it can never be less than zero, so testing for that 
> is pointless and also generates a warning when building with gcc -W. This 
> patch eliminates the pointless check.

Didn't Linus already reject this one 6 months ago?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Christoph Hellwig]

On Fri, Apr 15, 2005 at 02:31:00AM +0100, Matthew Wilcox wrote:
> On Fri, Apr 15, 2005 at 03:07:42AM +0200, Jesper Juhl wrote:
> > 'arg' is unsigned so it can never be less than zero, so testing for that 
> > is pointless and also generates a warning when building with gcc -W. This 
> > patch eliminates the pointless check.
> 
> Didn't Linus already reject this one 6 months ago?

I think Linux only complained if we're using some typedef that actually
may be signed.  For fcntl that 'arg' argument is unsigned and that's hardcoded
in the ABI.  So the check doesn't make sense at all.

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Matthew Wilcox]

On Fri, Apr 15, 2005 at 09:21:50AM +0100, Christoph Hellwig wrote:
> On Fri, Apr 15, 2005 at 02:31:00AM +0100, Matthew Wilcox wrote:
> > On Fri, Apr 15, 2005 at 03:07:42AM +0200, Jesper Juhl wrote:
> > > 'arg' is unsigned so it can never be less than zero, so testing for that 
> > > is pointless and also generates a warning when building with gcc -W. This 
> > > patch eliminates the pointless check.
> > 
> > Didn't Linus already reject this one 6 months ago?
> 
> I think Linux only complained if we're using some typedef that actually
> may be signed.  For fcntl that 'arg' argument is unsigned and that's hardcoded
> in the ABI.  So the check doesn't make sense at all.

No, it was exactly this patch:
http://www.ussg.iu.edu/hypermail/linux/kernel/0401.0/1816.html

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Christoph Hellwig]

On Fri, Apr 15, 2005 at 12:29:08PM +0100, Matthew Wilcox wrote:
> > I think Linux only complained if we're using some typedef that actually
> > may be signed.  For fcntl that 'arg' argument is unsigned and that's hardcoded
> > in the ABI.  So the check doesn't make sense at all.
> 
> No, it was exactly this patch:
> http://www.ussg.iu.edu/hypermail/linux/kernel/0401.0/1816.html

Hmm.  Looks I absolutely disagree with Linus on this one ;-)

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Herbert Xu]

Christoph Hellwig <hch@infradead.org> wrote:
>
>> No, it was exactly this patch:
>> http://www.ussg.iu.edu/hypermail/linux/kernel/0401.0/1816.html
> 
> Hmm.  Looks I absolutely disagree with Linus on this one ;-)

Me too.  The compiler doesn't really have much choice here.  If
it ignores all comparisons of unsigned integers to less than zero
then we could miss real bugs like this:

int foo(unsigned int val)
{
	return val < 0;
}

where the user probably wanted a signed comparison.

I suppose it could be smart and stay quiet about

val < 0 || val > BOUND

However, gcc is slow enough as it is without adding unnecessary
smarts like this.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Matthew Wilcox]

On Fri, Apr 15, 2005 at 10:03:05PM +1000, Herbert Xu wrote:
> I suppose it could be smart and stay quiet about
> 
> val < 0 || val > BOUND
> 
> However, gcc is slow enough as it is without adding unnecessary
> smarts like this.

It only warns with -W on, not with -Wall, so I see no compelling
reason to fix this.  I think the real problem here is that 'arg'
is declared 2 pages earlier in the function prototype (aka the
function-growth-hormone-imbalance syndrome).

There's two good ways of fixing this, adding a f_setsig() function:

static inline int f_setsig(struct file *filp, unsigned long arg)
{
	if (arg > _NSIG)
		return -EINVAL;

	filp->f_owner.signum = arg;
	return 0;
}
...
	case F_SETSIG:
		err = f_setsig(filp, arg);
		break;

or add a function that checks a variable to see if it's a valid signal number:

#define valid_signal(arg)	((unsigned long)arg <= _NSIG)
...
	case F_SETSIG:
		if (!valid_signal(arg))
			break;
		err = 0;
		filp->f_owner.signum = arg;
		break;

Looks like futex.c, ptrace.c, signal.c, sys.c and almost every
architecture's ptrace code could easily make use of the latter, but not
the former.  It also looks like we have a few off-by-one errors.  For
example, in h8300's ptrace code:

                case PTRACE_SYSCALL:
                case PTRACE_CONT: {
                        ret = -EIO;
                        if ((unsigned long) data >= _NSIG)
                                break ;

but

                case PTRACE_SINGLESTEP: {
                        ret = -EIO;
                        if ((unsigned long) data > _NSIG)
                                break;

so I'd recommend the second solution.  But be careful not to "fix up"
cases like:

./kernel/exit.c:        if (sig < 1 || sig > _NSIG)

where we really don't want to allow zero.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Jesper Juhl]

On Fri, 15 Apr 2005, Matthew Wilcox wrote:

> On Fri, Apr 15, 2005 at 10:03:05PM +1000, Herbert Xu wrote:
> > I suppose it could be smart and stay quiet about
> > 
> > val < 0 || val > BOUND
> > 
> > However, gcc is slow enough as it is without adding unnecessary
> > smarts like this.
> 
> It only warns with -W on, not with -Wall, so I see no compelling
> reason to fix this.

Fixing the -W warning was not the main point. The main point was simply 
that the check makes no sense at all.

>  I think the real problem here is that 'arg'
> is declared 2 pages earlier in the function prototype (aka the
> function-growth-hormone-imbalance syndrome).
> 
> There's two good ways of fixing this, adding a f_setsig() function:
> 
[...]
> or add a function that checks a variable to see if it's a valid signal number:
[...]
> 
> Looks like futex.c, ptrace.c, signal.c, sys.c and almost every
> architecture's ptrace code could easily make use of the latter, but not
> the former.  It also looks like we have a few off-by-one errors.  For
[...]
> so I'd recommend the second solution.

Thank you for your feedback, that makes a lot of sense. That should get 
rid of the pointless tests, get rid of the -W warning and get rid of the 
off-by-one errors - sounds good to me.
I'll create patches and send them along shortly.


>  But be careful not to "fix up"
> cases like:
> 
> ./kernel/exit.c:        if (sig < 1 || sig > _NSIG)
> 
> where we really don't want to allow zero.
> 
I'll watch out for those.  
I can either leave them alone or re-write as one of
    if (valid_signal(sig) && sig != 0)
    if (valid_signal(sig) && sig > 0)
any preference?
Personally I would probably go with the 
'rewrite as  if (valid_signal(sig) && sig > 0)' one to encourage use of 
the new valid_signal() function and make usage consistent.


-- 
Jesper Juhl

Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero

[Jesper Juhl]

On Fri, 15 Apr 2005, Matthew Wilcox wrote:

> On Fri, Apr 15, 2005 at 03:07:42AM +0200, Jesper Juhl wrote:
> > 'arg' is unsigned so it can never be less than zero, so testing for that 
> > is pointless and also generates a warning when building with gcc -W. This 
> > patch eliminates the pointless check.
> 
> Didn't Linus already reject this one 6 months ago?
> 
Hmmm, perhaps you are right. There was some discussion about similar 
patches a while back. That had slipped my mind.  

-- 
Jesper