From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756908AbcHVWcK (ORCPT ); Mon, 22 Aug 2016 18:32:10 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:35400 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752016AbcHVWcJ (ORCPT ); Mon, 22 Aug 2016 18:32:09 -0400 MIME-Version: 1.0 In-Reply-To: <20160822175307.GI3482@linux.vnet.ibm.com> References: <1471470132-29499-1-git-send-email-keescook@chromium.org> <1471470132-29499-5-git-send-email-keescook@chromium.org> <4367840.AWTJIHYqbe@wuerfel> <20160822175307.GI3482@linux.vnet.ibm.com> From: Kees Cook Date: Mon, 22 Aug 2016 15:32:06 -0700 X-Google-Sender-Auth: tKL32FcivmqnO07ckstO-oo9d_Q Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption To: "kernel-hardening@lists.openwall.com" Cc: Arnd Bergmann , Laura Abbott , Steven Rostedt , Daniel Micay , Joe Perches , Stephen Boyd , Syed Rameez Mustafa , Greg Kroah-Hartman , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , "Aneesh Kumar K.V" , "Kirill A. Shutemov" , Michael Ellerman , Andrew Morton , Dan Williams , Jan Kara , Thomas Gleixner , Josef Bacik , Ingo Molnar , Tejun Heo , Andrey Ryabinin , Nikolay Aleksandrov , Dmitry Vyukov , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 22, 2016 at 10:53 AM, Paul E. McKenney wrote: > On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote: >> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote: >> > + >> > +/* >> > + * Since detected data corruption should stop operation on the affected >> > + * structures, this returns false if the corruption condition is found. >> > + */ >> > +#define CHECK_DATA_CORRUPTION(condition, fmt, ...) \ >> > + do { \ >> > + if (unlikely(condition)) { \ >> > + if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \ >> > + pr_err(fmt, ##__VA_ARGS__); \ >> > + BUG(); \ >> > + } else \ >> > + WARN(1, fmt, ##__VA_ARGS__); \ >> > + return false; \ >> > + } \ >> > + } while (0) >> > + >> >> I think the "return false" inside of the macro makes it easy to misread >> what is actually going on. >> >> How about making it a macro that returns the condition argument? >> >> #define CHECK_DATA_CORRUPTION(condition, fmt, ...) \ >> ({ \ >> bool _condition = unlikely(condition); \ >> if (_condition) { \ >> ... >> } \ >> _condition; \ >> }) > > That does look better, now that you mention it. Kees, any objections? That's fine with me; it'll require changing the callers of the macros to test their results, but that should be clean change. -Kees -- Kees Cook Nexus Security From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <20160822175307.GI3482@linux.vnet.ibm.com> References: <1471470132-29499-1-git-send-email-keescook@chromium.org> <1471470132-29499-5-git-send-email-keescook@chromium.org> <4367840.AWTJIHYqbe@wuerfel> <20160822175307.GI3482@linux.vnet.ibm.com> From: Kees Cook Date: Mon, 22 Aug 2016 15:32:06 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [kernel-hardening] Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption To: "kernel-hardening@lists.openwall.com" Cc: Arnd Bergmann , Laura Abbott , Steven Rostedt , Daniel Micay , Joe Perches , Stephen Boyd , Syed Rameez Mustafa , Greg Kroah-Hartman , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , "Aneesh Kumar K.V" , "Kirill A. Shutemov" , Michael Ellerman , Andrew Morton , Dan Williams , Jan Kara , Thomas Gleixner , Josef Bacik , Ingo Molnar , Tejun Heo , Andrey Ryabinin , Nikolay Aleksandrov , Dmitry Vyukov , LKML List-ID: On Mon, Aug 22, 2016 at 10:53 AM, Paul E. McKenney wrote: > On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote: >> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote: >> > + >> > +/* >> > + * Since detected data corruption should stop operation on the affected >> > + * structures, this returns false if the corruption condition is found. >> > + */ >> > +#define CHECK_DATA_CORRUPTION(condition, fmt, ...) \ >> > + do { \ >> > + if (unlikely(condition)) { \ >> > + if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \ >> > + pr_err(fmt, ##__VA_ARGS__); \ >> > + BUG(); \ >> > + } else \ >> > + WARN(1, fmt, ##__VA_ARGS__); \ >> > + return false; \ >> > + } \ >> > + } while (0) >> > + >> >> I think the "return false" inside of the macro makes it easy to misread >> what is actually going on. >> >> How about making it a macro that returns the condition argument? >> >> #define CHECK_DATA_CORRUPTION(condition, fmt, ...) \ >> ({ \ >> bool _condition = unlikely(condition); \ >> if (_condition) { \ >> ... >> } \ >> _condition; \ >> }) > > That does look better, now that you mention it. Kees, any objections? That's fine with me; it'll require changing the callers of the macros to test their results, but that should be clean change. -Kees -- Kees Cook Nexus Security