Re: [PATCH 4/4] check-block: Enable iotests with SafeStack

[Daniele Buono]

On 5/21/2020 5:59 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 29, 2020 at 03:44:20PM -0400, Daniele Buono wrote:
> > SafeStack is a stack protection technique implemented in llvm. It is
> > enabled with a -fsanitize flag.
> > iotests are currently disabled when any -fsanitize option is used.
> > Since SafeStack is useful on production environments, and its
> > implementation may break the binary, filter it out when the check is
> > performed, so that if SafeStack was the only -fsanitize option, iotests
> > are still performed.
>
> I can't parse this sentence. What does "its implementation may break the
> binary" mean? Do you mean it's worth running tests with SafeStack
> enabled because it exposes failures that go unnoticed without SafeStack?

What I meant is that, without proper changes, SafeStack breaks 
co-routines. Since they are heavily used in the io subsystem, this is 
probably the best class of tests to make sure co-routines are working 
fine with SafeStack.

I initially re-enabled the iotests for my internal testing.

Since I wasn't seeing any issue, I thought it would be useful to keep 
running this to make sure future implementations of SafeStack won't 
break co-routines again.

> > Signed-off-by: Daniele Buono <address@hidden>
> > ---
> >   tests/check-block.sh | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/check-block.sh b/tests/check-block.sh
> > index ad320c21ba..8e29c868e5 100755
> > --- a/tests/check-block.sh
> > +++ b/tests/check-block.sh
> > @@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; 
> > then
> >       exit 0
> >   fi
> >   
> > -if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
> > +# Disable tests with any sanitizer except for SafeStack
> > +CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
> > +SANITIZE_FLAGS=""
> > +#Remove all occurrencies of -fsanitize=safe-stack
> > +for i in ${CFLAGS}; do
> > +        if [ "${i}" != "-fsanitize=safe-stack" ]; then
> > +                SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
> > +        fi
> > +done
> > +if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
> > +    # Have a sanitize flag that is not allowed, stop
> >       echo "Sanitizers are enabled ==> Not running the qemu-iotests."
> >       exit 0
> >   fi
>
> The commit that disabled check-block.sh with sanitizers said:
>
>    The sanitizers (especially the address sanitizer from Clang) are
>    sometimes printing out warnings or false positives - this spoils
>    the output of the iotests, causing some of the tests to fail.
>
> It seems fine to allow SafeStack if check-block.sh currently passes with
> it enabled. Does it pass and produce no extra output?
Yes, that was the idea. SafeStack should pass the tests without extra 
output.

It did (pass) on my testing machine. However I don't remember if I did 
the full (slow) check or only the partial one.

Will check again before I submit v2
> Stefan