Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667740

Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN

Show all headers | View raw

On Fri, Jun 16, 2017 at 02:01:57PM +0200, Arnd Bergmann wrote:
> On Thu, Jun 15, 2017 at 6:53 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jun 15, 2017 at 06:52:21AM +0200, Greg Kroah-Hartman wrote:
> >> On Wed, Jun 14, 2017 at 11:15:38PM +0200, Arnd Bergmann wrote:
> >> > As reported by kernelci, some functions in the VT code use significant
> >> > amounts of kernel stack when local variables get inlined into the caller
> >> > multiple times:
> >> >
> >> > drivers/tty/vt/keyboard.c: In function 'kbd_keycode':
> >> > drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> >> >
> >> > Annotating those functions as noinline_if_stackbloat prevents the inlining
> >> > and reduces the overall stack usage in this driver.
> >> >
> >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> > ---
> >> >  drivers/tty/vt/keyboard.c | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> >> > index f4166263bb3a..c0d111444a0e 100644
> >> > --- a/drivers/tty/vt/keyboard.c
> >> > +++ b/drivers/tty/vt/keyboard.c
> >> > @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt)
> >> >  /*
> >> >   * Helper Functions.
> >> >   */
> >> > -static void put_queue(struct vc_data *vc, int ch)
> >> > +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch)
> >> >  {
> >> >     tty_insert_flip_char(&vc->port, ch, 0);
> >> >     tty_schedule_flip(&vc->port);
> >> >  }
> >>
> >> Ugh, really?  We have to start telling gcc not to be stupid here?
> >> That's not going to be easy, and will just entail us doing this all over
> >> the place, right?
> >>
> >> The code isn't asking to be inlined, so why is gcc allowing it to be
> >> done that way?  Doesn't that imply gcc is the problem here?
> >
> > Wait, you are now, in this patch, _asking_ for it to be inlined.  How is
> > that solving anything?
> 
> The three functions that gain the attribute are all those that gcc decided
> to inline for itself. Usually gcc makes reasonable inlining decisions, so
> I left the existing behavior my marking them as 'inline' without
> CONFIG_KASAN and 'noinline' when KASAN is enabled.

But why should we have to care about this?  If gcc wanted to inline
them, and it did so in a way that blows up the stack, that would be a
gcc bug, right?  Why do I have to tell gcc "don't inline", when really,
I never told it to inline it in the first place?

> Would you rather see this patch instead?
> 
> diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
> index c28dd523f96e..25348c5ffcb7 100644
> --- a/include/linux/tty_flip.h
> +++ b/include/linux/tty_flip.h
> @@ -13,8 +13,8 @@ extern int tty_prepare_flip_string(struct tty_port *port,
>  extern void tty_flip_buffer_push(struct tty_port *port);
>  void tty_schedule_flip(struct tty_port *port);
> 
> -static inline int tty_insert_flip_char(struct tty_port *port,
> -                                       unsigned char ch, char flag)
> +static noinline_if_stackbloat int
> +tty_insert_flip_char(struct tty_port *port, unsigned char ch, char flag)
>  {
>         struct tty_buffer *tb = port->buf.tail;
>         int change;
> 
> This is just as good at eliminating the crazy stack usage in vt/keyboard.o,
> but it will also impact all other users of that function.

How is this function blowing up the stack?  We have 2 variables being
added, that's it.  Are we really that low on stack that 2 words is too
much?

And no, we shouldn't need to do this.  It sounds like ksan is the
problem here...

thanks,

greg k-h

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 00/11] bring back stack frame warning with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:20 +0200
  [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:20 +0200
    Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Samuel Thibault <samuel.thibault@ens-lyon.org> - 2017-06-14 23:30 +0200
      Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-15 00:00 +0200
        Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Samuel Thibault <samuel.thibault@ens-lyon.org> - 2017-06-15 00:20 +0200
    Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-15 07:00 +0200
      Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-15 07:00 +0200
        Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-16 14:10 +0200
          Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-16 15:10 +0200
            Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-16 17:50 +0200
              Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Samuel Thibault <samuel.thibault@ens-lyon.org> - 2017-06-16 18:00 +0200
                Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-16 19:30 +0200
                Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-16 23:00 +0200
                Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-16 23:10 +0200
  [PATCH v2 04/11] rocker: mark rocker_tlv_put_* functions as noinline_if_stackbloat Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:20 +0200
  [PATCH v2 01/11] compiler: introduce noinline_if_stackbloat annotation Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:20 +0200
  [PATCH v2 09/11] brcmsmac: split up wlc_phy_workarounds_nphy Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200
  [PATCH v2 06/11] dvb-frontends: reduce stack size in i2c access Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200
  [PATCH v2 11/11] kasan: rework Kconfig settings Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200
    Re: [PATCH v2 11/11] kasan: rework Kconfig settings Dmitry Vyukov <dvyukov@google.com> - 2017-06-15 09:10 +0200
      Re: [PATCH v2 11/11] kasan: rework Kconfig settings Dmitry Vyukov <dvyukov@google.com> - 2017-06-16 13:50 +0200
      Re: [PATCH v2 11/11] kasan: rework Kconfig settings Arnd Bergmann <arnd@arndb.de> - 2017-06-16 13:50 +0200
  [PATCH v2 08/11] brcmsmac: make some local variables 'static const' to reduce stack size Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200
    Re: [PATCH v2 08/11] brcmsmac: make some local variables 'static const' to reduce stack size Kalle Valo <kvalo@codeaurora.org> - 2017-06-15 17:00 +0200
  [PATCH v2 07/11] r820t: mark register functions as noinline_if_stackbloat Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200
  [PATCH v2 05/11] mtd: cfi: reduce stack size with KASAN Arnd Bergmann <arnd@arndb.de> - 2017-06-14 23:30 +0200

csiph-web