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
