* Potential static analysis ideas @ 2021-07-23 19:10 Dan Carpenter 2021-07-24 13:33 ` Geert Uytterhoeven 2021-07-26 15:50 ` Paul E. McKenney 0 siblings, 2 replies; 30+ messages in thread From: Dan Carpenter @ 2021-07-23 19:10 UTC (permalink / raw) To: ksummit; +Cc: Julia Lawall Rust has many good static analysis features but if we wanted we could implement a number of stricter rules in C. With Smatch I have tried to focus on exclusively on finding bugs because everyone can agree that bugs are bad. But if some subsystems wanted to implement stricter rules just as a hardenning measure then that's a doable thing. For example, I've tried a bunch of approaches to warning about when the user can trigger an integer overflow. The challenge is that most integer overflows are harmless and do not cause a real life bug. I've also written some checks for locking bugs where the warning is "expected lock 'foo' held when accessing struct member 'bar'". The problem is that I'm not a locking expert. My process is 1) Infer the pairing between this lock protect this pointer. 2) Make list of places where the pointer holds a function pointer and we free it under lock (I still need to add the locking part). 3) Make a list of places where we access the data without holding the lock and the attacker can control the timing with userfaultd or whatever. This sort of locking check which finds real life exploitable bugs is much more complicated to write. If there were subsystems which had simpler, hardenned rules then that would be easier. Another idea is that if I were a subsystem maintainer, I think I would enforce my prefered error handling rules. 1) Every allocation function must have exactly one free function. 2) Every function should clean up after itself on failure. 3) Never free things that aren't allocated. 4) The frees must happen in reverse order from how they were allocated. These days we have disabled GCC's uninitialized warnings so it falls to static analysis devs to review all the new warnings. I sometimes ignore warnings if they look like probably they aren't a bug. Does this function allow zero size writes? Does this switch statement really need a default case? Maybe sometimes I miss bugs. Here is another example of something which I have a check for but I haven't pushed. ret = frob(); if (!ret) return ret; This code is normally correct but sometimes it means the if statement is reversed and it should be "if (ret) return ret;". To me returning a literal is always more clear but but clearly the original author felt "ret" was more clear... It's only a few bugs per year so it's not a big deal either way. A sort of related example that I have pushed is this: int ret = 0; /* 20 lines of code */ if (condition) goto cleanup; These trigger a published Smatch warning for missing error codes but I only email people when I can't understand the code. Please, put the "ret = 0;" within 4 lines of the goto. Those are just some ideas of things we could do if we wanted to use static analysis for hardenning instead of for fixing bugs. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-23 19:10 Potential static analysis ideas Dan Carpenter @ 2021-07-24 13:33 ` Geert Uytterhoeven 2021-07-24 13:40 ` Julia Lawall ` (3 more replies) 2021-07-26 15:50 ` Paul E. McKenney 1 sibling, 4 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-24 13:33 UTC (permalink / raw) To: Dan Carpenter; +Cc: ksummit, Julia Lawall Hi Dan, On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > These days we have disabled GCC's uninitialized warnings so it falls > to static analysis devs to review all the new warnings. I sometimes > ignore warnings if they look like probably they aren't a bug. Does this > function allow zero size writes? Does this switch statement really need > a default case? Maybe sometimes I miss bugs. Yeah, I tended to find a few real bugs every release using the good old gcc 4.2, before support for it was dropped. I hope someone still runs modern compilers with GCC's uninitialized warnings enabled again? > Here is another example of something which I have a check for but I > haven't pushed. > > ret = frob(); > if (!ret) > return ret; > > This code is normally correct but sometimes it means the if statement is > reversed and it should be "if (ret) return ret;". To me returning a > literal is always more clear but but clearly the original author felt > "ret" was more clear... It's only a few bugs per year so it's not a big > deal either way. To make it work well, you need to know if frob() and/or the current function return an error code or not. While you can use some heuristics (e.g. is there any return -Exxx), perhaps we can add an annotation to indicate if a function returns an error code, or an error pointer? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 13:33 ` Geert Uytterhoeven @ 2021-07-24 13:40 ` Julia Lawall 2021-07-24 14:08 ` Arnd Bergmann ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Julia Lawall @ 2021-07-24 13:40 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Dan Carpenter, ksummit On Sat, 24 Jul 2021, Geert Uytterhoeven wrote: > Hi Dan, > > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > These days we have disabled GCC's uninitialized warnings so it falls > > to static analysis devs to review all the new warnings. I sometimes > > ignore warnings if they look like probably they aren't a bug. Does this > > function allow zero size writes? Does this switch statement really need > > a default case? Maybe sometimes I miss bugs. > > Yeah, I tended to find a few real bugs every release using the good old > gcc 4.2, before support for it was dropped. > I hope someone still runs modern compilers with GCC's uninitialized > warnings enabled again? > > > Here is another example of something which I have a check for but I > > haven't pushed. > > > > ret = frob(); > > if (!ret) > > return ret; > > > > This code is normally correct but sometimes it means the if statement is > > reversed and it should be "if (ret) return ret;". To me returning a > > literal is always more clear but but clearly the original author felt > > "ret" was more clear... It's only a few bugs per year so it's not a big > > deal either way. > > To make it work well, you need to know if frob() and/or the current > function return an error code or not. While you can use some heuristics > (e.g. is there any return -Exxx), perhaps we can add an annotation to > indicate if a function returns an error code, or an error pointer? FWIW, the ability of Coccinelle to add and match against annotations is currently being greatly improved by Keisuke Nishimura. julia > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 13:33 ` Geert Uytterhoeven 2021-07-24 13:40 ` Julia Lawall @ 2021-07-24 14:08 ` Arnd Bergmann 2021-07-24 23:18 ` Laurent Pinchart 2021-07-26 7:05 ` Dan Carpenter 3 siblings, 0 replies; 30+ messages in thread From: Arnd Bergmann @ 2021-07-24 14:08 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Dan Carpenter, ksummit, Julia Lawall On Sat, Jul 24, 2021 at 3:33 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Dan, > > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > These days we have disabled GCC's uninitialized warnings so it falls > > to static analysis devs to review all the new warnings. I sometimes > > ignore warnings if they look like probably they aren't a bug. Does this > > function allow zero size writes? Does this switch statement really need > > a default case? Maybe sometimes I miss bugs. > > Yeah, I tended to find a few real bugs every release using the good old > gcc 4.2, before support for it was dropped. > I hope someone still runs modern compilers with GCC's uninitialized > warnings enabled again? Unfortunately, the new inlining in gcc-10 and up has made the Wmaybe-uninitialized option pretty much unusable, especially since all the uninitialized_var() annotations are gone. I tried for a while to keep that running on my randconfig build machine, but the number of warnings that got added was overwhelming after a while. This does contain a number of real bugs, and at least clang and smatch can both catch a notable subset of those, but it would be good to have a replacement that warns developers about not adding uninitialized variable uses in the first place. Arnd ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 13:33 ` Geert Uytterhoeven 2021-07-24 13:40 ` Julia Lawall 2021-07-24 14:08 ` Arnd Bergmann @ 2021-07-24 23:18 ` Laurent Pinchart 2021-07-24 23:45 ` NeilBrown 2021-07-26 7:05 ` Dan Carpenter 3 siblings, 1 reply; 30+ messages in thread From: Laurent Pinchart @ 2021-07-24 23:18 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Dan Carpenter, ksummit, Julia Lawall On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote: > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter wrote: > > These days we have disabled GCC's uninitialized warnings so it falls > > to static analysis devs to review all the new warnings. I sometimes > > ignore warnings if they look like probably they aren't a bug. Does this > > function allow zero size writes? Does this switch statement really need > > a default case? Maybe sometimes I miss bugs. > > Yeah, I tended to find a few real bugs every release using the good old > gcc 4.2, before support for it was dropped. > I hope someone still runs modern compilers with GCC's uninitialized > warnings enabled again? > > > Here is another example of something which I have a check for but I > > haven't pushed. > > > > ret = frob(); > > if (!ret) > > return ret; > > > > This code is normally correct but sometimes it means the if statement is > > reversed and it should be "if (ret) return ret;". To me returning a > > literal is always more clear but but clearly the original author felt > > "ret" was more clear... It's only a few bugs per year so it's not a big > > deal either way. > > To make it work well, you need to know if frob() and/or the current > function return an error code or not. While you can use some heuristics > (e.g. is there any return -Exxx), perhaps we can add an annotation to > indicate if a function returns an error code, or an error pointer? https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ I think it would be useful, if not for the tools, at least for developers. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 23:18 ` Laurent Pinchart @ 2021-07-24 23:45 ` NeilBrown 2021-07-26 7:25 ` Arnd Bergmann 0 siblings, 1 reply; 30+ messages in thread From: NeilBrown @ 2021-07-24 23:45 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Geert Uytterhoeven, Dan Carpenter, ksummit, Julia Lawall On Sun, 25 Jul 2021, Laurent Pinchart wrote: > On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote: > > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter wrote: > > > These days we have disabled GCC's uninitialized warnings so it falls > > > to static analysis devs to review all the new warnings. I sometimes > > > ignore warnings if they look like probably they aren't a bug. Does this > > > function allow zero size writes? Does this switch statement really need > > > a default case? Maybe sometimes I miss bugs. > > > > Yeah, I tended to find a few real bugs every release using the good old > > gcc 4.2, before support for it was dropped. > > I hope someone still runs modern compilers with GCC's uninitialized > > warnings enabled again? > > > > > Here is another example of something which I have a check for but I > > > haven't pushed. > > > > > > ret = frob(); > > > if (!ret) > > > return ret; > > > > > > This code is normally correct but sometimes it means the if statement is > > > reversed and it should be "if (ret) return ret;". To me returning a > > > literal is always more clear but but clearly the original author felt > > > "ret" was more clear... It's only a few bugs per year so it's not a big > > > deal either way. > > > > To make it work well, you need to know if frob() and/or the current > > function return an error code or not. While you can use some heuristics > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > indicate if a function returns an error code, or an error pointer? > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > I think it would be useful, if not for the tools, at least for > developers. Agreed. I added some code to smatch so that I could annotate pointers to say if they are allowed to be NULL. The implementation isn't perfect, but I love having that extra documentation about when I do or don't have to check for NULL. NeilBrown ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 23:45 ` NeilBrown @ 2021-07-26 7:25 ` Arnd Bergmann 2021-07-26 7:53 ` Geert Uytterhoeven 2021-07-26 21:43 ` NeilBrown 0 siblings, 2 replies; 30+ messages in thread From: Arnd Bergmann @ 2021-07-26 7:25 UTC (permalink / raw) To: NeilBrown Cc: Laurent Pinchart, Geert Uytterhoeven, Dan Carpenter, ksummit, Julia Lawall On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > To make it work well, you need to know if frob() and/or the current > > > function return an error code or not. While you can use some heuristics > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > indicate if a function returns an error code, or an error pointer? > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > I think it would be useful, if not for the tools, at least for > > developers. > > Agreed. I added some code to smatch so that I could annotate pointers to > say if they are allowed to be NULL. The implementation isn't perfect, > but I love having that extra documentation about when I do or don't have > to check for NULL. I can think of four different annotations that limit what a pointer return from a function can be: a) either a valid pointer or NULL, but never an error pointer, b) either a valid pointer or an error pointer, but not NULL, c) always a valid pointer, never NULL or an error, d) always NULL, but callers are expected to check for error pointers. The last one is the tricky bit because there are a couple of subsystems that intentionally return an opaque NULL pointer to indicate success when the subsystem is compile-time disabled, and expect callers to not dereference that pointer but instead pass it into other stubbed-out interfaces. I think this one is rather specific to the kernel, but also really useful to have in a static checker because everyone gets those wrong. Arnd ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 7:25 ` Arnd Bergmann @ 2021-07-26 7:53 ` Geert Uytterhoeven 2021-07-26 8:20 ` Arnd Bergmann 2021-07-26 21:43 ` NeilBrown 1 sibling, 1 reply; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-26 7:53 UTC (permalink / raw) To: Arnd Bergmann Cc: NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit, Julia Lawall Hi Arnd, On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > To make it work well, you need to know if frob() and/or the current > > > > function return an error code or not. While you can use some heuristics > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > indicate if a function returns an error code, or an error pointer? > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > I think it would be useful, if not for the tools, at least for > > > developers. > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > say if they are allowed to be NULL. The implementation isn't perfect, > > but I love having that extra documentation about when I do or don't have > > to check for NULL. > > I can think of four different annotations that limit what a pointer return from > a function can be: > > a) either a valid pointer or NULL, but never an error pointer, > b) either a valid pointer or an error pointer, but not NULL, > c) always a valid pointer, never NULL or an error, > d) always NULL, but callers are expected to check for error pointers. e) either a valid pointer, NULL, or an error pointer The last pattern is seen with the various *get*_optional() functions. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 7:53 ` Geert Uytterhoeven @ 2021-07-26 8:20 ` Arnd Bergmann 2021-07-26 8:39 ` Geert Uytterhoeven ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Arnd Bergmann @ 2021-07-26 8:20 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit, Julia Lawall On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > To make it work well, you need to know if frob() and/or the current > > > > > function return an error code or not. While you can use some heuristics > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > > indicate if a function returns an error code, or an error pointer? > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > I think it would be useful, if not for the tools, at least for > > > > developers. > > > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > > say if they are allowed to be NULL. The implementation isn't perfect, > > > but I love having that extra documentation about when I do or don't have > > > to check for NULL. > > > > I can think of four different annotations that limit what a pointer return from > > a function can be: > > > > a) either a valid pointer or NULL, but never an error pointer, > > b) either a valid pointer or an error pointer, but not NULL, > > c) always a valid pointer, never NULL or an error, > > d) always NULL, but callers are expected to check for error pointers. > > e) either a valid pointer, NULL, or an error pointer > > The last pattern is seen with the various *get*_optional() functions. I would always consider those the exact bug that I meant with "because everyone gets those wrong". I think the idea of the "optional" functions is that you have two implementations b) and d) and pick one of them at compile time. To the caller this means either an error pointer or success, but checking for NULL is a bug in the caller, while conditionally returning NULL or ERR_PTR() would be a bug in the interface. Arnd ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:20 ` Arnd Bergmann @ 2021-07-26 8:39 ` Geert Uytterhoeven 2021-07-26 8:52 ` Arnd Bergmann 2021-07-26 8:55 ` Julia Lawall 2021-07-26 9:13 ` Dan Carpenter 2 siblings, 1 reply; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-26 8:39 UTC (permalink / raw) To: Arnd Bergmann Cc: NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit, Julia Lawall Hi Arnd, On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > To make it work well, you need to know if frob() and/or the current > > > > > > function return an error code or not. While you can use some heuristics > > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > > > indicate if a function returns an error code, or an error pointer? > > > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > > > I think it would be useful, if not for the tools, at least for > > > > > developers. > > > > > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > > > say if they are allowed to be NULL. The implementation isn't perfect, > > > > but I love having that extra documentation about when I do or don't have > > > > to check for NULL. > > > > > > I can think of four different annotations that limit what a pointer return from > > > a function can be: > > > > > > a) either a valid pointer or NULL, but never an error pointer, > > > b) either a valid pointer or an error pointer, but not NULL, > > > c) always a valid pointer, never NULL or an error, > > > d) always NULL, but callers are expected to check for error pointers. > > > > e) either a valid pointer, NULL, or an error pointer > > > > The last pattern is seen with the various *get*_optional() functions. > > I would always consider those the exact bug that I meant with "because > everyone gets those wrong". I think the idea of the "optional" functions is > that you have two implementations b) and d) and pick one of them > at compile time. To the caller this means either an error pointer or > success, but checking for NULL is a bug in the caller, while conditionally > returning NULL or ERR_PTR() would be a bug in the interface. e) is not for optional kernel features, but for optional hardware functionality. So it is not known at compile time, but depends on e.g. the DTB. See e.g. devm_clk_get_optional(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:39 ` Geert Uytterhoeven @ 2021-07-26 8:52 ` Arnd Bergmann 2021-07-26 9:11 ` Geert Uytterhoeven 0 siblings, 1 reply; 30+ messages in thread From: Arnd Bergmann @ 2021-07-26 8:52 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit, Julia Lawall On Mon, Jul 26, 2021 at 10:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > e) either a valid pointer, NULL, or an error pointer > > > > > > The last pattern is seen with the various *get*_optional() functions. > > > > I would always consider those the exact bug that I meant with "because > > everyone gets those wrong". I think the idea of the "optional" functions is > > that you have two implementations b) and d) and pick one of them > > at compile time. To the caller this means either an error pointer or > > success, but checking for NULL is a bug in the caller, while conditionally > > returning NULL or ERR_PTR() would be a bug in the interface. > > e) is not for optional kernel features, but for optional hardware > functionality. So it is not known at compile time, but depends on > e.g. the DTB. See e.g. devm_clk_get_optional(). Ah right, I forgot we had those as well. However, I suppose no amount of annotation is going to help with those, as far as the checker is concerned, I would just leave this as not annotated. At least with the clk API, the callers are not able to dereference the pointer because the definition of 'struct clk' is private to drivers/clk/clk.c. Arnd ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:52 ` Arnd Bergmann @ 2021-07-26 9:11 ` Geert Uytterhoeven 0 siblings, 0 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-26 9:11 UTC (permalink / raw) To: Arnd Bergmann Cc: NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit, Julia Lawall Hi Arnd, On Mon, Jul 26, 2021 at 10:52 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Jul 26, 2021 at 10:39 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > > > e) either a valid pointer, NULL, or an error pointer > > > > > > > > The last pattern is seen with the various *get*_optional() functions. > > > > > > I would always consider those the exact bug that I meant with "because > > > everyone gets those wrong". I think the idea of the "optional" functions is > > > that you have two implementations b) and d) and pick one of them > > > at compile time. To the caller this means either an error pointer or > > > success, but checking for NULL is a bug in the caller, while conditionally > > > returning NULL or ERR_PTR() would be a bug in the interface. > > > > e) is not for optional kernel features, but for optional hardware > > functionality. So it is not known at compile time, but depends on > > e.g. the DTB. See e.g. devm_clk_get_optional(). > > Ah right, I forgot we had those as well. However, I suppose no amount > of annotation is going to help with those, as far as the checker is concerned, > I would just leave this as not annotated. At least with the clk API, > the callers are not able to dereference the pointer because the definition > of 'struct clk' is private to drivers/clk/clk.c. And the clk API supports calling further clock ops with a NULL pointer, which eases handling optional clocks. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:20 ` Arnd Bergmann 2021-07-26 8:39 ` Geert Uytterhoeven @ 2021-07-26 8:55 ` Julia Lawall 2021-07-26 9:08 ` Hannes Reinecke 2021-07-26 9:13 ` Dan Carpenter 2 siblings, 1 reply; 30+ messages in thread From: Julia Lawall @ 2021-07-26 8:55 UTC (permalink / raw) To: Arnd Bergmann Cc: Geert Uytterhoeven, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit On Mon, 26 Jul 2021, Arnd Bergmann wrote: > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > To make it work well, you need to know if frob() and/or the current > > > > > > function return an error code or not. While you can use some heuristics > > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > > > indicate if a function returns an error code, or an error pointer? > > > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > > > I think it would be useful, if not for the tools, at least for > > > > > developers. > > > > > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > > > say if they are allowed to be NULL. The implementation isn't perfect, > > > > but I love having that extra documentation about when I do or don't have > > > > to check for NULL. > > > > > > I can think of four different annotations that limit what a pointer return from > > > a function can be: > > > > > > a) either a valid pointer or NULL, but never an error pointer, > > > b) either a valid pointer or an error pointer, but not NULL, > > > c) always a valid pointer, never NULL or an error, > > > d) always NULL, but callers are expected to check for error pointers. > > > > e) either a valid pointer, NULL, or an error pointer > > > > The last pattern is seen with the various *get*_optional() functions. > > I would always consider those the exact bug that I meant with "because > everyone gets those wrong". I think the idea of the "optional" functions is > that you have two implementations b) and d) and pick one of them > at compile time. To the caller this means either an error pointer or > success, but checking for NULL is a bug in the caller, while conditionally > returning NULL or ERR_PTR() would be a bug in the interface. I'm not sure to understand the "bug in the caller" part. Couldn't there be two possible definitions of the called function, according to different configuration options, and a single caller that calls both? Also, over 230 files contain functions that return both NULL and ERR_PTR. A random example, chosen for conciseness, is the following from fs/overlayfs/inode.c: struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, bool is_upper) { struct inode *inode, *key = d_inode(real); inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); if (!inode) return NULL; if (!ovl_verify_inode(inode, is_upper ? NULL : real, is_upper ? real : NULL, false)) { iput(inode); return ERR_PTR(-ESTALE); } return inode; } julia ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:55 ` Julia Lawall @ 2021-07-26 9:08 ` Hannes Reinecke 2021-07-26 9:16 ` Geert Uytterhoeven 2021-07-26 9:17 ` Dan Carpenter 0 siblings, 2 replies; 30+ messages in thread From: Hannes Reinecke @ 2021-07-26 9:08 UTC (permalink / raw) To: Julia Lawall, Arnd Bergmann Cc: Geert Uytterhoeven, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit On 7/26/21 10:55 AM, Julia Lawall wrote: > > > On Mon, 26 Jul 2021, Arnd Bergmann wrote: > >> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: >>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: >>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote: >>>>>>> To make it work well, you need to know if frob() and/or the current >>>>>>> function return an error code or not. While you can use some heuristics >>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to >>>>>>> indicate if a function returns an error code, or an error pointer? >>>>>> >>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ >>>>>> >>>>>> I think it would be useful, if not for the tools, at least for >>>>>> developers. >>>>> >>>>> Agreed. I added some code to smatch so that I could annotate pointers to >>>>> say if they are allowed to be NULL. The implementation isn't perfect, >>>>> but I love having that extra documentation about when I do or don't have >>>>> to check for NULL. >>>> >>>> I can think of four different annotations that limit what a pointer return from >>>> a function can be: >>>> >>>> a) either a valid pointer or NULL, but never an error pointer, >>>> b) either a valid pointer or an error pointer, but not NULL, >>>> c) always a valid pointer, never NULL or an error, >>>> d) always NULL, but callers are expected to check for error pointers. >>> >>> e) either a valid pointer, NULL, or an error pointer >>> >>> The last pattern is seen with the various *get*_optional() functions. >> >> I would always consider those the exact bug that I meant with "because >> everyone gets those wrong". I think the idea of the "optional" functions is >> that you have two implementations b) and d) and pick one of them >> at compile time. To the caller this means either an error pointer or >> success, but checking for NULL is a bug in the caller, while conditionally >> returning NULL or ERR_PTR() would be a bug in the interface. > > I'm not sure to understand the "bug in the caller" part. Couldn't there > be two possible definitions of the called function, according to different > configuration options, and a single caller that calls both? > > Also, over 230 files contain functions that return both NULL and ERR_PTR. > A random example, chosen for conciseness, is the following from > fs/overlayfs/inode.c: > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > bool is_upper) > { > struct inode *inode, *key = d_inode(real); > > inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); > if (!inode) > return NULL; > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > is_upper ? real : NULL, false)) { > iput(inode); > return ERR_PTR(-ESTALE); > } > > return inode; > } > And that I would consider a coding error. If a function is able to return an error pointer it should _always_ return an error pointer; here it would be trivial to return -ENXIO instead of NULL in the first condition. Not doing so is just sloppy programming IMO. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:08 ` Hannes Reinecke @ 2021-07-26 9:16 ` Geert Uytterhoeven 2021-07-26 9:28 ` Julia Lawall 2021-07-26 17:54 ` James Bottomley 2021-07-26 9:17 ` Dan Carpenter 1 sibling, 2 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-26 9:16 UTC (permalink / raw) To: Hannes Reinecke Cc: Julia Lawall, Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit Hi Hannes, On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke <hare@suse.de> wrote: > On 7/26/21 10:55 AM, Julia Lawall wrote: > > On Mon, 26 Jul 2021, Arnd Bergmann wrote: > >> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > >>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > >>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote: > >>>>>>> To make it work well, you need to know if frob() and/or the current > >>>>>>> function return an error code or not. While you can use some heuristics > >>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to > >>>>>>> indicate if a function returns an error code, or an error pointer? > >>>>>> > >>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > >>>>>> > >>>>>> I think it would be useful, if not for the tools, at least for > >>>>>> developers. > >>>>> > >>>>> Agreed. I added some code to smatch so that I could annotate pointers to > >>>>> say if they are allowed to be NULL. The implementation isn't perfect, > >>>>> but I love having that extra documentation about when I do or don't have > >>>>> to check for NULL. > >>>> > >>>> I can think of four different annotations that limit what a pointer return from > >>>> a function can be: > >>>> > >>>> a) either a valid pointer or NULL, but never an error pointer, > >>>> b) either a valid pointer or an error pointer, but not NULL, > >>>> c) always a valid pointer, never NULL or an error, > >>>> d) always NULL, but callers are expected to check for error pointers. > >>> > >>> e) either a valid pointer, NULL, or an error pointer > >>> > >>> The last pattern is seen with the various *get*_optional() functions. > >> > >> I would always consider those the exact bug that I meant with "because > >> everyone gets those wrong". I think the idea of the "optional" functions is > >> that you have two implementations b) and d) and pick one of them > >> at compile time. To the caller this means either an error pointer or > >> success, but checking for NULL is a bug in the caller, while conditionally > >> returning NULL or ERR_PTR() would be a bug in the interface. > > > > I'm not sure to understand the "bug in the caller" part. Couldn't there > > be two possible definitions of the called function, according to different > > configuration options, and a single caller that calls both? > > > > Also, over 230 files contain functions that return both NULL and ERR_PTR. > > A random example, chosen for conciseness, is the following from > > fs/overlayfs/inode.c: > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > > bool is_upper) > > { > > struct inode *inode, *key = d_inode(real); > > > > inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); > > if (!inode) > > return NULL; > > > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > > is_upper ? real : NULL, false)) { > > iput(inode); > > return ERR_PTR(-ESTALE); > > } > > > > return inode; > > } > > > And that I would consider a coding error. > If a function is able to return an error pointer it should _always_ > return an error pointer; here it would be trivial to return -ENXIO > instead of NULL in the first condition. > > Not doing so is just sloppy programming IMO. In this case I agree. For optional resources, the ability to return NULL (and for the caller not having to care if the check should be for -ENOENT, -ENODEV, or something else), and accept NULL for further operations greatly simplifies the handling of optional resources like clocks, enable gpios, resets, regulators, ... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:16 ` Geert Uytterhoeven @ 2021-07-26 9:28 ` Julia Lawall 2021-07-26 9:35 ` Hannes Reinecke 2021-07-26 17:54 ` James Bottomley 1 sibling, 1 reply; 30+ messages in thread From: Julia Lawall @ 2021-07-26 9:28 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Hannes Reinecke, Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit > > > Also, over 230 files contain functions that return both NULL and ERR_PTR. > > > A random example, chosen for conciseness, is the following from > > > fs/overlayfs/inode.c: > > > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > > > bool is_upper) > > > { > > > struct inode *inode, *key = d_inode(real); > > > > > > inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); > > > if (!inode) > > > return NULL; > > > > > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > > > is_upper ? real : NULL, false)) { > > > iput(inode); > > > return ERR_PTR(-ESTALE); > > > } > > > > > > return inode; > > > } > > > > > And that I would consider a coding error. > > If a function is able to return an error pointer it should _always_ > > return an error pointer; here it would be trivial to return -ENXIO > > instead of NULL in the first condition. > > > > Not doing so is just sloppy programming IMO. > > In this case I agree. I looked at the calling context, and it is not so obvious. There are different behaviors in the two cases at both callsites. It is like what Dan describes. If the thing is not available, we just move on. If it is available some action is needed. If there is an actual error, some error handling is needed. julia > > For optional resources, the ability to return NULL (and for the caller > not having to care if the check should be for -ENOENT, -ENODEV, > or something else), and accept NULL for further operations greatly > simplifies the handling of optional resources like clocks, enable > gpios, resets, regulators, ... > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:28 ` Julia Lawall @ 2021-07-26 9:35 ` Hannes Reinecke 2021-07-26 10:03 ` Julia Lawall 0 siblings, 1 reply; 30+ messages in thread From: Hannes Reinecke @ 2021-07-26 9:35 UTC (permalink / raw) To: Julia Lawall, Geert Uytterhoeven Cc: Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit On 7/26/21 11:28 AM, Julia Lawall wrote: >>>> Also, over 230 files contain functions that return both NULL and ERR_PTR. >>>> A random example, chosen for conciseness, is the following from >>>> fs/overlayfs/inode.c: >>>> >>>> struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, >>>> bool is_upper) >>>> { >>>> struct inode *inode, *key = d_inode(real); >>>> >>>> inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); >>>> if (!inode) >>>> return NULL; >>>> >>>> if (!ovl_verify_inode(inode, is_upper ? NULL : real, >>>> is_upper ? real : NULL, false)) { >>>> iput(inode); >>>> return ERR_PTR(-ESTALE); >>>> } >>>> >>>> return inode; >>>> } >>>> >>> And that I would consider a coding error. >>> If a function is able to return an error pointer it should _always_ >>> return an error pointer; here it would be trivial to return -ENXIO >>> instead of NULL in the first condition. >>> >>> Not doing so is just sloppy programming IMO. >> >> In this case I agree. > > I looked at the calling context, and it is not so obvious. There are > different behaviors in the two cases at both callsites. It is like what > Dan describes. If the thing is not available, we just move on. If it is > available some action is needed. If there is an actual error, some error > handling is needed. > But isn't 'not available' an error, too? And isn't that exactly why we have individual errnos? Why do we have to introduce different classes of errors (errno, NULL pointer), when we could have used a simple errno lik -ENXIO? And, of course, documentation for that function outlining what exactly the meaning of the individual error numbers is. But then we would need that anyway to clarify how the caller should interpret a 'NULL' return value. So: not convinced :-) Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:35 ` Hannes Reinecke @ 2021-07-26 10:03 ` Julia Lawall 0 siblings, 0 replies; 30+ messages in thread From: Julia Lawall @ 2021-07-26 10:03 UTC (permalink / raw) To: Hannes Reinecke Cc: Julia Lawall, Geert Uytterhoeven, Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit [-- Attachment #1: Type: text/plain, Size: 2641 bytes --] On Mon, 26 Jul 2021, Hannes Reinecke wrote: > On 7/26/21 11:28 AM, Julia Lawall wrote: > > > > > Also, over 230 files contain functions that return both NULL and > > > > > ERR_PTR. > > > > > A random example, chosen for conciseness, is the following from > > > > > fs/overlayfs/inode.c: > > > > > > > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry > > > > > *real, > > > > > bool is_upper) > > > > > { > > > > > struct inode *inode, *key = d_inode(real); > > > > > > > > > > inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, > > > > > key); > > > > > if (!inode) > > > > > return NULL; > > > > > > > > > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > > > > > is_upper ? real : NULL, false)) { > > > > > iput(inode); > > > > > return ERR_PTR(-ESTALE); > > > > > } > > > > > > > > > > return inode; > > > > > } > > > > > > > > > And that I would consider a coding error. > > > > If a function is able to return an error pointer it should _always_ > > > > return an error pointer; here it would be trivial to return -ENXIO > > > > instead of NULL in the first condition. > > > > > > > > Not doing so is just sloppy programming IMO. > > > > > > In this case I agree. > > > > I looked at the calling context, and it is not so obvious. There are > > different behaviors in the two cases at both callsites. It is like what > > Dan describes. If the thing is not available, we just move on. If it is > > available some action is needed. If there is an actual error, some error > > handling is needed. > > > But isn't 'not available' an error, too? No, it doesn't seem to be. > And isn't that exactly why we have individual errnos? > > Why do we have to introduce different classes of errors (errno, NULL pointer), > when we could have used a simple errno lik -ENXIO? > And, of course, documentation for that function outlining what exactly the > meaning of the individual error numbers is. > But then we would need that anyway to clarify how the caller should interpret > a 'NULL' return value. > > So: not convinced :-) OK, I basically agree. The point is just that there are going to be two tests at both call sites either way. julia > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg > HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:16 ` Geert Uytterhoeven 2021-07-26 9:28 ` Julia Lawall @ 2021-07-26 17:54 ` James Bottomley 2021-07-26 18:16 ` Linus Torvalds 2021-07-26 18:31 ` Laurent Pinchart 1 sibling, 2 replies; 30+ messages in thread From: James Bottomley @ 2021-07-26 17:54 UTC (permalink / raw) To: Geert Uytterhoeven, Hannes Reinecke Cc: Julia Lawall, Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit On Mon, 2021-07-26 at 11:16 +0200, Geert Uytterhoeven wrote: > Hi Hannes, > > On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke <hare@suse.de> > wrote: > > On 7/26/21 10:55 AM, Julia Lawall wrote: > > > On Mon, 26 Jul 2021, Arnd Bergmann wrote: > > > > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven < > > > > geert@linux-m68k.org> wrote: > > > > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> > > > > > wrote: > > > > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> > > > > > > wrote: > > > > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > > > > To make it work well, you need to know if frob() > > > > > > > > > and/or the current > > > > > > > > > function return an error code or not. While you can > > > > > > > > > use some heuristics > > > > > > > > > (e.g. is there any return -Exxx), perhaps we can add > > > > > > > > > an annotation to > > > > > > > > > indicate if a function returns an error code, or an > > > > > > > > > error pointer? > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > I think it would be useful, if not for the tools, at > > > > > > > > least for > > > > > > > > developers. > > > > > > > > > > > > > > Agreed. I added some code to smatch so that I could > > > > > > > annotate pointers to > > > > > > > say if they are allowed to be NULL. The implementation > > > > > > > isn't perfect, > > > > > > > but I love having that extra documentation about when I > > > > > > > do or don't have > > > > > > > to check for NULL. > > > > > > > > > > > > I can think of four different annotations that limit what a > > > > > > pointer return from > > > > > > a function can be: > > > > > > > > > > > > a) either a valid pointer or NULL, but never an error > > > > > > pointer, > > > > > > b) either a valid pointer or an error pointer, but not > > > > > > NULL, > > > > > > c) always a valid pointer, never NULL or an error, > > > > > > d) always NULL, but callers are expected to check for error > > > > > > pointers. > > > > > > > > > > e) either a valid pointer, NULL, or an error pointer > > > > > > > > > > The last pattern is seen with the various *get*_optional() > > > > > functions. > > > > > > > > I would always consider those the exact bug that I meant with > > > > "because > > > > everyone gets those wrong". I think the idea of the "optional" > > > > functions is > > > > that you have two implementations b) and d) and pick one of > > > > them > > > > at compile time. To the caller this means either an error > > > > pointer or > > > > success, but checking for NULL is a bug in the caller, while > > > > conditionally > > > > returning NULL or ERR_PTR() would be a bug in the interface. > > > > > > I'm not sure to understand the "bug in the caller" > > > part. Couldn't there > > > be two possible definitions of the called function, according to > > > different > > > configuration options, and a single caller that calls both? > > > > > > Also, over 230 files contain functions that return both NULL and > > > ERR_PTR. > > > A random example, chosen for conciseness, is the following from > > > fs/overlayfs/inode.c: > > > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct > > > dentry *real, > > > bool is_upper) > > > { > > > struct inode *inode, *key = d_inode(real); > > > > > > inode = ilookup5(sb, (unsigned long) key, > > > ovl_inode_test, key); > > > if (!inode) > > > return NULL; > > > > > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > > > is_upper ? real : NULL, false)) { > > > iput(inode); > > > return ERR_PTR(-ESTALE); > > > } > > > > > > return inode; > > > } > > > > > And that I would consider a coding error. > > If a function is able to return an error pointer it should _always_ > > return an error pointer; here it would be trivial to return -ENXIO > > instead of NULL in the first condition. > > > > Not doing so is just sloppy programming IMO. > > In this case I agree. Actually, I don't think so ... we have NULL return all over the inode and dentry code. It's a legitimate return for "I couldn't find what you asked for" or in the dentry case "I have no current entry". The error returns are usually an explicit "there was some problem during the lookup". James ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 17:54 ` James Bottomley @ 2021-07-26 18:16 ` Linus Torvalds 2021-07-26 21:53 ` NeilBrown 2021-07-26 18:31 ` Laurent Pinchart 1 sibling, 1 reply; 30+ messages in thread From: Linus Torvalds @ 2021-07-26 18:16 UTC (permalink / raw) To: James Bottomley Cc: Geert Uytterhoeven, Hannes Reinecke, Julia Lawall, Arnd Bergmann, NeilBrown, Laurent Pinchart, Dan Carpenter, ksummit On Mon, Jul 26, 2021 at 10:55 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > Actually, I don't think so ... we have NULL return all over the inode > and dentry code. It's a legitimate return for "I couldn't find what > you asked for" or in the dentry case "I have no current entry". The > error returns are usually an explicit "there was some problem during > the lookup". Absolutely. We have several cases of "valid pointer, NULL _or_ error pointer". Some of those things are not always about error pointers either. An example of this same kind of "three-way answer" is get_cached_acl(), which can return - an actual honest-to-goodness real posix_acl pointer - NULL for "there is no ACL data" - special value for "no cached data", need to ask the filesystem That particular case isn't using the error pointers, but it's very similar in nature to that "this is an explicit error" vs "don't know" kind of thing. Anyway, gcc has this completely ass-backwards "nonnull" function attribute, sadly it's pure garbage. It's not a "type of this pointer" thing, it's a "this function does not take NULL as argument X" thing. We could relatively easily add a "__nonnull" type attribute (and "__errptr") and parse it in sparse, but it would be _all_ over the place, and without real compiler support it's probably not worth it (people already don't run sparse all that much, and a lot of the things sparse can warn about are too verbose to be be used in practice). But in _theory_ it would be lovely to be able to say "this pointer cannot be NULL" or "this pointer can be an error pointer", but I suspect it really ends up being everywhere and more pain than gain. Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 18:16 ` Linus Torvalds @ 2021-07-26 21:53 ` NeilBrown 0 siblings, 0 replies; 30+ messages in thread From: NeilBrown @ 2021-07-26 21:53 UTC (permalink / raw) To: Linus Torvalds Cc: James Bottomley, Geert Uytterhoeven, Hannes Reinecke, Julia Lawall, Arnd Bergmann, Laurent Pinchart, Dan Carpenter, ksummit On Tue, 27 Jul 2021, Linus Torvalds wrote: > > Anyway, gcc has this completely ass-backwards "nonnull" function > attribute, sadly it's pure garbage. It's not a "type of this pointer" > thing, it's a "this function does not take NULL as argument X" thing. Yeah, I found that annoying too. It seems to be purely about optimization. Marking a function parameter as "nonnull" means the compiler can optimize away any tests against NULL. > > We could relatively easily add a "__nonnull" type attribute (and > "__errptr") and parse it in sparse, but it would be _all_ over the sparse already parses __attribute__((safe)) which seems to mean something like __nonnull. It needs a bit of work to make it useful. > place, and without real compiler support it's probably not worth it > (people already don't run sparse all that much, and a lot of the > things sparse can warn about are too verbose to be be used in > practice). I'm not convinced that "real compiler support" is essential. checkpatch.pl does provide some value despite not being "real compiler support" and not being mandated. NeilBrown ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 17:54 ` James Bottomley 2021-07-26 18:16 ` Linus Torvalds @ 2021-07-26 18:31 ` Laurent Pinchart 1 sibling, 0 replies; 30+ messages in thread From: Laurent Pinchart @ 2021-07-26 18:31 UTC (permalink / raw) To: James Bottomley Cc: Geert Uytterhoeven, Hannes Reinecke, Julia Lawall, Arnd Bergmann, NeilBrown, Dan Carpenter, ksummit Hi James, On Mon, Jul 26, 2021 at 10:54:59AM -0700, James Bottomley wrote: > On Mon, 2021-07-26 at 11:16 +0200, Geert Uytterhoeven wrote: > > On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke wrote: > > > On 7/26/21 10:55 AM, Julia Lawall wrote: > > > > On Mon, 26 Jul 2021, Arnd Bergmann wrote: > > > > > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven wrote: > > > > > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann wrote: > > > > > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown wrote: > > > > > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > > > > > To make it work well, you need to know if frob() and/or the current > > > > > > > > > > function return an error code or not. While you can use some heuristics > > > > > > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > > > > > > > indicate if a function returns an error code, or an error pointer? > > > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > > > I think it would be useful, if not for the tools, at least for > > > > > > > > > developers. > > > > > > > > > > > > > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > > > > > > > say if they are allowed to be NULL. The implementation isn't perfect, > > > > > > > > but I love having that extra documentation about when I do or don't have > > > > > > > > to check for NULL. > > > > > > > > > > > > > > I can think of four different annotations that limit what a pointer return from > > > > > > > a function can be: > > > > > > > > > > > > > > a) either a valid pointer or NULL, but never an error pointer, > > > > > > > b) either a valid pointer or an error pointer, but not NULL, > > > > > > > c) always a valid pointer, never NULL or an error, > > > > > > > d) always NULL, but callers are expected to check for error > > > > > > > pointers. > > > > > > > > > > > > e) either a valid pointer, NULL, or an error pointer > > > > > > > > > > > > The last pattern is seen with the various *get*_optional() > > > > > > functions. > > > > > > > > > > I would always consider those the exact bug that I meant with "because > > > > > everyone gets those wrong". I think the idea of the "optional" functions is > > > > > that you have two implementations b) and d) and pick one of them > > > > > at compile time. To the caller this means either an error pointer or > > > > > success, but checking for NULL is a bug in the caller, while conditionally > > > > > returning NULL or ERR_PTR() would be a bug in the interface. > > > > > > > > I'm not sure to understand the "bug in the caller" part. Couldn't there > > > > be two possible definitions of the called function, according to different > > > > configuration options, and a single caller that calls both? > > > > > > > > Also, over 230 files contain functions that return both NULL and ERR_PTR. > > > > A random example, chosen for conciseness, is the following from > > > > fs/overlayfs/inode.c: > > > > > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > > > > bool is_upper) > > > > { > > > > struct inode *inode, *key = d_inode(real); > > > > > > > > inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key); > > > > if (!inode) > > > > return NULL; > > > > > > > > if (!ovl_verify_inode(inode, is_upper ? NULL : real, > > > > is_upper ? real : NULL, false)) { > > > > iput(inode); > > > > return ERR_PTR(-ESTALE); > > > > } > > > > > > > > return inode; > > > > } > > > > > > > And that I would consider a coding error. > > > If a function is able to return an error pointer it should _always_ > > > return an error pointer; here it would be trivial to return -ENXIO > > > instead of NULL in the first condition. > > > > > > Not doing so is just sloppy programming IMO. > > > > In this case I agree. > > Actually, I don't think so ... we have NULL return all over the inode > and dentry code. It's a legitimate return for "I couldn't find what > you asked for" or in the dentry case "I have no current entry". The > error returns are usually an explicit "there was some problem during > the lookup". It's a matter of semantics. From a technical point of view, both NULL or a particular error code can mean "not found". What has been bothering me for a long time, and keeps doing so, is the lack of standardization in the semantics. Even within a subsystem, different semantics can be used, and that's the source of bugs and overall pain for developers. I'm not sure if complete standardization at the kernel level is possible, but any step we can take in that direction would I believe be an improvement. At the very least, we need a way for developers to easily figure out what semantics a given function uses. Having to read the source code, sometimes diving deep in call stacks, to find if a function can return NULL, is too painful. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 9:08 ` Hannes Reinecke 2021-07-26 9:16 ` Geert Uytterhoeven @ 2021-07-26 9:17 ` Dan Carpenter 1 sibling, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2021-07-26 9:17 UTC (permalink / raw) To: Hannes Reinecke Cc: Julia Lawall, Arnd Bergmann, Geert Uytterhoeven, NeilBrown, Laurent Pinchart, ksummit On Mon, Jul 26, 2021 at 11:08:01AM +0200, Hannes Reinecke wrote: > And that I would consider a coding error. > If a function is able to return an error pointer it should _always_ return > an error pointer; here it would be trivial to return -ENXIO instead of NULL > in the first condition. > > Not doing so is just sloppy programming IMO. > > Cheers, But it's not an error. One place where this bit me recently was I was trying to debug some code and I added: if (err) printk("function blah returned err"); But their list iterator returns an error code when it got to the end of the list so I was flooded with errors. It be nicer "if we can't allocate enough memory for the next item return an error pointer. If we come to the end of the loop then return NULL" regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 8:20 ` Arnd Bergmann 2021-07-26 8:39 ` Geert Uytterhoeven 2021-07-26 8:55 ` Julia Lawall @ 2021-07-26 9:13 ` Dan Carpenter 2 siblings, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2021-07-26 9:13 UTC (permalink / raw) To: Arnd Bergmann Cc: Geert Uytterhoeven, NeilBrown, Laurent Pinchart, ksummit, Julia Lawall On Mon, Jul 26, 2021 at 10:20:45AM +0200, Arnd Bergmann wrote: > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > To make it work well, you need to know if frob() and/or the current > > > > > > function return an error code or not. While you can use some heuristics > > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > > > indicate if a function returns an error code, or an error pointer? > > > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > > > > > I think it would be useful, if not for the tools, at least for > > > > > developers. > > > > > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > > > say if they are allowed to be NULL. The implementation isn't perfect, > > > > but I love having that extra documentation about when I do or don't have > > > > to check for NULL. > > > > > > I can think of four different annotations that limit what a pointer return from > > > a function can be: > > > > > > a) either a valid pointer or NULL, but never an error pointer, > > > b) either a valid pointer or an error pointer, but not NULL, > > > c) always a valid pointer, never NULL or an error, > > > d) always NULL, but callers are expected to check for error pointers. > > > > e) either a valid pointer, NULL, or an error pointer > > > > The last pattern is seen with the various *get*_optional() functions. > > I would always consider those the exact bug that I meant with "because > everyone gets those wrong". I think the idea of the "optional" functions is > that you have two implementations b) and d) and pick one of them > at compile time. To the caller this means either an error pointer or > success, but checking for NULL is a bug in the caller, while conditionally > returning NULL or ERR_PTR() would be a bug in the interface. > > Arnd When a function returns both error pointers and NULL then the NULL just means the feature is diliberately disabled. So don't print an error, but NULL returns still have to be handled. The other mistake I see with this is when the function returns an error pointer the caller just says this is optional so let's continue. p = get_optional(); if (IS_ERR(p)) p = NULL; It might be helpful if it lets the system boot but generally errors should be reported to the user. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 7:25 ` Arnd Bergmann 2021-07-26 7:53 ` Geert Uytterhoeven @ 2021-07-26 21:43 ` NeilBrown 1 sibling, 0 replies; 30+ messages in thread From: NeilBrown @ 2021-07-26 21:43 UTC (permalink / raw) To: Arnd Bergmann Cc: Laurent Pinchart, Geert Uytterhoeven, Dan Carpenter, ksummit, Julia Lawall On Mon, 26 Jul 2021, Arnd Bergmann wrote: > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote: > > On Sun, 25 Jul 2021, Laurent Pinchart wrote: > > > > > > > > To make it work well, you need to know if frob() and/or the current > > > > function return an error code or not. While you can use some heuristics > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to > > > > indicate if a function returns an error code, or an error pointer? > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/ > > > > > > I think it would be useful, if not for the tools, at least for > > > developers. > > > > Agreed. I added some code to smatch so that I could annotate pointers to > > say if they are allowed to be NULL. The implementation isn't perfect, > > but I love having that extra documentation about when I do or don't have > > to check for NULL. > > I can think of four different annotations that limit what a pointer return from > a function can be: > > a) either a valid pointer or NULL, but never an error pointer, > b) either a valid pointer or an error pointer, but not NULL, > c) always a valid pointer, never NULL or an error, > d) always NULL, but callers are expected to check for error pointers. > > The last one is the tricky bit because there are a couple of subsystems > that intentionally return an opaque NULL pointer to indicate success > when the subsystem is compile-time disabled, and expect callers to not > dereference that pointer but instead pass it into other stubbed-out > interfaces. I think this one is rather specific to the kernel, but also really > useful to have in a static checker because everyone gets those wrong. And now we see the worms wriggling their way out of the can.... Some pointers can also have the 'lsb' overloaded, sometimes as a bit-spin-lock. And pointers stored in a structure can have temporal type requirements - different at various places in the life cycle. Most obviously, "non-NULL" pointers can be NULL (or unitiallised) during inititalization, and may have other inconsistencies during finalization. But it isn't hard to find more complexity. "context" structures are particularly suseptible to this. I've been looking at 'struct fs_context' recently. Normally you would use one of the allocation functions which will always set ->ops and a few other fields. So maybe ->ops is "always valid". Then you initialize some extra bits. Maybe call vfs_parse_fs_string() etc. Finally you call vfs_create_mount(). But you *can* call vfs_create_mount() after only setting ->root ->sb_flags and ->source. ->ops can be uninitialised! So what annotations would you give to the various fields? To capture this completely you would need a series of sub-types of 'fs_context', each with different guarantees or requirements about the values of different fields. I love contemplating what the "perfect" system would be, but I doubt a near-perfect system would be much fun to program in, and that last 20% to get to perfection would take a while. So if we wanted to do something like this, it would need to be completely opt-in. The default would be current behaviour, and we would want to have annotations for both what is, and what isn't, promised. "what is" would be something like "this pointer is never NULL". "what isn't" would be more like __must_check. "this pointer might be NULL and you really have to check before you dereference it". For an unannotated pointer, it is not an error to test if it is NULL, or to dereference without testing. And, of course, we would need to be able to cast between the different promises. NeilBrown ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-24 13:33 ` Geert Uytterhoeven ` (2 preceding siblings ...) 2021-07-24 23:18 ` Laurent Pinchart @ 2021-07-26 7:05 ` Dan Carpenter 3 siblings, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2021-07-26 7:05 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: ksummit, Julia Lawall On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote: > > Here is another example of something which I have a check for but I > > haven't pushed. > > > > ret = frob(); > > if (!ret) > > return ret; > > > > This code is normally correct but sometimes it means the if statement is > > reversed and it should be "if (ret) return ret;". To me returning a > > literal is always more clear but but clearly the original author felt > > "ret" was more clear... It's only a few bugs per year so it's not a big > > deal either way. > > To make it work well, you need to know if frob() and/or the current > function return an error code or not. While you can use some heuristics > (e.g. is there any return -Exxx), perhaps we can add an annotation to > indicate if a function returns an error code, or an error pointer? The function is normal return zero or error codes, but the question is do we want to return for success or for failure. This kind of bug is normally caught very early in testing but some of the big refactor patches can't be tested on every hardware. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-23 19:10 Potential static analysis ideas Dan Carpenter 2021-07-24 13:33 ` Geert Uytterhoeven @ 2021-07-26 15:50 ` Paul E. McKenney 2021-07-27 9:38 ` Dan Carpenter 1 sibling, 1 reply; 30+ messages in thread From: Paul E. McKenney @ 2021-07-26 15:50 UTC (permalink / raw) To: Dan Carpenter; +Cc: ksummit, Julia Lawall On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote: > Rust has many good static analysis features but if we wanted we could > implement a number of stricter rules in C. With Smatch I have tried to > focus on exclusively on finding bugs because everyone can agree that > bugs are bad. But if some subsystems wanted to implement stricter rules > just as a hardenning measure then that's a doable thing. > > For example, I've tried a bunch of approaches to warning about when the > user can trigger an integer overflow. The challenge is that most > integer overflows are harmless and do not cause a real life bug. I would not want overflow checks for unsigned integers, but it might be helpful for signed integers. But yes, most of us rely on fwrapv, so that kernelwide checks for signed integer overflow will be quite noisy. Thanx, Paul ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-26 15:50 ` Paul E. McKenney @ 2021-07-27 9:38 ` Dan Carpenter 2021-07-27 9:50 ` Geert Uytterhoeven 2021-07-27 16:06 ` Paul E. McKenney 0 siblings, 2 replies; 30+ messages in thread From: Dan Carpenter @ 2021-07-27 9:38 UTC (permalink / raw) To: Paul E. McKenney; +Cc: ksummit, Julia Lawall On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote: > On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote: > > Rust has many good static analysis features but if we wanted we could > > implement a number of stricter rules in C. With Smatch I have tried to > > focus on exclusively on finding bugs because everyone can agree that > > bugs are bad. But if some subsystems wanted to implement stricter rules > > just as a hardenning measure then that's a doable thing. > > > > For example, I've tried a bunch of approaches to warning about when the > > user can trigger an integer overflow. The challenge is that most > > integer overflows are harmless and do not cause a real life bug. > > I would not want overflow checks for unsigned integers, but it might > be helpful for signed integers. But yes, most of us rely on fwrapv, > so that kernelwide checks for signed integer overflow will be quite noisy. Since we use -fwrapv then even signed integer overflows are defined and I haven't seen a way that checking for signed integer overflows can be useful. With integer overflows I'm more talking about integer overflows from the user. And I imagine a subsystem specific thing as a kind of "We want extra security but aren't ready to port everything to Rust" type option. I have almost 2 thousand of these warnings. This first example is from the ioctl and probably root only. Plus commit 6d13de1489b6 ("uaccess: disallow > INT_MAX copy sizes") really improved security. drivers/fpga/dfl-fme-pr.c 83 if (copy_from_user(&port_pr, argp, minsz)) 84 return -EFAULT; 85 86 if (port_pr.argsz < minsz || port_pr.flags) 87 return -EINVAL; 88 89 /* get fme header region */ 90 fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev, 91 FME_FEATURE_ID_HEADER); 92 93 /* check port id */ 94 v = readq(fme_hdr + FME_HDR_CAP); 95 if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) { 96 dev_dbg(&pdev->dev, "port number more than maximum\n"); 97 return -EINVAL; 98 } 99 100 /* 101 * align PR buffer per PR bandwidth, as HW ignores the extra padding 102 * data automatically. 103 */ 104 length = ALIGN(port_pr.buffer_size, 4); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This ALIGN() operation can overflow but only to zero. 105 106 buf = vmalloc(length); kmalloc(() allows zero size allocations but vmalloc() will return NULL. And actually, in April, Nicholas Piggin made it trigger a WARN_ONCE(). 107 if (!buf) 108 return -ENOMEM; 109 110 if (copy_from_user(buf, 111 (void __user *)(unsigned long)port_pr.buffer_address, 112 port_pr.buffer_size)) { ^^^^^^^^^^^^^^^^^^^ So this can't corrupt memory for the reasons given above. (It's still buggy because it doesn't zero out the last three bytes between port_pr.buffer_size and length, but that's a different issue). regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-27 9:38 ` Dan Carpenter @ 2021-07-27 9:50 ` Geert Uytterhoeven 2021-07-27 16:06 ` Paul E. McKenney 1 sibling, 0 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2021-07-27 9:50 UTC (permalink / raw) To: Dan Carpenter; +Cc: Paul E. McKenney, ksummit, Julia Lawall On Tue, Jul 27, 2021 at 11:38 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote: > > On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote: > > > Rust has many good static analysis features but if we wanted we could > > > implement a number of stricter rules in C. With Smatch I have tried to > > > focus on exclusively on finding bugs because everyone can agree that > > > bugs are bad. But if some subsystems wanted to implement stricter rules > > > just as a hardenning measure then that's a doable thing. > > > > > > For example, I've tried a bunch of approaches to warning about when the > > > user can trigger an integer overflow. The challenge is that most > > > integer overflows are harmless and do not cause a real life bug. > > > > I would not want overflow checks for unsigned integers, but it might > > be helpful for signed integers. But yes, most of us rely on fwrapv, > > so that kernelwide checks for signed integer overflow will be quite noisy. > > Since we use -fwrapv then even signed integer overflows are defined and > I haven't seen a way that checking for signed integer overflows can be > useful. For all people using git grep and investigating: we don't pass -fwrapv explicitly, it is implied by -fno-strict-overflow. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Potential static analysis ideas 2021-07-27 9:38 ` Dan Carpenter 2021-07-27 9:50 ` Geert Uytterhoeven @ 2021-07-27 16:06 ` Paul E. McKenney 1 sibling, 0 replies; 30+ messages in thread From: Paul E. McKenney @ 2021-07-27 16:06 UTC (permalink / raw) To: Dan Carpenter; +Cc: ksummit, Julia Lawall On Tue, Jul 27, 2021 at 12:38:08PM +0300, Dan Carpenter wrote: > On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote: > > On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote: > > > Rust has many good static analysis features but if we wanted we could > > > implement a number of stricter rules in C. With Smatch I have tried to > > > focus on exclusively on finding bugs because everyone can agree that > > > bugs are bad. But if some subsystems wanted to implement stricter rules > > > just as a hardenning measure then that's a doable thing. > > > > > > For example, I've tried a bunch of approaches to warning about when the > > > user can trigger an integer overflow. The challenge is that most > > > integer overflows are harmless and do not cause a real life bug. > > > > I would not want overflow checks for unsigned integers, but it might > > be helpful for signed integers. But yes, most of us rely on fwrapv, > > so that kernelwide checks for signed integer overflow will be quite noisy. > > Since we use -fwrapv then even signed integer overflows are defined and > I haven't seen a way that checking for signed integer overflows can be > useful. Just because the compiler defines something does not mean that it cannot be involved in a bug. ;-) > With integer overflows I'm more talking about integer overflows from the > user. And I imagine a subsystem specific thing as a kind of "We want > extra security but aren't ready to port everything to Rust" type option. Which was what I was also imagining, but along different lines. But I agree that what you are proposing might be useful. Thanx, Paul > I have almost 2 thousand of these warnings. This first example is from > the ioctl and probably root only. Plus commit 6d13de1489b6 ("uaccess: > disallow > INT_MAX copy sizes") really improved security. > > drivers/fpga/dfl-fme-pr.c > 83 if (copy_from_user(&port_pr, argp, minsz)) > 84 return -EFAULT; > 85 > 86 if (port_pr.argsz < minsz || port_pr.flags) > 87 return -EINVAL; > 88 > 89 /* get fme header region */ > 90 fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev, > 91 FME_FEATURE_ID_HEADER); > 92 > 93 /* check port id */ > 94 v = readq(fme_hdr + FME_HDR_CAP); > 95 if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) { > 96 dev_dbg(&pdev->dev, "port number more than maximum\n"); > 97 return -EINVAL; > 98 } > 99 > 100 /* > 101 * align PR buffer per PR bandwidth, as HW ignores the extra padding > 102 * data automatically. > 103 */ > 104 length = ALIGN(port_pr.buffer_size, 4); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This ALIGN() operation can overflow but only to zero. > > 105 > 106 buf = vmalloc(length); > > kmalloc(() allows zero size allocations but vmalloc() will return NULL. > And actually, in April, Nicholas Piggin made it trigger a WARN_ONCE(). > > 107 if (!buf) > 108 return -ENOMEM; > 109 > 110 if (copy_from_user(buf, > 111 (void __user *)(unsigned long)port_pr.buffer_address, > 112 port_pr.buffer_size)) { > ^^^^^^^^^^^^^^^^^^^ > So this can't corrupt memory for the reasons given above. > > (It's still buggy because it doesn't zero out the last three bytes > between port_pr.buffer_size and length, but that's a different issue). > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-07-27 16:06 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 19:10 Potential static analysis ideas Dan Carpenter 2021-07-24 13:33 ` Geert Uytterhoeven 2021-07-24 13:40 ` Julia Lawall 2021-07-24 14:08 ` Arnd Bergmann 2021-07-24 23:18 ` Laurent Pinchart 2021-07-24 23:45 ` NeilBrown 2021-07-26 7:25 ` Arnd Bergmann 2021-07-26 7:53 ` Geert Uytterhoeven 2021-07-26 8:20 ` Arnd Bergmann 2021-07-26 8:39 ` Geert Uytterhoeven 2021-07-26 8:52 ` Arnd Bergmann 2021-07-26 9:11 ` Geert Uytterhoeven 2021-07-26 8:55 ` Julia Lawall 2021-07-26 9:08 ` Hannes Reinecke 2021-07-26 9:16 ` Geert Uytterhoeven 2021-07-26 9:28 ` Julia Lawall 2021-07-26 9:35 ` Hannes Reinecke 2021-07-26 10:03 ` Julia Lawall 2021-07-26 17:54 ` James Bottomley 2021-07-26 18:16 ` Linus Torvalds 2021-07-26 21:53 ` NeilBrown 2021-07-26 18:31 ` Laurent Pinchart 2021-07-26 9:17 ` Dan Carpenter 2021-07-26 9:13 ` Dan Carpenter 2021-07-26 21:43 ` NeilBrown 2021-07-26 7:05 ` Dan Carpenter 2021-07-26 15:50 ` Paul E. McKenney 2021-07-27 9:38 ` Dan Carpenter 2021-07-27 9:50 ` Geert Uytterhoeven 2021-07-27 16:06 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).