| Message ID | 1429870070-30310-1-git-send-email-firogm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > > > >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)