From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932254AbdBGUtZ (ORCPT ); Tue, 7 Feb 2017 15:49:25 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:35289 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754721AbdBGUtW (ORCPT ); Tue, 7 Feb 2017 15:49:22 -0500 Date: Tue, 7 Feb 2017 12:39:21 -0800 From: "Paul E. McKenney" To: Kees Cook Cc: Arnd Bergmann , linux-kernel@vger.kernel.org, mingo@kernel.org, jiangshanlai@gmail.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com Subject: Re: [PATCH] bug: Switch data corruption check to __must_check Reply-To: paulmck@linux.vnet.ibm.com References: <20170206204547.GA125312@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170206204547.GA125312@beast> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17020720-0012-0000-0000-0000139599F9 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006574; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000202; SDB=6.00818619; UDB=6.00400056; IPR=6.00596079; BA=6.00005123; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014214; XFM=3.00000011; UTC=2017-02-07 20:49:18 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17020720-0013-0000-0000-00004B36FB8E Message-Id: <20170207203921.GZ30506@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-02-07_11:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1702070198 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Kees Cook > --- > 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 >