* [PATCH] serial: 8250_omap: fix a timeout loop condition @ 2021-04-29 7:19 Dan Carpenter 2021-04-29 9:23 ` Alexander Sverdlin 2021-04-29 11:08 ` Andy Shevchenko 0 siblings, 2 replies; 13+ messages in thread From: Dan Carpenter @ 2021-04-29 7:19 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, Andy Shevchenko, linux-serial, kernel-janitors This loop ends on -1 so the error message will never be printed. Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/tty/serial/8250/8250_omap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 8ac11eaeca51..c06631ced414 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -813,7 +813,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p) poll_count--) cpu_relax(); - if (!poll_count) + if (poll_count == -1) dev_err(p->port.dev, "teardown incomplete\n"); } } -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-29 7:19 [PATCH] serial: 8250_omap: fix a timeout loop condition Dan Carpenter @ 2021-04-29 9:23 ` Alexander Sverdlin 2021-04-29 11:08 ` Andy Shevchenko 1 sibling, 0 replies; 13+ messages in thread From: Alexander Sverdlin @ 2021-04-29 9:23 UTC (permalink / raw) To: Dan Carpenter, Greg Kroah-Hartman Cc: Jiri Slaby, Vignesh Raghavendra, Johan Hovold, Andy Shevchenko, linux-serial, kernel-janitors Hello Dan! On 29/04/2021 09:19, Dan Carpenter wrote: > This loop ends on -1 so the error message will never be printed. > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") Looks good to me! Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/tty/serial/8250/8250_omap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index 8ac11eaeca51..c06631ced414 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -813,7 +813,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p) > poll_count--) > cpu_relax(); > > - if (!poll_count) > + if (poll_count == -1) > dev_err(p->port.dev, "teardown incomplete\n"); > } > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-29 7:19 [PATCH] serial: 8250_omap: fix a timeout loop condition Dan Carpenter 2021-04-29 9:23 ` Alexander Sverdlin @ 2021-04-29 11:08 ` Andy Shevchenko 2021-04-29 13:02 ` Dan Carpenter 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2021-04-29 11:08 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > This loop ends on -1 so the error message will never be printed. > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> ... > poll_count--) > cpu_relax(); > > - if (!poll_count) > + if (poll_count == -1) Why not to change poll_count-- to --poll_count? I would even prefer to replace entire loop with read_poll_timeout_atomic(). But do we even need atomic here? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-29 11:08 ` Andy Shevchenko @ 2021-04-29 13:02 ` Dan Carpenter 2021-04-30 8:46 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-04-29 13:02 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > This loop ends on -1 so the error message will never be printed. > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > ... > > > poll_count--) > > cpu_relax(); > > > > - if (!poll_count) > > + if (poll_count == -1) > > Why not to change poll_count-- to --poll_count? > Either one is fine. I considered several different ways and wrote the patch twice. The downside of --poll_count is that it's an off by one in that the author clearly intended to loop 25 times. It doesn't really matter if we only loop 24 but off by ones are aesthetically unpleasant. regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-29 13:02 ` Dan Carpenter @ 2021-04-30 8:46 ` Andy Shevchenko 2021-04-30 8:47 ` Andy Shevchenko 2021-04-30 11:41 ` Dan Carpenter 0 siblings, 2 replies; 13+ messages in thread From: Andy Shevchenko @ 2021-04-30 8:46 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote: > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > > This loop ends on -1 so the error message will never be printed. > > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > ... > > > > > poll_count--) > > > cpu_relax(); > > > > > > - if (!poll_count) > > > + if (poll_count == -1) > > > > Why not to change poll_count-- to --poll_count? > > > > Either one is fine. I considered several different ways and wrote the > patch twice. The downside of --poll_count is that it's an off by one > in that the author clearly intended to loop 25 times. It doesn't really > matter if we only loop 24 but off by ones are aesthetically unpleasant. I didn't get. If you use --poll_count you get exactly 25 times and moreover, you may convert variable to unsigned type. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 8:46 ` Andy Shevchenko @ 2021-04-30 8:47 ` Andy Shevchenko 2021-04-30 11:41 ` Dan Carpenter 1 sibling, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2021-04-30 8:47 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote: > > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > > > This loop ends on -1 so the error message will never be printed. > > > > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > ... > > > > > > > poll_count--) > > > > cpu_relax(); > > > > > > > > - if (!poll_count) > > > > + if (poll_count == -1) > > > > > > Why not to change poll_count-- to --poll_count? > > > > > > > Either one is fine. I considered several different ways and wrote the > > patch twice. The downside of --poll_count is that it's an off by one > > in that the author clearly intended to loop 25 times. It doesn't really > > matter if we only loop 24 but off by ones are aesthetically unpleasant. > > I didn't get. If you use --poll_count you get exactly 25 times and moreover, > you may convert variable to unsigned type. It seems I missed in the first message to mention that it is in association with using do {} while (); model, which is more clear for wait loops. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 8:46 ` Andy Shevchenko 2021-04-30 8:47 ` Andy Shevchenko @ 2021-04-30 11:41 ` Dan Carpenter 2021-04-30 12:53 ` Andy Shevchenko 1 sibling, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-04-30 11:41 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote: > > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > > > This loop ends on -1 so the error message will never be printed. > > > > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > ... > > > > > > > poll_count--) > > > > cpu_relax(); > > > > > > > > - if (!poll_count) > > > > + if (poll_count == -1) > > > > > > Why not to change poll_count-- to --poll_count? > > > > > > > Either one is fine. I considered several different ways and wrote the > > patch twice. The downside of --poll_count is that it's an off by one > > in that the author clearly intended to loop 25 times. It doesn't really > > matter if we only loop 24 but off by ones are aesthetically unpleasant. > > I didn't get. If you use --poll_count you get exactly 25 times and moreover, > you may convert variable to unsigned type. > Here is a small test to show that it loops 24 times. #include <stdio.h> int main(void) { int i = 25; while (--i) printf("%d\n", i); return 0; } gcc test.c ./a.out | tac Why would I make it unsigned? As a static analysis developer, pointlessly unsigned variables are one of the leading causes for the bugs I see. There are times where a iterator counter needs to be unsigned long, or u64 but I have never seen a case where changing an iterator from "int i;" to "unsigned int i;" solves a real life kernel bug. It only introduces bugs. regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 11:41 ` Dan Carpenter @ 2021-04-30 12:53 ` Andy Shevchenko 2021-04-30 13:33 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2021-04-30 12:53 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote: > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > > On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote: > > > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > > > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > > > > This loop ends on -1 so the error message will never be printed. > > > > > > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > ... > > > > > > > > > poll_count--) > > > > > cpu_relax(); > > > > > > > > > > - if (!poll_count) > > > > > + if (poll_count == -1) > > > > > > > > Why not to change poll_count-- to --poll_count? > > > > > > > > > > Either one is fine. I considered several different ways and wrote the > > > patch twice. The downside of --poll_count is that it's an off by one > > > in that the author clearly intended to loop 25 times. It doesn't really > > > matter if we only loop 24 but off by ones are aesthetically unpleasant. > > > > I didn't get. If you use --poll_count you get exactly 25 times and moreover, > > you may convert variable to unsigned type. > > > > Here is a small test to show that it loops 24 times. > > #include <stdio.h> > > int main(void) > { > int i = 25; > > while (--i) > printf("%d\n", i); > > return 0; > } > > gcc test.c > ./a.out | tac > > Why would I make it unsigned? As a static analysis developer, > pointlessly unsigned variables are one of the leading causes for the > bugs I see. > > There are times where a iterator counter needs to be unsigned long, or > u64 but I have never seen a case where changing an iterator from > "int i;" to "unsigned int i;" solves a real life kernel bug. It only > introduces bugs. See my followup to that, I meant unsigned int count; do { ... } while (--count); It doesn't solve bug, but prevents the code be read incorrectly like what you are fixing can be avoided with do {} while (); along with unsigned type. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 12:53 ` Andy Shevchenko @ 2021-04-30 13:33 ` Dan Carpenter 2021-04-30 14:21 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-04-30 13:33 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote: > On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote: > > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > > > On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote: > > > > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote: > > > > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote: > > > > > > This loop ends on -1 so the error message will never be printed. > > > > > > > > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown") > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > > > ... > > > > > > > > > > > poll_count--) > > > > > > cpu_relax(); > > > > > > > > > > > > - if (!poll_count) > > > > > > + if (poll_count == -1) > > > > > > > > > > Why not to change poll_count-- to --poll_count? > > > > > > > > > > > > > Either one is fine. I considered several different ways and wrote the > > > > patch twice. The downside of --poll_count is that it's an off by one > > > > in that the author clearly intended to loop 25 times. It doesn't really > > > > matter if we only loop 24 but off by ones are aesthetically unpleasant. > > > > > > I didn't get. If you use --poll_count you get exactly 25 times and moreover, > > > you may convert variable to unsigned type. > > > > > > > Here is a small test to show that it loops 24 times. > > > > #include <stdio.h> > > > > int main(void) > > { > > int i = 25; > > > > while (--i) > > printf("%d\n", i); > > > > return 0; > > } > > > > gcc test.c > > ./a.out | tac > > > > Why would I make it unsigned? As a static analysis developer, > > pointlessly unsigned variables are one of the leading causes for the > > bugs I see. > > > > There are times where a iterator counter needs to be unsigned long, or > > u64 but I have never seen a case where changing an iterator from > > "int i;" to "unsigned int i;" solves a real life kernel bug. It only > > introduces bugs. > > See my followup to that, I meant > > unsigned int count; > > do { > ... > } while (--count); > > It doesn't solve bug, but prevents the code be read incorrectly like what you > are fixing can be avoided with do {} while (); along with unsigned type. > Why would you use an unsigned int for this??? regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 13:33 ` Dan Carpenter @ 2021-04-30 14:21 ` Andy Shevchenko 2021-05-03 6:54 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2021-04-30 14:21 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 04:33:29PM +0300, Dan Carpenter wrote: > On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote: > > On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote: > > > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: ... > > > Why would I make it unsigned? As a static analysis developer, > > > pointlessly unsigned variables are one of the leading causes for the > > > bugs I see. > > > > > > There are times where a iterator counter needs to be unsigned long, or > > > u64 but I have never seen a case where changing an iterator from > > > "int i;" to "unsigned int i;" solves a real life kernel bug. It only > > > introduces bugs. > > > > See my followup to that, I meant > > > > unsigned int count; > > > > do { > > ... > > } while (--count); > > > > It doesn't solve bug, but prevents the code be read incorrectly like what you > > are fixing can be avoided with do {} while (); along with unsigned type. > > Why would you use an unsigned int for this??? Why it should be signed? You clearly show the amount of iterations. Check for null I guess even compact in the assembly in comparison to -1. I do not see any point why it should be signed. For what purpose? It's a *down* counter. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-04-30 14:21 ` Andy Shevchenko @ 2021-05-03 6:54 ` Dan Carpenter 2021-05-03 9:51 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-05-03 6:54 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Fri, Apr 30, 2021 at 05:21:24PM +0300, Andy Shevchenko wrote: > On Fri, Apr 30, 2021 at 04:33:29PM +0300, Dan Carpenter wrote: > > On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote: > > > On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote: > > > > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > > ... > > > > > Why would I make it unsigned? As a static analysis developer, > > > > pointlessly unsigned variables are one of the leading causes for the > > > > bugs I see. > > > > > > > > There are times where a iterator counter needs to be unsigned long, or > > > > u64 but I have never seen a case where changing an iterator from > > > > "int i;" to "unsigned int i;" solves a real life kernel bug. It only > > > > introduces bugs. > > > > > > See my followup to that, I meant > > > > > > unsigned int count; > > > > > > do { > > > ... > > > } while (--count); > > > > > > It doesn't solve bug, but prevents the code be read incorrectly like what you > > > are fixing can be avoided with do {} while (); along with unsigned type. > > > > Why would you use an unsigned int for this??? > > Why it should be signed? You clearly show the amount of iterations. Check for > null I guess even compact in the assembly in comparison to -1. > > I do not see any point why it should be signed. For what purpose? > > It's a *down* counter. Yeah. And people regularly test down counters for >= 0. Signed ints are safer. Unsigned ints are a *leading* cause of bugs in the kernel. I don't know if they're in the top five but they're definitely in the top ten. Also if you need a larger type you should switch to a 64 bit type. The 2-4 million range is very narrow. I have never seen a single kernel bug where the for loop counter was "int i;" and making it "unsigned int i;" fixed a real life kernel bug. Of course, there are times when unsigned int is appropriate, like for sizes or because it's in the spec. It's frustrating to me because GCC encourages people to make loop counters unsigned and it introduces bugs. I'm looking at the git log right now and I see that someone changed: void dt_to_asm(FILE *f, struct dt_info *dti, int version) { struct version_info *vi = NULL; - int i; + unsigned int i; struct data strbuf = empty_data; struct reserve_info *re; const char *symprefix = "dt"; There are two loops in that function: for (i = 0; i < ARRAY_SIZE(version_table); i++) { This the one that generates the warning. GCC knows at compile time that ARRAY_SIZE() is 5. ARGH!!! GCC is so lazy and horrible. If I did this in Smatch people would never accept it. Even if ARRAY_SIZE() were higher than INT_MAX the loop would behave the same regardless of whether it was signed or not because of type promotion. The other loop is: for (i = 0; i < reservenum; i++) { In this case "reservenum" comes from the command line. In the original code if it were negative that would be a harmless no-op but now because i is unsigned it's a crashing bug. Why did GCC not generate a warning for this? The code was obviously bad before, that's true, but now in a very measurable way it has become worse. This example is not really important. I only brought it up because it is most recent example of people changing "int i;" to "unsigned int i;". But there have been other cases like this which have had a security impact. regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-05-03 6:54 ` Dan Carpenter @ 2021-05-03 9:51 ` Andy Shevchenko 2021-05-14 8:00 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2021-05-03 9:51 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Mon, May 03, 2021 at 09:54:39AM +0300, Dan Carpenter wrote: > On Fri, Apr 30, 2021 at 05:21:24PM +0300, Andy Shevchenko wrote: > > On Fri, Apr 30, 2021 at 04:33:29PM +0300, Dan Carpenter wrote: > > > On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote: > > > > On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote: > > > > > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote: > > > > ... > > > > > > > Why would I make it unsigned? As a static analysis developer, > > > > > pointlessly unsigned variables are one of the leading causes for the > > > > > bugs I see. > > > > > > > > > > There are times where a iterator counter needs to be unsigned long, or > > > > > u64 but I have never seen a case where changing an iterator from > > > > > "int i;" to "unsigned int i;" solves a real life kernel bug. It only > > > > > introduces bugs. > > > > > > > > See my followup to that, I meant > > > > > > > > unsigned int count; > > > > > > > > do { > > > > ... > > > > } while (--count); > > > > > > > > It doesn't solve bug, but prevents the code be read incorrectly like what you > > > > are fixing can be avoided with do {} while (); along with unsigned type. > > > > > > Why would you use an unsigned int for this??? > > > > Why it should be signed? You clearly show the amount of iterations. Check for > > null I guess even compact in the assembly in comparison to -1. > > > > I do not see any point why it should be signed. For what purpose? > > > > It's a *down* counter. > > Yeah. And people regularly test down counters for >= 0. I don't know that. What I see the test is as simple as while (--count) which is basically > 0. > Signed ints > are safer. Any research article about it? What about wrong integral promotions which I consider is a root cause of many bugs? People should learn that, or the C (standard) should be fixed to make it easier to get. > Unsigned ints are a *leading* cause of bugs in the kernel. Again, where this statistics comes from? Maybe it's a simple answer to the question that review in kernel is not good enough? > I don't know if they're in the top five but they're definitely in the > top ten. Again, I don't see any proofs. > Also if you need a larger type you should switch to a 64 bit type. The > 2-4 million range is very narrow. > > I have never seen a single kernel bug where the for loop counter was > "int i;" and making it "unsigned int i;" fixed a real life kernel bug. Your very patch suggests one of the solution to switch to unsigned to fix a kernel bug (though with lowest severity). > Of course, there are times when unsigned int is appropriate, like for > sizes or because it's in the spec. > > It's frustrating to me because GCC encourages people to make loop > counters unsigned and it introduces bugs. Any code which was written in unthought manner is a buggy code. So, how unsigned vs. signed loop counter any different here to any other buggy code? > I'm looking at the git log right now and I see that someone changed: > > void dt_to_asm(FILE *f, struct dt_info *dti, int version) > { > struct version_info *vi = NULL; > - int i; > + unsigned int i; > struct data strbuf = empty_data; > struct reserve_info *re; > const char *symprefix = "dt"; > > There are two loops in that function: > > for (i = 0; i < ARRAY_SIZE(version_table); i++) { > > This the one that generates the warning. GCC knows at compile time that > ARRAY_SIZE() is 5. ARGH!!! GCC is so lazy and horrible. If I did this > in Smatch people would never accept it. Even if ARRAY_SIZE() were > higher than INT_MAX the loop would behave the same regardless of whether > it was signed or not because of type promotion. > > The other loop is: > > for (i = 0; i < reservenum; i++) { > > In this case "reservenum" comes from the command line. In the original > code if it were negative that would be a harmless no-op but now because > i is unsigned it's a crashing bug. Why did GCC not generate a warning > for this? The code was obviously bad before, that's true, but now in a > very measurable way it has become worse. See above. I think the root cause that people do not understand C and how to program in C in bug-less manner. > This example is not really important. I only brought it up because it > is most recent example of people changing "int i;" to "unsigned int i;". > But there have been other cases like this which have had a security > impact. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] serial: 8250_omap: fix a timeout loop condition 2021-05-03 9:51 ` Andy Shevchenko @ 2021-05-14 8:00 ` Dan Carpenter 0 siblings, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2021-05-14 8:00 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra, Alexander Sverdlin, Johan Hovold, linux-serial, kernel-janitors On Mon, May 03, 2021 at 12:51:38PM +0300, Andy Shevchenko wrote: > > Signed ints > > are safer. > > Any research article about it? What about wrong integral promotions which > I consider is a root cause of many bugs? People should learn that, or the > C (standard) should be fixed to make it easier to get. > > > Unsigned ints are a *leading* cause of bugs in the kernel. > > Again, where this statistics comes from? Maybe it's a simple answer to the > question that review in kernel is not good enough? > When I say that unsigned ints are a leading cause of bug I'm talking about things like commit 95b079d8215b ("swiotlb: Fix the type of index") which does this: - unsigned int index, i; + unsigned int i; + int index; This is perhaps a poor example, because the patch doesn't really fix a bug. I don't think that the author thought about how type promotion works or else the commit would have said "this commit does not change runtime behavior". The other reason it's a bad example, is that although a "int i;" would work here in real life, the correct type is unsigned long i. The size of the allocation is specified by the user and allocated at boot with memblock_alloc() so it might actually be possible to allocate gigabytes of memory. I said that earlier, that if you really need an unsigned list counter then it's pretty likely it should be "unsigned long" instead of "unsigned int". But what I get from this example is that people are just declaring things "unsigned int" for no reason. I expect that unsigned comparisons with zero will drop off now that GCC has a compile time warning for that. I've been developing the kernel for a decade now and I've probably had to deal with one of these bugs every ten days on average over that period. Meanwhile GCC urges people to declare the list counter as "unsigned int i;" but I have never seen that fix even a single real life kernel bug. I'm not an academic but I have looked for these kinds of bugs. GCC warns about it even when it can see at compile time that the type makes no difference. There are performance reasons for unsigned types. Also inside structs then unsigned ints can make sense. Also it's nice to declare array indexes as unsigned because it means that every time you check against the max, there is a hidden check against negatives because of type promotion. But for list counters which start at zero and increment up then none of that applies. High level languages like Python don't have signed types. Pascal had unsigned types but Niklaus Wirth got rid of them in later programming languages like Oberon. Unsigned types and type promotion can help with performance but they make things more complicated. regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-05-14 8:00 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-29 7:19 [PATCH] serial: 8250_omap: fix a timeout loop condition Dan Carpenter 2021-04-29 9:23 ` Alexander Sverdlin 2021-04-29 11:08 ` Andy Shevchenko 2021-04-29 13:02 ` Dan Carpenter 2021-04-30 8:46 ` Andy Shevchenko 2021-04-30 8:47 ` Andy Shevchenko 2021-04-30 11:41 ` Dan Carpenter 2021-04-30 12:53 ` Andy Shevchenko 2021-04-30 13:33 ` Dan Carpenter 2021-04-30 14:21 ` Andy Shevchenko 2021-05-03 6:54 ` Dan Carpenter 2021-05-03 9:51 ` Andy Shevchenko 2021-05-14 8:00 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.