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@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.

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