Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 7 Sep 2016 10:17:06 -0700 | Subject | Re: [PATCH] usercopy: remove page-spanning test for now |
| |
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).
Linus
| |