Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1575125 > unrolled thread

[PATCH] bug: Switch data corruption check to __must_check

Started byKees Cook <keescook@chromium.org>
First post2017-02-06 21:50 +0100
Last post2017-02-07 22:40 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] bug: Switch data corruption check to __must_check Kees Cook <keescook@chromium.org> - 2017-02-06 21:50 +0100
    Re: [PATCH] bug: Switch data corruption check to __must_check Arnd Bergmann <arnd@arndb.de> - 2017-02-06 22:10 +0100
      Re: [PATCH] bug: Switch data corruption check to __must_check Kees Cook <keescook@chromium.org> - 2017-02-06 22:20 +0100
    Re: [PATCH] bug: Switch data corruption check to __must_check "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-02-07 21:50 +0100
      Re: [PATCH] bug: Switch data corruption check to __must_check Kees Cook <keescook@chromium.org> - 2017-02-07 22:00 +0100
        Re: [PATCH] bug: Switch data corruption check to __must_check "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-02-07 22:40 +0100

#1575125 — [PATCH] bug: Switch data corruption check to __must_check

FromKees Cook <keescook@chromium.org>
Date2017-02-06 21:50 +0100
Subject[PATCH] bug: Switch data corruption check to __must_check
Message-ID<t7Yox-1to-3@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1575138

FromArnd Bergmann <arnd@arndb.de>
Date2017-02-06 22:10 +0100
Message-ID<t7YHU-1PY-37@gated-at.bofh.it>
In reply to#1575125
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

[toc] | [prev] | [next] | [standalone]

#1575144

FromKees Cook <keescook@chromium.org>
Date2017-02-06 22:20 +0100
Message-ID<t7YRA-1TU-21@gated-at.bofh.it>
In reply to#1575138
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

[toc] | [prev] | [next] | [standalone]

#1576045

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-02-07 21:50 +0100
Message-ID<t8kS5-7Ey-7@gated-at.bofh.it>
In reply to#1575125
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
>

[toc] | [prev] | [next] | [standalone]

#1576053

FromKees Cook <keescook@chromium.org>
Date2017-02-07 22:00 +0100
Message-ID<t8l1N-7If-29@gated-at.bofh.it>
In reply to#1576045
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

[toc] | [prev] | [next] | [standalone]

#1576075

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-02-07 22:40 +0100
Message-ID<t8lEu-8e1-3@gated-at.bofh.it>
In reply to#1576053
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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web