lkml.org 
[lkml]   [2016]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.045 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site