Messages in this thread | | | Date | Tue, 7 Feb 2017 13:08:18 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] bug: Switch data corruption check to __must_check |
| |
On Tue, Feb 07, 2017 at 12:57:33PM -0800, Kees Cook wrote: > On Tue, Feb 7, 2017 at 12:39 PM, Paul E. McKenney > <paulmck@linux.vnet.ibm.com> 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 <arnd@arndb.de> > >> Signed-off-by: Kees Cook <keescook@chromium.org> > >> --- > >> 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.
Ah, I see it now. Color me blind!
Thanx, Paul
> >> + 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 >
| |