Re: [PATCH] usercopy: remove page-spanning test for now

[Vegard Nossum]

On 7 September 2016 at 19:17, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@xxxxxxxxxxxx> 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