* [PATCH] bug: Switch data corruption check to __must_check
@ 2017-02-06 20:45 Kees Cook
2017-02-06 21:04 ` Arnd Bergmann
2017-02-07 20:39 ` Paul E. McKenney
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2017-02-06 20:45 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Arnd Bergmann, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
edumazet, dvhart, fweisbec, oleg, bobby.prani
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(({ \
+ bool corruption = unlikely(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;
return true;
}
@@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry)
prev = entry->prev;
next = entry->next;
- CHECK_DATA_CORRUPTION(next == LIST_POISON1,
- "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
- entry, LIST_POISON1);
- CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
- "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
- entry, LIST_POISON2);
- CHECK_DATA_CORRUPTION(prev->next != entry,
- "list_del corruption. prev->next should be %p, but was %p\n",
- entry, prev->next);
- CHECK_DATA_CORRUPTION(next->prev != entry,
- "list_del corruption. next->prev should be %p, but was %p\n",
- entry, next->prev);
+ if (CHECK_DATA_CORRUPTION(next == LIST_POISON1,
+ "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
+ entry, LIST_POISON1) ||
+ CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
+ "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
+ entry, LIST_POISON2) ||
+ CHECK_DATA_CORRUPTION(prev->next != entry,
+ "list_del corruption. prev->next should be %p, but was %p\n",
+ entry, prev->next) ||
+ CHECK_DATA_CORRUPTION(next->prev != entry,
+ "list_del corruption. next->prev should be %p, but was %p\n",
+ entry, next->prev))
+ return false;
+
return true;
}
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bug: Switch data corruption check to __must_check
2017-02-06 20:45 [PATCH] bug: Switch data corruption check to __must_check Kees Cook
@ 2017-02-06 21:04 ` Arnd Bergmann
2017-02-06 21:18 ` Kees Cook
2017-02-07 20:39 ` Paul E. McKenney
1 sibling, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-06 21:04 UTC (permalink / raw)
To: Kees Cook
Cc: Paul E. McKenney, Linux Kernel Mailing List, mingo, jiangshanlai,
dipankar, Andrew Morton, mathieu.desnoyers, Josh Triplett, tglx,
peterz, Steven Rostedt, David Howells, edumazet, dvhart,
fweisbec, oleg, bobby.prani
On Mon, Feb 6, 2017 at 9:45 PM, Kees Cook <keescook@chromium.org> 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>
The patch looks ok, but I have no memory of suggesting this. Was this an older
conversation we had that I already forgot, or did you confuse me with someone
else?
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bug: Switch data corruption check to __must_check
2017-02-06 21:04 ` Arnd Bergmann
@ 2017-02-06 21:18 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-02-06 21:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Paul E. McKenney, Linux Kernel Mailing List, Ingo Molnar,
Lai Jiangshan, dipankar, Andrew Morton, Mathieu Desnoyers,
Josh Triplett, Thomas Gleixner, Peter Zijlstra, Steven Rostedt,
David Howells, Eric Dumazet, Darren Hart, Frederic Weisbecker,
Oleg Nesterov, pranith kumar
On Mon, Feb 6, 2017 at 1:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Feb 6, 2017 at 9:45 PM, Kees Cook <keescook@chromium.org> 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>
>
> The patch looks ok, but I have no memory of suggesting this. Was this an older
> conversation we had that I already forgot, or did you confuse me with someone
> else?
Old conversation that we had. :)
https://patchwork.kernel.org/patch/9286627/
It just took me *cough* 6 months ... :P
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bug: Switch data corruption check to __must_check
2017-02-06 20:45 [PATCH] bug: Switch data corruption check to __must_check Kees Cook
2017-02-06 21:04 ` Arnd Bergmann
@ 2017-02-07 20:39 ` Paul E. McKenney
2017-02-07 20:57 ` Kees Cook
1 sibling, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2017-02-07 20:39 UTC (permalink / raw)
To: Kees Cook
Cc: Arnd Bergmann, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
edumazet, dvhart, fweisbec, oleg, bobby.prani
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.
> + bool corruption = unlikely(condition); \
So corruption = unlikely(condition)? Sounds a bit optimistic to me! ;-)
> + 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?
Still it is nice to avoid the magic return from out of the middle of the
C-preprocessor macro.
Thanx, Paul
> return true;
> }
> @@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry)
> prev = entry->prev;
> next = entry->next;
>
> - CHECK_DATA_CORRUPTION(next == LIST_POISON1,
> - "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
> - entry, LIST_POISON1);
> - CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
> - "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
> - entry, LIST_POISON2);
> - CHECK_DATA_CORRUPTION(prev->next != entry,
> - "list_del corruption. prev->next should be %p, but was %p\n",
> - entry, prev->next);
> - CHECK_DATA_CORRUPTION(next->prev != entry,
> - "list_del corruption. next->prev should be %p, but was %p\n",
> - entry, next->prev);
> + if (CHECK_DATA_CORRUPTION(next == LIST_POISON1,
> + "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
> + entry, LIST_POISON1) ||
> + CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
> + "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
> + entry, LIST_POISON2) ||
> + CHECK_DATA_CORRUPTION(prev->next != entry,
> + "list_del corruption. prev->next should be %p, but was %p\n",
> + entry, prev->next) ||
> + CHECK_DATA_CORRUPTION(next->prev != entry,
> + "list_del corruption. next->prev should be %p, but was %p\n",
> + entry, next->prev))
> + return false;
> +
> return true;
>
> }
> --
> 2.7.4
>
>
> --
> Kees Cook
> Pixel Security
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bug: Switch data corruption check to __must_check
2017-02-07 20:39 ` Paul E. McKenney
@ 2017-02-07 20:57 ` Kees Cook
2017-02-07 21:08 ` Paul E. McKenney
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-02-07 20:57 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Arnd Bergmann, LKML, Ingo Molnar, Lai Jiangshan, dipankar,
Andrew Morton, Mathieu Desnoyers, Josh Triplett, Thomas Gleixner,
Peter Zijlstra, Steven Rostedt, David Howells, Eric Dumazet,
Darren Hart, Frederic Weisbecker, Oleg Nesterov, pranith kumar
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.
>> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bug: Switch data corruption check to __must_check
2017-02-07 20:57 ` Kees Cook
@ 2017-02-07 21:08 ` Paul E. McKenney
0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2017-02-07 21:08 UTC (permalink / raw)
To: Kees Cook
Cc: Arnd Bergmann, LKML, Ingo Molnar, Lai Jiangshan, dipankar,
Andrew Morton, Mathieu Desnoyers, Josh Triplett, Thomas Gleixner,
Peter Zijlstra, Steven Rostedt, David Howells, Eric Dumazet,
Darren Hart, Frederic Weisbecker, Oleg Nesterov, pranith kumar
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-07 21:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-06 20:45 [PATCH] bug: Switch data corruption check to __must_check Kees Cook
2017-02-06 21:04 ` Arnd Bergmann
2017-02-06 21:18 ` Kees Cook
2017-02-07 20:39 ` Paul E. McKenney
2017-02-07 20:57 ` Kees Cook
2017-02-07 21:08 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).