netdev - Re: [PATCH net-next,v2,1/2] once: implement DO_ONCE_LITE for non-fast-path "do once" functionality

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]

Date:   Thu, 27 May 2021 16:27:20 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tanner Love <tannerlove.kernel@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        Tanner Love <tannerlove@...gle.com>,
        kernel test robot <lkp@...el.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Mahesh Bandewar <maheshb@...gle.com>
Subject: Re: [PATCH net-next,v2,1/2] once: implement DO_ONCE_LITE for
 non-fast-path "do once" functionality

On Wed, 26 May 2021 19:13:35 -0400 Tanner Love wrote:
> From: Tanner Love <tannerlove@...gle.com>
> 
> Certain uses of "do once" functionality reside outside of fast path,
> and so do not require jump label patching via static keys, making
> existing DO_ONCE undesirable in such cases.
> 
> Replace uses of __section(".data.once") with DO_ONCE_LITE(_IF)?
> 
> [ i386 build warnings ]
> Reported-by: kernel test robot <lkp@...el.com>
> Signed-off-by: Tanner Love <tannerlove@...gle.com>
> Acked-by: Eric Dumazet <edumazet@...gle.com>
> Acked-by: Mahesh Bandewar <maheshb@...gle.com>
> ---
>  fs/xfs/xfs_message.h      | 13 +++----------
>  include/asm-generic/bug.h | 37 +++++++------------------------------
>  include/linux/once_lite.h | 24 ++++++++++++++++++++++++
>  include/linux/printk.h    | 23 +++--------------------
>  kernel/trace/trace.h      | 13 +++----------

You need to cast a wider net for the CC list here. This is not all
networking code. You should also probably CC LKML on general changes
like this.

>  #define xfs_printk_once(func, dev, fmt, ...)			\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once; 			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		func(dev, fmt, ##__VA_ARGS__);			\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(func, dev, fmt, ##__VA_ARGS__)

>  #ifdef CONFIG_PRINTK
>  #define printk_once(fmt, ...)					\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once;			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		printk(fmt, ##__VA_ARGS__);			\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(printk, fmt, ##__VA_ARGS__)
>  #define printk_deferred_once(fmt, ...)				\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once;			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		printk_deferred(fmt, ##__VA_ARGS__);		\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(printk_deferred, fmt, ##__VA_ARGS__)

These are not equivalent to your new macro below. They used to return
if the print was done or not, now they'll always return true. You need
to double check this doesn't break anything and add a note about it in
the commit message.

> +#define DO_ONCE_LITE_IF(condition, func, ...)				\
> +	({								\
> +		static bool __section(".data.once") __already_done;	\
> +		bool __ret_do_once = !!(condition);			\
> +									\
> +		if (unlikely(__ret_do_once && !__already_done)) {	\
> +			__already_done = true;				\
> +			func(__VA_ARGS__);				\
> +		}							\
> +		unlikely(__ret_do_once);				\
> +	})
> +