[PATCH] fs/compat: remove redundant 'less than zero' check

[PATCH] fs/compat: remove redundant 'less than zero' check

[Firo Yang]

Smatch complains about the check in compat.c.
fs/compat.c:565 compat_rw_copy_check_uvector() warn:
unsigned 'nr_segs' is never less than zero.

I think, there is no reason to check if the value nr_segs
is less than zero. So I removed it.

Signed-off-by: Firo Yang <firogm@gmail.com>
---
 fs/compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/compat.c b/fs/compat.c
index 6fd272d..beaf15b 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int type,
 		goto out;
 
 	ret = -EINVAL;
-	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
+	if (nr_segs > UIO_MAXIOV)
 		goto out;
 	if (nr_segs > fast_segs) {
 		ret = -ENOMEM;
-- 
2.1.0

Re: [PATCH] fs/compat: remove redundant 'less than zero' check

[Julia Lawall]

On Fri, 24 Apr 2015, Firo Yang wrote:

> Smatch complains about the check in compat.c.
> fs/compat.c:565 compat_rw_copy_check_uvector() warn:
> unsigned 'nr_segs' is never less than zero.
>
> I think, there is no reason to check if the value nr_segs
> is less than zero. So I removed it.

It would be good to explain why you think this.  What other statements in
the code imply this property?

julia

>
> Signed-off-by: Firo Yang <firogm@gmail.com>
> ---
>  fs/compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/compat.c b/fs/compat.c
> index 6fd272d..beaf15b 100644
> --- a/fs/compat.c
> +++ b/fs/compat.c
> @@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int type,
>  		goto out;
>
>  	ret = -EINVAL;
> -	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
> +	if (nr_segs > UIO_MAXIOV)
>  		goto out;
>  	if (nr_segs > fast_segs) {
>  		ret = -ENOMEM;
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fs/compat: remove redundant 'less than zero' check

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2297 bytes --]



On Fri, 24 Apr 2015, Firo Yang wrote:

> It was because that1. nr_segs stand for the number of 'segs' should be large
> or equal to 0, a positive number.

This is not very convincing.  A negative number could be used to encode a
failure of some previous operation, although normally that failure should
have been checked for elsewhere.

> 2. the type of nr_segs is 'unsigned long', It imply a positive number.

OK, this seems like a reasonable explanation.  Currently the test can
never be true, so there is no point to have it.  It would be good to note
this in the commit message of your patch.

julia

> 3. the code if (nr_segs > UIO_MAXIOV) is enough to keep the value nr_segs
> safe.
>
> Regards
> Firo
>
>
>
> On Fri, Apr 24, 2015 at 6:10 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
>       On Fri, 24 Apr 2015, Firo Yang wrote:
>
>       > Smatch complains about the check in compat.c.
>       > fs/compat.c:565 compat_rw_copy_check_uvector() warn:
>       > unsigned 'nr_segs' is never less than zero.
>       >
>       > I think, there is no reason to check if the value nr_segs
>       > is less than zero. So I removed it.
>
>       It would be good to explain why you think this.  What other
>       statements in
>       the code imply this property?
>
>       julia
>
>       >
>       > Signed-off-by: Firo Yang <firogm@gmail.com>
>       > ---
>       >  fs/compat.c | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/fs/compat.c b/fs/compat.c
>       > index 6fd272d..beaf15b 100644
>       > --- a/fs/compat.c
>       > +++ b/fs/compat.c
>       > @@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int
>       type,
>       >               goto out;
>       >
>       >       ret = -EINVAL;
>       > -     if (nr_segs > UIO_MAXIOV || nr_segs < 0)
>       > +     if (nr_segs > UIO_MAXIOV)
>       >               goto out;
>       >       if (nr_segs > fast_segs) {
>       >               ret = -ENOMEM;
>       > --
>       > 2.1.0
>       >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
>
>
>
>

Re: [PATCH] fs/compat: remove redundant 'less than zero' check

[Dan Carpenter]

On Fri, Apr 24, 2015 at 06:07:50PM +0800, Firo Yang wrote:

> -	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
> +	if (nr_segs > UIO_MAXIOV)

Linus said at kernel summit that he thinks this kind of checks are ok.

Smatch already ignores checks like:

	if (nr_segs < 0 || nr_segs > UIO_MAXIOV)

So making it ignore this one as well shouldn't be hard...  I'll change
Smatch to ignore this one as well.

regards,
dan carpenter

Re: [PATCH] fs/compat: remove redundant 'less than zero' check

[Julia Lawall]

On Fri, 24 Apr 2015, Dan Carpenter wrote:

> On Fri, Apr 24, 2015 at 06:07:50PM +0800, Firo Yang wrote:
>
> > -	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
> > +	if (nr_segs > UIO_MAXIOV)
>
> Linus said at kernel summit that he thinks this kind of checks are ok.
>
> Smatch already ignores checks like:
>
> 	if (nr_segs < 0 || nr_segs > UIO_MAXIOV)
>
> So making it ignore this one as well shouldn't be hard...  I'll change
> Smatch to ignore this one as well.

It is unsigned.  What is the point of cluttering the code?

julia

>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fs/compat: remove redundant 'less than zero' check

[Dan Carpenter]

On Fri, Apr 24, 2015 at 02:23:23PM +0200, Julia Lawall wrote:
> On Fri, 24 Apr 2015, Dan Carpenter wrote:
> 
> > On Fri, Apr 24, 2015 at 06:07:50PM +0800, Firo Yang wrote:
> >
> > > -	if (nr_segs > UIO_MAXIOV || nr_segs < 0)
> > > +	if (nr_segs > UIO_MAXIOV)
> >
> > Linus said at kernel summit that he thinks this kind of checks are ok.
> >
> > Smatch already ignores checks like:
> >
> > 	if (nr_segs < 0 || nr_segs > UIO_MAXIOV)
> >
> > So making it ignore this one as well shouldn't be hard...  I'll change
> > Smatch to ignore this one as well.
> 
> It is unsigned.  What is the point of cluttering the code?

I guess I don't feel strongly either way.  It was Linus who said
something like "the intent is clear from the code" so he isn't a fan of
these particular static checker fixes.

These are easy enough for me to filter out so they needn't cause a false
positive.

They're more of a style issue than anything else.  With coccinelle, it's
easier to care about style issues because you can fix them automatically.
In Smatch caring about style issues is a time suck.

regards,
dan carpenter