[libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build: ../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616 bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-larger-than=] virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) ^ 1 error generated. This series reworks the function a bit to bring the frame size below 8192. Regards, Tim Tim Wiederhake (3): virNWFilterRuleDefFixup: Factor out ethHdr as variable virNWFilterRuleDefFixup: Factor out ipHdr as variable virNWFilterRuleDefFixup: Factor out portData as variable src/conf/nwfilter_conf.c | 179 ++++++++++++++++----------------------- 1 file changed, 73 insertions(+), 106 deletions(-) -- 2.31.1
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a3109962af..62334edeec 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule) static void virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { + ethHdrDataDef *ethHdr; + #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG); switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask, - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr); - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask, - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr); + ethHdr = &rule->p.ethHdrFilter.ethHdr; + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr); + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr); break; case VIR_NWFILTER_RULE_PROTOCOL_VLAN: - COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataSrcMACMask, - rule->p.vlanHdrFilter.ethHdr.dataSrcMACAddr); - COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataDstMACMask, - rule->p.vlanHdrFilter.ethHdr.dataDstMACAddr); + ethHdr = &rule->p.vlanHdrFilter.ethHdr; + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr); + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr); break; case VIR_NWFILTER_RULE_PROTOCOL_STP: -- 2.31.1
On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a3109962af..62334edeec 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule) static void virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { + ethHdrDataDef *ethHdr; + #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG);
switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask, - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr); - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask, - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr); + ethHdr = &rule->p.ethHdrFilter.ethHdr; + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr); + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr);
I don't get why this reduces stack size at all ? COPY_NEG_SIGN is just doing a bitwise and + or + assignment which doesn't require any intermediate variables. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 2021-09-17 at 14:08 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a3109962af..62334edeec 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule) static void virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { + ethHdrDataDef *ethHdr; + #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG); switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask, - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr); - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask, - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr); + ethHdr = &rule->p.ethHdrFilter.ethHdr; + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr-
dataSrcMACAddr); + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr- dataDstMACAddr);
I don't get why this reduces stack size at all ?
COPY_NEG_SIGN is just doing a bitwise and + or + assignment which doesn't require any intermediate variables.
Regards, Daniel
I believe it's debug information for something that gets optimized away in non-debug (i.e. release) builds. Do you know of a way to ask clang to explain what takes up the space in a stack frame? Regards, Tim
On Fri, Sep 17, 2021 at 03:37:25PM +0200, Tim Wiederhake wrote:
On Fri, 2021-09-17 at 14:08 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index a3109962af..62334edeec 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule) static void virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { + ethHdrDataDef *ethHdr; + #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG); switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask, - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr); - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask, - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr); + ethHdr = &rule->p.ethHdrFilter.ethHdr; + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr-
dataSrcMACAddr); + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr- dataDstMACAddr);
I don't get why this reduces stack size at all ?
COPY_NEG_SIGN is just doing a bitwise and + or + assignment which doesn't require any intermediate variables.
Regards, Daniel
I believe it's debug information for something that gets optimized away in non-debug (i.e. release) builds.
Do you know of a way to ask clang to explain what takes up the space in a stack frame?
Not unless -save-temps shows it in the intermediate files Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 132 +++++++++++++++------------------------ 1 file changed, 52 insertions(+), 80 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 62334edeec..2448e4b70b 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2190,6 +2190,7 @@ static void virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { ethHdrDataDef *ethHdr; + ipHdrDataDef *ipHdr; #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ @@ -2234,18 +2235,16 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) break; case VIR_NWFILTER_RULE_PROTOCOL_IP: - COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataSrcIPMask, - rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataDstIPMask, - rule->p.ipHdrFilter.ipHdr.dataDstIPAddr); + ipHdr = &rule->p.ipHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); virNWFilterRuleDefFixupIPSet(&rule->p.ipHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_IPV6: - COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask, - rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask, - rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr); + ipHdr = &rule->p.ipv6HdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); COPY_NEG_SIGN(rule->p.ipv6HdrFilter.dataICMPTypeEnd, rule->p.ipv6HdrFilter.dataICMPTypeStart); COPY_NEG_SIGN(rule->p.ipv6HdrFilter.dataICMPCodeStart, @@ -2262,14 +2261,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_TCP: case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: - COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataSrcIPMask, - rule->p.tcpHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataDstIPMask, - rule->p.tcpHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataSrcIPTo, - rule->p.tcpHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataDstIPTo, - rule->p.tcpHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.tcpHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataSrcPortEnd, rule->p.tcpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortStart, @@ -2281,14 +2277,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_UDP: case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6: - COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataSrcIPMask, - rule->p.udpHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataDstIPMask, - rule->p.udpHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataSrcIPTo, - rule->p.udpHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataDstIPTo, - rule->p.udpHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.udpHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataSrcPortEnd, rule->p.udpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortStart, @@ -2300,53 +2293,41 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_UDPLITE: case VIR_NWFILTER_RULE_PROTOCOL_UDPLITEoIPV6: - COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataSrcIPMask, - rule->p.udpliteHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataDstIPMask, - rule->p.udpliteHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataSrcIPTo, - rule->p.udpliteHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataDstIPTo, - rule->p.udpliteHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.udpliteHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); virNWFilterRuleDefFixupIPSet(&rule->p.udpliteHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_ESP: case VIR_NWFILTER_RULE_PROTOCOL_ESPoIPV6: - COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataSrcIPMask, - rule->p.espHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataDstIPMask, - rule->p.espHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataSrcIPTo, - rule->p.espHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataDstIPTo, - rule->p.espHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.espHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); virNWFilterRuleDefFixupIPSet(&rule->p.espHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_AH: case VIR_NWFILTER_RULE_PROTOCOL_AHoIPV6: - COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataSrcIPMask, - rule->p.ahHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataDstIPMask, - rule->p.ahHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataSrcIPTo, - rule->p.ahHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataDstIPTo, - rule->p.ahHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.ahHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); virNWFilterRuleDefFixupIPSet(&rule->p.ahHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_SCTP: case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6: - COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataSrcIPMask, - rule->p.sctpHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataDstIPMask, - rule->p.sctpHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataSrcIPTo, - rule->p.sctpHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataDstIPTo, - rule->p.sctpHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.sctpHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataSrcPortEnd, rule->p.sctpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortStart, @@ -2358,14 +2339,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_ICMP: case VIR_NWFILTER_RULE_PROTOCOL_ICMPV6: - COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataSrcIPMask, - rule->p.icmpHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataDstIPMask, - rule->p.icmpHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataSrcIPTo, - rule->p.icmpHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataDstIPTo, - rule->p.icmpHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.icmpHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPCode, rule->p.icmpHdrFilter.dataICMPType); virNWFilterRuleDefFixupIPSet(&rule->p.icmpHdrFilter.ipHdr); @@ -2373,25 +2351,19 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_ALL: case VIR_NWFILTER_RULE_PROTOCOL_ALLoIPV6: - COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataSrcIPMask, - rule->p.allHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataDstIPMask, - rule->p.allHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataSrcIPTo, - rule->p.allHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataDstIPTo, - rule->p.allHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.allHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); break; case VIR_NWFILTER_RULE_PROTOCOL_IGMP: - COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataSrcIPMask, - rule->p.igmpHdrFilter.ipHdr.dataSrcIPAddr); - COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataDstIPMask, - rule->p.igmpHdrFilter.ipHdr.dataDstIPAddr); - COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataSrcIPTo, - rule->p.igmpHdrFilter.ipHdr.dataSrcIPFrom); - COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataDstIPTo, - rule->p.igmpHdrFilter.ipHdr.dataDstIPFrom); + ipHdr = &rule->p.igmpHdrFilter.ipHdr; + COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); + COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); + COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); + COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); virNWFilterRuleDefFixupIPSet(&rule->p.igmpHdrFilter.ipHdr); break; -- 2.31.1
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug mode, on clang. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 2448e4b70b..ad1edc98dd 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2191,6 +2191,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) { ethHdrDataDef *ethHdr; ipHdrDataDef *ipHdr; + portDataDef *portData; #define COPY_NEG_SIGN(A, B) \ (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \ @@ -2262,32 +2263,28 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_TCP: case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: ipHdr = &rule->p.tcpHdrFilter.ipHdr; + portData = &rule->p.tcpHdrFilter.portData; COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataSrcPortEnd, - rule->p.tcpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortStart, - rule->p.tcpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortEnd, - rule->p.tcpHdrFilter.portData.dataSrcPortStart); + COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart); virNWFilterRuleDefFixupIPSet(&rule->p.tcpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_UDP: case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6: ipHdr = &rule->p.udpHdrFilter.ipHdr; + portData = &rule->p.udpHdrFilter.portData; COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); - COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataSrcPortEnd, - rule->p.udpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortStart, - rule->p.udpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortEnd, - rule->p.udpHdrFilter.portData.dataSrcPortStart); + COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart); virNWFilterRuleDefFixupIPSet(&rule->p.udpHdrFilter.ipHdr); break; @@ -2324,16 +2321,14 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) case VIR_NWFILTER_RULE_PROTOCOL_SCTP: case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6: ipHdr = &rule->p.sctpHdrFilter.ipHdr; + portData = &rule->p.sctpHdrFilter.portData; COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr); COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr); COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom); COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataSrcPortEnd, - rule->p.sctpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortStart, - rule->p.sctpHdrFilter.portData.dataSrcPortStart); - COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortEnd, - rule->p.sctpHdrFilter.portData.dataSrcPortStart); + COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart); + COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart); virNWFilterRuleDefFixupIPSet(&rule->p.sctpHdrFilter.ipHdr); break; -- 2.31.1
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616 bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-larger-than=] virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) ^ 1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Why don't we just enlarge stack size limit for building with sanitizers ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616 bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe- larger-than=] virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) ^ 1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Why don't we just enlarge stack size limit for building with sanitizers ?
Regards, Daniel
We already double the stack size limit for debug builds, and increasing it further seemed excessive. Note that there is one more function that exceeds the limit, virDomainDefParseXML, for which I have a patch ready. I will send it once this series lands. Regards, Tim
On Fri, Sep 17, 2021 at 03:37:28PM +0200, Tim Wiederhake wrote:
On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616 bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe- larger-than=] virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) ^ 1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Why don't we just enlarge stack size limit for building with sanitizers ?
Regards, Daniel
We already double the stack size limit for debug builds, and increasing it further seemed excessive.
Note that there is one more function that exceeds the limit, virDomainDefParseXML, for which I have a patch ready. I will send it once this series lands.
We're not using sanitizers in production builds though, so IMHO we could even just run with no stack size checking entirely for such builds. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 2021-09-17 at 15:03 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 03:37:28PM +0200, Tim Wiederhake wrote:
On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616 bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe- larger-than=] virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) ^ 1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Why don't we just enlarge stack size limit for building with sanitizers ?
Regards, Daniel
We already double the stack size limit for debug builds, and increasing it further seemed excessive.
s/debug builds/sanitizer builds/ -- my mistake.
Note that there is one more function that exceeds the limit, virDomainDefParseXML, for which I have a patch ready. I will send it once this series lands.
We're not using sanitizers in production builds though, so IMHO we could even just run with no stack size checking entirely for such builds.
My local builds have "-Dbuildtype=debug". In gitlab's CI, the buildtype is not explictly set. This difference is what triggers the stack frame size warnings: $ export CC=clang $ meson -Db_sanitize=address,undefined -Db_lundef=false buildtype- default &> /dev/null $ ninja -C buildtype-default &>/dev/null && echo "ok" || echo "fail" ok $ meson -Db_sanitize=address,undefined -Db_lundef=false - Dbuildtype=debug buildtype-debug &> /dev/null $ ninja -C buildtype-debug >/dev/null && echo "ok" || echo "fail" fail Which is weird, as meson's default build type is "debug".
When libvirt is build with sanitizers enabled, in debug mode, on clang, virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes, as specified in meson.build:
s/in debug mode/buildtype explicitly set to debug/ Regards, Tim
participants (2)
-
Daniel P. Berrangé -
Tim Wiederhake