netdev - Re: [PATCH net-next v2] net: linux/skbuff.h: combine SKB

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201114115437.55eed094@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 14 Nov 2020 11:54:37 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Randy Dunlap <rdunlap@...radead.org>
Cc:     linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>,
        Aleksandr Nogikh <nogikh@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        linux-next@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: linux/skbuff.h: combine SKB_EXTENSIONS
 + KCOV handling

On Sat, 14 Nov 2020 09:46:18 -0800 Randy Dunlap wrote:
> The previous Kconfig patch led to some other build errors as
> reported by the 0day bot and my own overnight build testing.
> 
> These are all in <linux/skbuff.h> when KCOV is enabled but
> SKB_EXTENSIONS is not enabled, so fix those by combining those conditions
> in the header file.
> 
> Fixes: 6370cc3bbd8a ("net: add kcov handle to skb extensions")
> Fixes: 85ce50d337d1 ("net: kcov: don't select SKB_EXTENSIONS when there is no NET")
> Signed-off-by: Randy Dunlap <rdunlap@...radead.org>
> Reported-by: kernel test robot <lkp@...el.com>
> Cc: Aleksandr Nogikh <nogikh@...gle.com>
> Cc: Willem de Bruijn <willemb@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: linux-next@...r.kernel.org
> Cc: netdev@...r.kernel.org
> ---
> v2: (as suggested by Matthieu Baerts <matthieu.baerts@...sares.net>)
>   drop an extraneous space in a comment;
>   use CONFIG_SKB_EXTENSIONS instead of CONFIG_NET;

Thanks for the fix Randy!

> --- linux-next-20201113.orig/include/linux/skbuff.h
> +++ linux-next-20201113/include/linux/skbuff.h
> @@ -4151,7 +4151,7 @@ enum skb_ext_id {
>  #if IS_ENABLED(CONFIG_MPTCP)
>  	SKB_EXT_MPTCP,
>  #endif
> -#if IS_ENABLED(CONFIG_KCOV)
> +#if IS_ENABLED(CONFIG_KCOV) && IS_ENABLED(CONFIG_SKB_EXTENSIONS)

I don't think this part is necessary, this is already under an ifdef:

#ifdef CONFIG_SKB_EXTENSIONS
enum skb_ext_id {

if I'm reading the code right.

That said I don't know why the enum is under CONFIG_SKB_EXTENSIONS in
the first place.

If extensions are not used doesn't matter if we define the enum and with
how many entries.

At the same time if we take the enum from under the ifdef and add stubs
for skb_ext_add() and skb_ext_find() we could actually remove the stubs
for kcov-related helpers. That seems cleaner and less ifdefy to me.

WDYT?

>  	SKB_EXT_KCOV_HANDLE,
>  #endif
>  	SKB_EXT_NUM, /* must be last */
> @@ -4608,7 +4608,7 @@ static inline void skb_reset_redirect(st
>  #endif
>  }
>  
> -#ifdef CONFIG_KCOV
> +#if IS_ENABLED(CONFIG_KCOV) && IS_ENABLED(CONFIG_SKB_EXTENSIONS)
>  static inline void skb_set_kcov_handle(struct sk_buff *skb,
>  				       const u64 kcov_handle)
>  {
> @@ -4636,7 +4636,7 @@ static inline u64 skb_get_kcov_handle(st
>  static inline void skb_set_kcov_handle(struct sk_buff *skb,
>  				       const u64 kcov_handle) { }
>  static inline u64 skb_get_kcov_handle(struct sk_buff *skb) { return 0; }
> -#endif /* CONFIG_KCOV */
> +#endif /* CONFIG_KCOV && CONFIG_SKB_EXTENSIONS */
>  
>  #endif	/* __KERNEL__ */
>  #endif	/* _LINUX_SKBUFF_H */