Re: [PATCH net-next v2] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling

[Jakub Kicinski]

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@xxxxxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Cc: Aleksandr Nogikh <nogikh@xxxxxxxxxx>
> Cc: Willem de Bruijn <willemb@xxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: linux-next@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx
> ---
> v2: (as suggested by Matthieu Baerts <matthieu.baerts@xxxxxxxxxxxx>)
>   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 */