Messages in this thread | | | From | Kees Cook <> | Date | Mon, 22 Aug 2016 15:32:06 -0700 | Subject | Re: [kernel-hardening] Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption |
| |
On Mon, Aug 22, 2016 at 10:53 AM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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
| |