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

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

[Randy Dunlap]

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@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Aleksandr Nogikh <nogikh@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-next@vger.kernel.org
Cc: netdev@vger.kernel.org
---
v2: (as suggested by Matthieu Baerts <matthieu.baerts@tessares.net>)
  drop an extraneous space in a comment;
  use CONFIG_SKB_EXTENSIONS instead of CONFIG_NET;

 include/linux/skbuff.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- 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)
 	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 */

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@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Aleksandr Nogikh <nogikh@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: linux-next@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> v2: (as suggested by Matthieu Baerts <matthieu.baerts@tessares.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 */

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

[Randy Dunlap]

On 11/14/20 11:54 AM, Jakub Kicinski wrote:
> 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@infradead.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Cc: Aleksandr Nogikh <nogikh@google.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: linux-next@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> ---
>> v2: (as suggested by Matthieu Baerts <matthieu.baerts@tessares.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.

Oops, you are correct. Sorry I missed that.


> 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?

Good thing I am on my third cup of coffee.

OK, it looks like that should work -- with less #ifdef-ery.

I'll work at it...


>>  	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 */
> 

thanks.
-- 
~Randy