On Tue, Feb 7, 2017 at 12:39 PM, Paul E. McKenney
<[email protected]> wrote:
> On Mon, Feb 06, 2017 at 12:45:47PM -0800, Kees Cook wrote:
>> The CHECK_DATA_CORRUPTION() macro was designed to have callers do
>> something meaningful/protective on failure. However, using "return false"
>> in the macro too strictly limits the design patterns of callers. Instead,
>> let callers handle the logic test directly, but make sure that the result
>> IS checked by forcing __must_check (which appears to not be able to be
>> used directly on macro expressions).
>>
>> Suggested-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> include/linux/bug.h | 12 +++++++-----
>> lib/list_debug.c | 45 ++++++++++++++++++++++++---------------------
>> 2 files changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>> index baff2e8fc8a8..5828489309bb 100644
>> --- a/include/linux/bug.h
>> +++ b/include/linux/bug.h
>> @@ -124,18 +124,20 @@ static inline enum bug_trap_type report_bug(unsigned
>> long bug_addr,
>>
>> /*
>> * Since detected data corruption should stop operation on the affected
>> - * structures, this returns false if the corruption condition is found.
>> + * structures. Return value must be checked and sanely acted on by caller.
>> */
>> +static inline __must_check bool check_data_corruption(bool v) { return v; }
>> #define CHECK_DATA_CORRUPTION(condition, fmt, ...) \
>> - do { \
>> - if (unlikely(condition)) { \
>> + check_data_corruption(({ \
>
> The definition of check_data_corruption() is in some other patch? I don't
> see it in current mainline. I am not seeing what it might be doing.
It's immediately before the #define line above. It's nothing more than
an inline argument pass-through, but since it's a _function_ I can
attach __must_check to it, which I can't do for a conditional
expression macro. And I gave it the meaningful name so when someone
fails to check CHECK_DATA_CORRUPTION, they'll get a gcc warning about
"check_data_corruption" which will lead them here.
>> + bool corruption = unlikely(condition); \
>
> So corruption = unlikely(condition)? Sounds a bit optimistic to me! ;-)
It's true though! :) Nearly all calls to CHECK_DATA_CORRUPTION()
should end up with a false condition.
>
>> + if (corruption) { \
>> if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
>> pr_err(fmt, ##__VA_ARGS__); \
>> BUG(); \
>> } else \
>> WARN(1, fmt, ##__VA_ARGS__); \
>> - return false; \
>> } \
>> - } while (0)
>> + corruption; \
>> + }))
>>
>> #endif /* _LINUX_BUG_H */
>> diff --git a/lib/list_debug.c b/lib/list_debug.c
>> index 7f7bfa55eb6d..a34db8d27667 100644
>> --- a/lib/list_debug.c
>> +++ b/lib/list_debug.c
>> @@ -20,15 +20,16 @@
>> bool __list_add_valid(struct list_head *new, struct list_head *prev,
>> struct list_head *next)
>> {
>> - CHECK_DATA_CORRUPTION(next->prev != prev,
>> - "list_add corruption. next->prev should be prev (%p), but was
>> %p. (next=%p).\n",
>> - prev, next->prev, next);
>> - CHECK_DATA_CORRUPTION(prev->next != next,
>> - "list_add corruption. prev->next should be next (%p), but was
>> %p. (prev=%p).\n",
>> - next, prev->next, prev);
>> - CHECK_DATA_CORRUPTION(new == prev || new == next,
>> - "list_add double add: new=%p, prev=%p, next=%p.\n",
>> - new, prev, next);
>> + if (CHECK_DATA_CORRUPTION(next->prev != prev,
>> + "list_add corruption. next->prev should be prev (%p),
>> but was %p. (next=%p).\n",
>> + prev, next->prev, next) ||
>> + CHECK_DATA_CORRUPTION(prev->next != next,
>> + "list_add corruption. prev->next should be next (%p),
>> but was %p. (prev=%p).\n",
>> + next, prev->next, prev) ||
>> + CHECK_DATA_CORRUPTION(new == prev || new == next,
>> + "list_add double add: new=%p, prev=%p, next=%p.\n",
>> + new, prev, next))
>> + return false;
>
> That -is- one ornate "if" condition, isn't it?
It is, yes. :)
> Still it is nice to avoid the magic return from out of the middle of the
> C-preprocessor macro.
Agreed. I had fun with indenting to make it passably readable. :P
-Kees
--
Kees Cook
Pixel Security
