Reduce stack frame size of virNWFilterRuleDefFixup

[libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup

Posted by Tim Wiederhake 2 years, 7 months ago

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

Re: [libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup

Posted by Daniel P. Berrangé 2 years, 7 months ago

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 :|

Re: [libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup

Posted by Tim Wiederhake 2 years, 7 months ago

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

Re: [libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup

Posted by Daniel P. Berrangé 2 years, 7 months ago

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 :|

Re: [libvirt PATCH 0/3] Reduce stack frame size of virNWFilterRuleDefFixup

Posted by Tim Wiederhake 2 years, 7 months ago

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