linux-kernel - Re: [RFC][PATCH v1 1/1] Add WARN_ONCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAHp75VcYrTX7TecQuNWP+ZmSbQYyuhtGKv1wSK9b5BSBsT2_ZA@mail.gmail.com>
Date:	Fri, 14 Aug 2015 21:25:37 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Casey Schaufler <casey@...aufler-ca.com>
Cc:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH v1 1/1] Add WARN_ONCE_RETURN macro

On Fri, Aug 14, 2015 at 6:48 PM, Casey Schaufler <casey@...aufler-ca.com> wrote:
> On 8/14/2015 6:43 AM, Andy Shevchenko wrote:
>> The rationale is to replace the
>>       if (condition) {
>>               WARN_ONCE(1, ...);
>>               return value;
>>       }
>> by
>>       WARN_ONCE_RETURN(condition, value, ...);
>
> The kernel coding style says:
>
> Things to avoid when using macros:
>
> 1) macros that affect control flow:
>
>         #define FOO(x)                                  \
>                 do {                                    \
>                         if (blah(x) < 0)                \
>                                 return -EBUGGERED;      \
>                 } while(0)
>
> is a _very_ bad idea.  It looks like a function call but exits the "calling"
> function; don't break the internal parsers of those who will read the code.

My intuition tells me something like that, that's why RFC (and on
Friday). I didn't think it's already clarified in the CosdingStyle.
Thanks for remind about this nice document.

And sorry for a noise.

>
> So no, I won't be taking this unless there's a change in
> the coding guidelines, which I coincidentally agree with.
>
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> ---
>>  include/asm-generic/bug.h  | 17 +++++++++++++++++
>>  security/smack/smack_lsm.c |  5 +----
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 630dd23..fbf20fd 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -126,6 +126,18 @@ extern void warn_slowpath_null(const char *file, const int line);
>>       unlikely(__ret_warn_once);                              \
>>  })
>>
>> +#define WARN_ONCE_RETURN(condition, value, format...)                \
>> +do {                                                         \
>> +     static bool __section(.data.unlikely) __warned;         \
>> +     int __ret_warn_once = !!(condition);                    \
>> +                                                             \
>> +     if (unlikely(__ret_warn_once)) {                        \
>> +             if (WARN(!__warned, format))                    \
>> +                     __warned = true;                        \
>> +             return value;                                   \
>> +     }                                                       \
>> +} while (0)
>> +
>>  #define WARN_TAINT_ONCE(condition, taint, format...) ({      \
>>       static bool __section(.data.unlikely) __warned;         \
>>       int __ret_warn_once = !!(condition);                    \
>> @@ -162,6 +174,11 @@ extern void warn_slowpath_null(const char *file, const int line);
>>
>>  #define WARN_ON_ONCE(condition) WARN_ON(condition)
>>  #define WARN_ONCE(condition, format...) WARN(condition, format)
>> +#define WARN_ONCE_RETURN(condition, value, format...)                        \
>> +do {                                                                 \
>> +     if (WARN(condition, format))                                    \
>> +             return value;                                           \
>> +} while (0)
>>  #define WARN_TAINT(condition, taint, format...) WARN(condition, format)
>>  #define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 54fb3a1..4160d0e 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -4401,10 +4401,7 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
>>       struct smack_known *skp;
>>       char *rule = vrule;
>>
>> -     if (unlikely(!rule)) {
>> -             WARN_ONCE(1, "Smack: missing rule\n");
>> -             return -ENOENT;
>> -     }
>> +     WARN_ONCE_RETURN(unlikely(!rule), -ENOENT, "Smack: missing rule\n");
>>
>>       if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER)
>>               return 0;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/