Re: [PATCH 10/18] fwd_rule: Move rule conflict checking from fwd_rule_add() to caller

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]

On Wed, Apr 08, 2026 at 11:37:04AM +1000, David Gibson wrote:
> On Wed, Apr 08, 2026 at 01:14:50AM +0200, Stefano Brivio wrote:
> > On Tue,  7 Apr 2026 13:16:22 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > Meanwhile, I noticed some warnings that strangely appear only during the
> > build of passt.avx2:
> > 
> > cc -Wall -Wextra -Wno-format-zero-length -Wformat-security -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -pie -fPIE -DPAGE_SIZE=4096 -DVERSION=\"2026_01_20.386b5f5-84-ge87c74f\" -DDUAL_STACK_SOCKETS=1 -DHAS_GETRANDOM -fstack-protector-strong -Ofast -mavx2 -ftree-vectorize -funroll-loops   \
> > 	arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c -o passt.avx2 
> > In file included from util.h:22,
> >                  from ip.h:12,
> >                  from fwd_rule.h:16,
> >                  from fwd_rule.c:20:
> > fwd_rule.c: In function ‘fwd_rules_info’:
> > fwd_rule.c:86:22: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >    86 |                 info("    %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf)));
> >       |                      ^~~~~~~~
> > log.h:31:66: note: in definition of macro ‘info’
> >    31 | #define info(...)               logmsg(true, false, LOG_INFO,    __VA_ARGS__)
> >       |                                                                  ^~~~~~~~~~~
> > fwd_rule.c:86:27: note: format string is defined here
> >    86 |                 info("    %s", fwd_rule_fmt(&rules[i], buf, sizeof(buf)));
> >       |                           ^~
> > 
> > ...but I don't think I looked at the code changes causing them, yet (and
> > didn't bisect the series either). This is with gcc 13.3.0.
> 
> Huh.  I don't see those those with gcc 15.2.1.  I _think_ it's a false
> positive.  Investigating...

Ok, I think I have a workaround for this.  I reproduced the problem,
and unsurprisingly it's introduced in patch 7/18 that introduces the
new rule formatting helpers.

It appears to occur with gcc 12, 13 & 14, but not 15.  It's not the
AVX2 per-se that triggers it, but the fact we use -Ofast for the
passt.avx2 build - it appears to trigger if we go above -O2.  The gcc
man page does warn that high optimization levels might cause false
positives for this warning:

    -Wformat-overflow=level
        Warn about calls to formatted input/output functions such as
	"sprintf" and "vsprintf" that might overflow the destination
	buffer.  When the exact number of bytes written by a format
	directive cannot be determined at compile-time it is estimated
	based on heuristics that depend on the level argument and on
	optimization.  While enabling optimization will in most cases
	improve the accuracy of the warning, it may also result in
	false positives.

It's theoretically true that fwd_rule_fmt() can return NULL.  However
the buffer is sized so that shouldn't ever happen.  However gcc seems
to think it knows it *is* returning NULL, not just that it could.

I'm pretty sure my calculation for the max buffer size is correct.
Even if it's not, I'm pretty sure there is a gcc bug in play here:
I get the same warning if I replace the guts of fwd_rule_fmt() with an
snprintf() of a short fixed string to the output buffer, then check
the (now statically known) return value against the buffer size.

I _think_ the reason it's occurring here but not the many other places
we use a similar pattern (e.g. every inet_ntop() called as a printf
argument) is because here the caller is in the same translation unit
as fwd_rule_fmt(), so gcc thinks it can look into fwd_rule_fmt() and
reason about it, but it's reasoning wrong.

In fact, looking closer it seems the problem is triggered when this
function is inlined, which I guess it's trying to do on the higher
optimization levels.  In v2 I've added a ((noinline)) attribute if
__GNUC__ < 15, which appears to do the trick.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]