Message-ID: <CAK8P3a0ZvZR-rqYqrsx7h5zZBPKUqopipXyApoHNOa5VgiL7UA@mail.gmail.com>
Date:   Fri, 10 Jan 2020 09:04:41 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        PMWG CI <pmwg-ci@...aro.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Private Kernel Alias <private-kwg@...aro.org>,
        Kees Cook <keescook@...omium.org>,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481

On Fri, Jan 10, 2020 at 7:27 AM Stephen Boyd <sboyd@...nel.org> wrote:
> Quoting Arnd Bergmann (2020-01-08 07:13:46)
> > On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins <brendanhiggins@...gle.com> wrote:
> > > On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@...db.de> wrote:
> > > test function, which allocates the object, and then calls the unit
> > > test function with a reference to the object allocation; then we could
> > > just reuse that allocation and we can avoid making a bunch of
> > > piecemeal heap allocations.
> > >
> > > What do people think? Any other ideas?
>
> How about forcing inlining of kunit_do_assertion()? That may allow the
> compiler to remove all the assertion structs and inline the arguments
> from the struct to whatever functions the assertion functions call? It
> may bloat the text size.

I haven't tried it, but I'm fairly sure that would not reliably fix it. The
problem is that the local 'struct kunit_assert' structure escapes to
an extern function call it is passed to by reference. If we inline
kunit_do_assertion(), nothing really changes in that regard as
the compiler still has to construct and initialize that structure on
the stack.

However, the reverse would be possible. Turning
KUNIT_BASE_BINARY_ASSERTION() into an extern
function that takes all the arguments without passing a
structure would solve it. I've prototyped this by changing
KUNIT_BINARY_EQ_ASSERTION() and
KUNIT_BINARY_NE_ASSERTION() like

@@ -651,13 +649,19 @@ do {
                                \
                                    fmt,                                       \
                                    ##__VA_ARGS__)

-#define KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right)             \
+#define __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right)           \
        KUNIT_BINARY_NE_MSG_ASSERTION(test,                                    \
                                      assert_type,                             \
                                      left,                                    \
                                      right,                                   \
                                      NULL)

+static __maybe_unused noinline void KUNIT_BINARY_NE_ASSERTION(struct
kunit *test, int assert_type,
+                                       long long left, long long right)
+{
+       __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right);
+}
+
 #define KUNIT_BINARY_PTR_NE_MSG_ASSERTION(test,
                \
                                          assert_type,                         \
                                          left,                                \


A little more work is needed to make the varargs and
code location passing all work correctly.

> > The idea of annotating it got me thinking about what could be
> > done to improve the structleak plugin, and that in turn got me on
> > the right track to a silly but trivial fix for the issue: The only thing
> > that structleak does here is to initialize the implied padding in
> > the kunit_binary_assert structure. If there is no padding, it all
> > works out find and the structures don't get pinned to the stack
> > because the plugin can simply ignore them.
> >
> > I tried out this patch and it works:
> >
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > index db6a0fca09b4..5b09439fa8ae 100644
> > --- a/include/kunit/assert.h
> > +++ b/include/kunit/assert.h
> > @@ -200,8 +200,9 @@ struct kunit_binary_assert {
> >         struct kunit_assert assert;
> >         const char *operation;
> >         const char *left_text;
> > -       long long left_value;
> >         const char *right_text;
> > +       long __pad;
> > +       long long left_value;
> >         long long right_value;
> >  };
> >
> > There may also be a problem in 'struct kunit_assert' depending on the
> > architecture, if there are any on which the 'enum kunit_assert_type'
> > type is 64 bit wide (which I think is allowed in C, but may not happen
> > on any toolchain that builds kernels).
> >
>
> What does the padding do? This is all magical!

It turned out to not work after all, The change above fixed some of the
cases I saw, but not others.

I'm still struggling to fully understand why the structleak gcc plugin
sometimes forces the structures on the stack and sometimes doesn't.
The idea for the patch above was to avoid implicit padding by making
the padding explicit. What happens with the implicit padding is that
the expanded macro containing code like

struct { const char *left_text; long long left_value; } assert =
    { .left_text  = # _left, .left_value =  _left };
func(&assert);

produces a partially initialized object on a 32-bit architecture, with the
padding between left_text and left_value being old stack data. The
structleak plugin forces this to be initialized to zero, which in turn
forces the structure to be allocated on the stack during the execution
of the function, not just within the surrounding basic block (this
is a known deficiency in structleak).

The theory so far made sense to me, except that as I said above the
padding alone did not fix the problem. :(

        Arnd

Powered by blists - more mailing lists