Messages in this thread | | | From | Vegard Nossum <> | Date | Wed, 7 Sep 2016 22:32:42 +0200 | Subject | Re: [PATCH] usercopy: remove page-spanning test for now |
| |
On 7 September 2016 at 19:17, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@chromium.org> wrote: >> >> +#ifndef CONFIG_HARDENED_USERCOPY_PAGESPAN >> + /* >> + * The page-spanning checks are hitting false positives, so >> + * do not check them for now. >> + */ >> + return NULL; >> +#endif >> + >> /* Allow kernel data region (if not marked as Reserved). */ >> if (ptr >= (const void *)_sdata && end <= (const void *)_edata) >> return NULL; > > No. Don't do patches like this. > > It's wrong for two reasons: > > (a) if you want to use an #ifdef to disable code, do so. Enclose the > code you want to disable with the #ifdef, not some *other* code that > then indirectly disables the code you want to disable. > > (b) don't do "surprising" things with control flow. It can cause > compiler warnings in reasonable compilers ("unreachable code"), but > it's also a strange pattern that throws people. > > So really, make the patch bigger but more legible. In fact, I think > the best option would be to simply turn the code you want to disable > into a helper function of its own, and then make the #ifdef enable or > disable the whole function. > > (That also solves the problems like having the declaration for > "endpage" and the other variables that is only used by the disabled > code be *with* the disabled code, so that you don't have to have > multiple ifdef'ed regions. I suspect avoiding that is a large reason > why you did the hacky thing in the first place).
For this particular case, one might also use something like
if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY_PAGESPAN)) return;
no ifdefs, the "early return" is a common pattern (and readable IMHO), and no unused variable warnings or separate ifdef blocks for variables and I *think* no unreachable code warnings.
Or? </bikeshed>
Vegard
| |