linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
@ 2020-11-16  3:17 Randy Dunlap
  2020-11-16 13:54 ` Matthieu Baerts
  0 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2020-11-16  3:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: Randy Dunlap, kernel test robot, Aleksandr Nogikh,
	Willem de Bruijn, Jakub Kicinski, linux-next, netdev,
	Matthieu Baerts

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.

Also, add stubs for skb_ext_add() and skb_ext_find() to reduce the
amount of ifdef-ery. (Jakub)

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
Cc: Matthieu Baerts <matthieu.baerts@tessares.net>
---
v4: The enum for SKB_EXT_KCOV_HANDLE needs to be exposed unconditionally
  because it is used in skb_get/set_kcov_handle(), which are always
  present since v3.
v3: (as suggested by Jakub Kicinski <kuba@kernel.org>)
  add stubs for skb_ext_add() and skb_ext_find() to reduce the ifdef-ery
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 |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

--- linux-next-20201113.orig/include/linux/skbuff.h
+++ linux-next-20201113/include/linux/skbuff.h
@@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s
 #endif
 }
 
-#ifdef CONFIG_SKB_EXTENSIONS
 enum skb_ext_id {
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	SKB_EXT_BRIDGE_NF,
@@ -4151,12 +4150,11 @@ enum skb_ext_id {
 #if IS_ENABLED(CONFIG_MPTCP)
 	SKB_EXT_MPTCP,
 #endif
-#if IS_ENABLED(CONFIG_KCOV)
 	SKB_EXT_KCOV_HANDLE,
-#endif
 	SKB_EXT_NUM, /* must be last */
 };
 
+#ifdef CONFIG_SKB_EXTENSIONS
 /**
  *	struct skb_ext - sk_buff extensions
  *	@refcnt: 1 on allocation, deallocated on 0
@@ -4252,6 +4250,10 @@ static inline void skb_ext_del(struct sk
 static inline void __skb_ext_copy(struct sk_buff *d, const struct sk_buff *s) {}
 static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *s) {}
 static inline bool skb_has_extensions(struct sk_buff *skb) { return false; }
+static inline void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
+{ return NULL; }
+static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id)
+{ return NULL; }
 #endif /* CONFIG_SKB_EXTENSIONS */
 
 static inline void nf_reset_ct(struct sk_buff *skb)
@@ -4608,7 +4610,6 @@ static inline void skb_reset_redirect(st
 #endif
 }
 
-#ifdef CONFIG_KCOV
 static inline void skb_set_kcov_handle(struct sk_buff *skb,
 				       const u64 kcov_handle)
 {
@@ -4632,11 +4633,6 @@ static inline u64 skb_get_kcov_handle(st
 
 	return kcov_handle ? *kcov_handle : 0;
 }
-#else
-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	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
  2020-11-16  3:17 [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling Randy Dunlap
@ 2020-11-16 13:54 ` Matthieu Baerts
  2020-11-16 14:31   ` Florian Westphal
  0 siblings, 1 reply; 6+ messages in thread
From: Matthieu Baerts @ 2020-11-16 13:54 UTC (permalink / raw)
  To: Randy Dunlap, linux-kernel
  Cc: kernel test robot, Aleksandr Nogikh, Willem de Bruijn,
	Jakub Kicinski, linux-next, netdev, Florian Westphal

Hi Randy,

On 16/11/2020 04:17, 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.
> 
> Also, add stubs for skb_ext_add() and skb_ext_find() to reduce the
> amount of ifdef-ery. (Jakub)

It makes sense, good idea!

Thank you for the new version!

> --- linux-next-20201113.orig/include/linux/skbuff.h
> +++ linux-next-20201113/include/linux/skbuff.h
> @@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s
>   #endif
>   }
>   
> -#ifdef CONFIG_SKB_EXTENSIONS
>   enum skb_ext_id {
>   #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>   	SKB_EXT_BRIDGE_NF,
> @@ -4151,12 +4150,11 @@ enum skb_ext_id {
>   #if IS_ENABLED(CONFIG_MPTCP)
>   	SKB_EXT_MPTCP,
>   #endif
> -#if IS_ENABLED(CONFIG_KCOV)
>   	SKB_EXT_KCOV_HANDLE,
> -#endif

I don't think we should remove this #ifdef: the number of extensions are 
currently limited to 8, we might not want to always have KCOV there even 
if we don't want it. I think adding items in this enum only when needed 
was the intension of Florian (+cc) when creating these SKB extensions.
Also, this will increase a tiny bit some structures, see "struct skb_ext()".

But apart from that, I think we are fine, even if we add new extensions 
in the future after this kcov one.

So if we think it is better to remove these #ifdef here, we should be 
OK. But if we prefer not to do that, we should then not add stubs for 
skb_ext_{add,find}() and keep the ones for skb_[gs]et_kcov_handle().

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
  2020-11-16 13:54 ` Matthieu Baerts
@ 2020-11-16 14:31   ` Florian Westphal
  2020-11-16 15:30     ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2020-11-16 14:31 UTC (permalink / raw)
  To: Matthieu Baerts
  Cc: Randy Dunlap, linux-kernel, kernel test robot, Aleksandr Nogikh,
	Willem de Bruijn, Jakub Kicinski, linux-next, netdev,
	Florian Westphal

Matthieu Baerts <matthieu.baerts@tessares.net> wrote:
> > --- linux-next-20201113.orig/include/linux/skbuff.h
> > +++ linux-next-20201113/include/linux/skbuff.h
> > @@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s
> >   #endif
> >   }
> > -#ifdef CONFIG_SKB_EXTENSIONS
> >   enum skb_ext_id {
> >   #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> >   	SKB_EXT_BRIDGE_NF,
> > @@ -4151,12 +4150,11 @@ enum skb_ext_id {
> >   #if IS_ENABLED(CONFIG_MPTCP)
> >   	SKB_EXT_MPTCP,
> >   #endif
> > -#if IS_ENABLED(CONFIG_KCOV)
> >   	SKB_EXT_KCOV_HANDLE,
> > -#endif
> 
> I don't think we should remove this #ifdef: the number of extensions are
> currently limited to 8, we might not want to always have KCOV there even if
> we don't want it. I think adding items in this enum only when needed was the
> intension of Florian (+cc) when creating these SKB extensions.
> Also, this will increase a tiny bit some structures, see "struct skb_ext()".

Yes, I would also prefer to retrain the ifdef.

Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives
a compile error if the extension is not enabled.

> So if we think it is better to remove these #ifdef here, we should be OK.
> But if we prefer not to do that, we should then not add stubs for
> skb_ext_{add,find}() and keep the ones for skb_[gs]et_kcov_handle().

Yes, exactly, I did not add these stubs because I could not figure out
a case where an empty skb_ext_{add,find} would be wanted.

If your code calls skb_ext_add() but no skb extensions exist you forgot
a SELECT/DEPENDS SKB_EXTENSIONS in Kconfig & compiler error would tell
you that.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
  2020-11-16 14:31   ` Florian Westphal
@ 2020-11-16 15:30     ` Jakub Kicinski
  2020-11-16 17:08       ` Randy Dunlap
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-11-16 15:30 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Florian Westphal, Matthieu Baerts, linux-kernel,
	kernel test robot, Aleksandr Nogikh, Willem de Bruijn,
	linux-next, netdev

On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote:
> > > @@ -4151,12 +4150,11 @@ enum skb_ext_id {
> > >   #if IS_ENABLED(CONFIG_MPTCP)
> > >   	SKB_EXT_MPTCP,
> > >   #endif
> > > -#if IS_ENABLED(CONFIG_KCOV)
> > >   	SKB_EXT_KCOV_HANDLE,
> > > -#endif  
> > 
> > I don't think we should remove this #ifdef: the number of extensions are
> > currently limited to 8, we might not want to always have KCOV there even if
> > we don't want it. I think adding items in this enum only when needed was the
> > intension of Florian (+cc) when creating these SKB extensions.
> > Also, this will increase a tiny bit some structures, see "struct skb_ext()".  
> 
> Yes, I would also prefer to retrain the ifdef.
> 
> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives
> a compile error if the extension is not enabled.

Oh well, sorry for taking you down the wrong path Randy!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
  2020-11-16 15:30     ` Jakub Kicinski
@ 2020-11-16 17:08       ` Randy Dunlap
  2020-11-16 17:16         ` Florian Westphal
  0 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2020-11-16 17:08 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Florian Westphal, Matthieu Baerts, linux-kernel,
	kernel test robot, Aleksandr Nogikh, Willem de Bruijn,
	linux-next, netdev

On 11/16/20 7:30 AM, Jakub Kicinski wrote:
> On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote:
>>>> @@ -4151,12 +4150,11 @@ enum skb_ext_id {
>>>>   #if IS_ENABLED(CONFIG_MPTCP)
>>>>   	SKB_EXT_MPTCP,
>>>>   #endif
>>>> -#if IS_ENABLED(CONFIG_KCOV)
>>>>   	SKB_EXT_KCOV_HANDLE,
>>>> -#endif  
>>>
>>> I don't think we should remove this #ifdef: the number of extensions are
>>> currently limited to 8, we might not want to always have KCOV there even if
>>> we don't want it. I think adding items in this enum only when needed was the
>>> intension of Florian (+cc) when creating these SKB extensions.
>>> Also, this will increase a tiny bit some structures, see "struct skb_ext()".  
>>
>> Yes, I would also prefer to retrain the ifdef.
>>
>> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives
>> a compile error if the extension is not enabled.
> 
> Oh well, sorry for taking you down the wrong path Randy!

No problem.
So we are back to v2, right?
Do I need to resend that one?

thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
  2020-11-16 17:08       ` Randy Dunlap
@ 2020-11-16 17:16         ` Florian Westphal
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2020-11-16 17:16 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Jakub Kicinski, Florian Westphal, Matthieu Baerts, linux-kernel,
	kernel test robot, Aleksandr Nogikh, Willem de Bruijn,
	linux-next, netdev

Randy Dunlap <rdunlap@infradead.org> wrote:
> On 11/16/20 7:30 AM, Jakub Kicinski wrote:
> > On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote:
> >>>> @@ -4151,12 +4150,11 @@ enum skb_ext_id {
> >>>>   #if IS_ENABLED(CONFIG_MPTCP)
> >>>>   	SKB_EXT_MPTCP,
> >>>>   #endif
> >>>> -#if IS_ENABLED(CONFIG_KCOV)
> >>>>   	SKB_EXT_KCOV_HANDLE,
> >>>> -#endif  
> >>>
> >>> I don't think we should remove this #ifdef: the number of extensions are
> >>> currently limited to 8, we might not want to always have KCOV there even if
> >>> we don't want it. I think adding items in this enum only when needed was the
> >>> intension of Florian (+cc) when creating these SKB extensions.
> >>> Also, this will increase a tiny bit some structures, see "struct skb_ext()".  
> >>
> >> Yes, I would also prefer to retrain the ifdef.
> >>
> >> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives
> >> a compile error if the extension is not enabled.
> > 
> > Oh well, sorry for taking you down the wrong path Randy!
> 
> No problem.
> So we are back to v2, right?

Yes, you can still drop the line

>> +#if IS_ENABLED(CONFIG_KCOV) && IS_ENABLED(CONFIG_SKB_EXTENSIONS)

for enum skb_ext_id (alreadyt under SKB_EXTENSIONS).

Other than that v2 looks good to me.

Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-16 17:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16  3:17 [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling Randy Dunlap
2020-11-16 13:54 ` Matthieu Baerts
2020-11-16 14:31   ` Florian Westphal
2020-11-16 15:30     ` Jakub Kicinski
2020-11-16 17:08       ` Randy Dunlap
2020-11-16 17:16         ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).