Re: [PATCH 1/2] net: fix skb_ext_total_length() BUILD_BUG_ON with CONFIG_GCOV_PROFILE_ALL

[Konstantin Khorenko]

On 4/7/26 09:55, Paolo Abeni wrote:
> On 4/2/26 4:05 PM, Konstantin Khorenko wrote:
...
> > Fixes: 5d21d0a65b57 ("net: generalize calculation of skb extensions length")
> > Fixes: d6e5794b06c0 ("net: avoid build bug in skb extension length calculation")
> >
> > Signed-off-by: Konstantin Khorenko <khorenko@xxxxxxxxxxxxx>
>
> No empty lines in the tags area.

Sure, will fix.

> Also given the commit description, isn't the introduction of the 5th skb
> extension a better fixes tag?

Well, if we did not have 5d21d0a65b57 ("net: generalize calculation of skb extensions length"), we 
won't have a problem even after 5th skb extension.

On the other hand, yes, the defect reveals itself only after the appearance of the 5th skb extension, 
so we can also treat it guilty.

i will change the Fixes: tag.

> > Reviewed-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > ---
> >   net/core/skbuff.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 0e217041958a..47c7f0ab6e84 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5145,7 +5145,7 @@ static const u8 skb_ext_type_len[] = {
> >   #endif
> >   };
> >   
> > -static __always_inline unsigned int skb_ext_total_length(void)
> > +static __always_inline __no_profile unsigned int skb_ext_total_length(void)
> >   {
> >   	unsigned int l = SKB_EXT_CHUNKSIZEOF(struct skb_ext);
> >   	int i;
> > @@ -5159,9 +5159,7 @@ static __always_inline unsigned int skb_ext_total_length(void)
> >   static void skb_extensions_init(void)
> >   {
> >   	BUILD_BUG_ON(SKB_EXT_NUM > 8);
> > -#if !IS_ENABLED(CONFIG_KCOV_INSTRUMENT_ALL)
> >   	BUILD_BUG_ON(skb_ext_total_length() > 255);
> > -#endif
>
> Sashiko notes that there could be still build breakage:
>
> https://sashiko.dev/#/patchset/20260402140558.1437002-1-khorenko%40virtuozzo.com
>
> Could you please double check the above?
Sashiko is great!

The concern about KCOV is valid in theory but doesn't apply in practice. Here's why:

__no_profile (__no_profile_instrument_function__) indeed only prevents GCOV profiling counters
(-fprofile-arcs) from being inserted. It has no effect on KCOV instrumentation
(-fsanitize-coverage=trace-pc), which would require __no_sanitize_coverage instead.

However, KCOV instrumentation does not break constant folding in the first place.
I verified this with a standalone test: a __always_inline function with a loop over a const array 
(mimicking skb_ext_total_length()), compiled with different instrumentation flags:

 * No instrumentation: BUILD_BUG_ON passes (constant folded)
 * GCOV (-fprofile-arcs -ftest-coverage -fno-tree-loop-im): BUILD_BUG_ON fails
 * KCOV (-fsanitize-coverage=trace-pc): BUILD_BUG_ON passes (constant folded)
 * GCOV + atomic (-fprofile-arcs -ftest-coverage -fno-tree-loop-im -fprofile-update=atomic): 
BUILD_BUG_ON fails

The difference is in how GCC instruments code. GCOV inserts global counter increments inside the loop 
body. Combined with -fno-tree-loop-im, these counter operations prevent GCC from proving the loop 
result is a compile-time constant.

KCOV only inserts __sanitizer_cov_trace_pc() callbacks at basic block entries - these are opaque 
function calls that don't participate in value computation, so GCC can still see the loop iterates 
over a const array and fold it.


> I think a 'noinline' in skb_extensions_init() would address any
> complains on patch 2/2

Yes, will add "noinline" to be on a safe side.

--
Konstantin Khorenko