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

Groups > linux.kernel > #1666233 > unrolled thread

[PATCH v2 00/11] bring back stack frame warning with KASAN

Started byArnd Bergmann <arnd@arndb.de>
First post2017-06-14 23:20 +0200
Last post2017-06-14 23:30 +0200
Articles 20 on this page of 26 — 6 participants

Back to article view | Back to linux.kernel

Contents

  [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

Page 1 of 2  [1] 2  Next page →

#1666233 — [PATCH v2 00/11] bring back stack frame warning with KASAN

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:20 +0200
Subject[PATCH v2 00/11] bring back stack frame warning with KASAN
Message-ID<tSnRL-1U4-3@gated-at.bofh.it>
This is a new version of patches I originally submitted back in
March [1], this time reducing the size of the series even further.

This minimal set of patches only makes sure that we do get
frame size warnings in allmodconfig for x86_64 and arm64 again,
even with KASAN enabled.

The changes this time are reduced to:

- I'm introducing "noinline_if_stackbloat" and use it in a number
  of places that suffer from inline functions with local variables
  - netlink, as used in various parts of the kernel
  - a number of drivers/media drivers
  - a handful of wireless network drivers
- a rework for the brcmsmac driver
- -fsanitize-address-use-after-scope is moved to a separate
  CONFIG_KASAN_EXTRA option that increases the warning limit
- CONFIG_KASAN_EXTRA is disabled with CONFIG_COMPILE_TEST,
  improving compile speed and disabling code that leads to
  valid warnings on gcc-7.0.1
- kmemcheck conflicts with CONFIG_KASAN_EXTRA

Compared to the previous version, I no longer have patches
to fix all the CONFIG_KASAN_EXTRA warnings:

- READ_ONCE/WRITE_ONCE cause problems in lots of code
- typecheck() causes huge problems in a few places
- many more uses of noinline_if_stackbloat

This series lets us add back a stack frame warning for the regular
2048 bytes without CONFIG_KASAN_EXTRA. I set the warning limit with
KASAN_EXTRA to 3072, since I have an additional set of patches
to address all files that surpass that limit. We can debate whether
we want to apply those as a follow-up, or instead remove the option
entirely.

Another follow-up series I have reduces the warning limit with
KASAN to 1536, and without KASAN to 1280 for 64-bit architectures.

I hope that Andrew can pick up the entire series for mmotm, and
we can eventually backport most of it to stable kernels and
address the warnings that kernelci still reports for this problem [2].

     Arnd

[1] https://lkml.org/lkml/2017/3/2/508
[2] https://kernelci.org/build/id/593f89a659b51463306b958d/logs/

 kasan: rework Kconfig settings
 brcmsmac: reindent split functions
 brcmsmac: split up wlc_phy_workarounds_nphy
 brcmsmac: make some local variables 'static const' to reduce stack size
 r820t: mark register functions as noinline_if_stackbloat
 dvb-frontends: reduce stack size in i2c access
 mtd: cfi: reduce stack size with KASAN
 rocker: mark rocker_tlv_put_* functions as noinline_if_stackbloat
 tty: kbd: reduce stack size with KASAN
 netlink: mark nla_put_{u8,u16,u32} noinline_if_stackbloat
 compiler: introduce noinline_if_stackbloat annotation

Arnd Bergmann (11):
 drivers/media/dvb-frontends/ascot2e.c                        |    3 +-
 drivers/media/dvb-frontends/cxd2841er.c                      |    4 +-
 drivers/media/dvb-frontends/drx39xyj/drxj.c                  |   14 +-
 drivers/media/dvb-frontends/helene.c                         |    4 +-
 drivers/media/dvb-frontends/horus3a.c                        |    2 +-
 drivers/media/dvb-frontends/itd1000.c                        |    2 +-
 drivers/media/dvb-frontends/mt312.c                          |    2 +-
 drivers/media/dvb-frontends/si2165.c                         |   14 +-
 drivers/media/dvb-frontends/stb0899_drv.c                    |    2 +-
 drivers/media/dvb-frontends/stb6100.c                        |    2 +-
 drivers/media/dvb-frontends/stv0367.c                        |    2 +-
 drivers/media/dvb-frontends/stv090x.c                        |    2 +-
 drivers/media/dvb-frontends/stv6110.c                        |    2 +-
 drivers/media/dvb-frontends/stv6110x.c                       |    2 +-
 drivers/media/dvb-frontends/tda8083.c                        |    2 +-
 drivers/media/dvb-frontends/zl10039.c                        |    2 +-
 drivers/media/tuners/r820t.c                                 |    4 +-
 drivers/mtd/chips/cfi_cmdset_0020.c                          |    8 +-
 drivers/net/ethernet/rocker/rocker_tlv.h                     |   24 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c | 1856 +++++++++++++++++++++++-------------------------
 drivers/tty/vt/keyboard.c                                    |    6 +-
 include/linux/compiler.h                                     |   11 +
 include/linux/mtd/map.h                                      |    8 +-
 include/net/netlink.h                                        |   36 +-
 lib/Kconfig.debug                                            |    4 +-
 lib/Kconfig.kasan                                            |   11 +-
 lib/Kconfig.kmemcheck                                        |    1 +
 scripts/Makefile.kasan                                       |    3 +
 28 files changed, 986 insertions(+), 1047 deletions(-)

-- 
2.9.0

[toc] | [next] | [standalone]

#1666234 — [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:20 +0200
Subject[PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSnRL-1U4-5@gated-at.bofh.it>
In reply to#1666233
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);
 }
 
-static void puts_queue(struct vc_data *vc, char *cp)
+static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp)
 {
 	while (*cp) {
 		tty_insert_flip_char(&vc->port, *cp, 0);
@@ -555,7 +555,7 @@ static void fn_inc_console(struct vc_data *vc)
 	set_console(i);
 }
 
-static void fn_send_intr(struct vc_data *vc)
+static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc)
 {
 	tty_insert_flip_char(&vc->port, 0, TTY_BREAK);
 	tty_schedule_flip(&vc->port);
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

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

FromSamuel Thibault <samuel.thibault@ens-lyon.org>
Date2017-06-14 23:30 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSo1r-1Xi-7@gated-at.bofh.it>
In reply to#1666234
Hello,

Arnd Bergmann, on mer. 14 juin 2017 23:15:38 +0200, 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.


> --- 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);
>  }

I'm surprised that this be able generate so much stack use: the
tty_insert_flip_char inline only uses a pointer and an int.

And I'm surprised that multiple inlines can accumulate stack usage.

I however agree that it's a bad idea to inline it in functions where
it's called so many times (and we're talking about the keyboard anyway).

> -static void puts_queue(struct vc_data *vc, char *cp)
> +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp)

I don't see why, it's only called once in the callers. k_fn, however, is
called several times in k_pad, so that could be why, but then it's
rather be the inlining of k_fn which is a bad idea.

> -static void fn_send_intr(struct vc_data *vc)
> +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc)

This one is only referenced, not called, I don't see how that could pose
problem.

Samuel

[toc] | [prev] | [next] | [standalone]

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

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-15 00:00 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSout-28y-3@gated-at.bofh.it>
In reply to#1666240
On Wed, Jun 14, 2017 at 11:28 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Arnd Bergmann, on mer. 14 juin 2017 23:15:38 +0200, 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.
>
>
>> --- 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);
>>  }
>
> I'm surprised that this be able generate so much stack use: the
> tty_insert_flip_char inline only uses a pointer and an int.
>
> And I'm surprised that multiple inlines can accumulate stack usage.

The reason is that CONFIG_KASAN forces each local variable
to have a separate location on the stack whenever it gets
passed into an external function (tty_insert_flip_string_flags in this
case), so the sanitizer is able to report exactly which instance
caused the problem.

> I however agree that it's a bad idea to inline it in functions where
> it's called so many times (and we're talking about the keyboard anyway).
>
>> -static void puts_queue(struct vc_data *vc, char *cp)
>> +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp)
>
> I don't see why, it's only called once in the callers. k_fn, however, is
> called several times in k_pad, so that could be why, but then it's
> rather be the inlining of k_fn which is a bad idea.

It's called by applkey, which in turn is called by k_pad(), and this
all gets inlined by default.

>> -static void fn_send_intr(struct vc_data *vc)
>> +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc)
>
> This one is only referenced, not called, I don't see how that could pose
> problem.

I was surprised by this as well, but it seems that gcc these days is
smart enough to turn the indirect function calls for k_handler[type]
and/or f_handler[value] into inlines again when it has already
determined the index to be constant.

It's been a while since I looked at the patch, and I'd have to
disassemble it again to figure out the details, but I'm pretty sure
I needed this to get the stack usage down to normal levels.

       Arnd

[toc] | [prev] | [next] | [standalone]

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

FromSamuel Thibault <samuel.thibault@ens-lyon.org>
Date2017-06-15 00:20 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSoNP-2uY-1@gated-at.bofh.it>
In reply to#1666253
Arnd Bergmann, on mer. 14 juin 2017 23:56:39 +0200, wrote:
> > I however agree that it's a bad idea to inline it in functions where
> > it's called so many times (and we're talking about the keyboard anyway).
> >
> >> -static void puts_queue(struct vc_data *vc, char *cp)
> >> +static noinline_if_stackbloat void puts_queue(struct vc_data *vc, char *cp)
> >
> > I don't see why, it's only called once in the callers. k_fn, however, is
> > called several times in k_pad, so that could be why, but then it's
> > rather be the inlining of k_fn which is a bad idea.
> 
> It's called by applkey, which in turn is called by k_pad(),

k_pad calls applkey twice only. Is that really to be considered bloat?

> >> -static void fn_send_intr(struct vc_data *vc)
> >> +static noinline_if_stackbloat void fn_send_intr(struct vc_data *vc)
> >
> > This one is only referenced, not called, I don't see how that could pose
> > problem.
> 
> I was surprised by this as well, but it seems that gcc these days is
> smart enough to turn the indirect function calls for k_handler[type]
> and/or f_handler[value] into inlines again when it has already
> determined the index to be constant.

Cool :) But I don't see how it can see find it out constant. The only
fn_handler[] caller is k_spec, using value as index. The only caller of
f_handler[] is kbd_keycode, using type as index, and keysym&0xff as
value.  That is definitely not constant :)  And it's only one caller, I
don't see how that can bloat.

Samuel

[toc] | [prev] | [next] | [standalone]

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

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2017-06-15 07:00 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSv2V-6ml-3@gated-at.bofh.it>
In reply to#1666234
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?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

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

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2017-06-15 07:00 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSv2V-6ml-5@gated-at.bofh.it>
In reply to#1666451
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?

greg k-h

[toc] | [prev] | [next] | [standalone]

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

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-16 14:10 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSYeB-8fF-5@gated-at.bofh.it>
In reply to#1666453
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.

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.

        Arnd

[toc] | [prev] | [next] | [standalone]

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

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2017-06-16 15:10 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tSZaG-s6-9@gated-at.bofh.it>
In reply to#1667683
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

[toc] | [prev] | [next] | [standalone]

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

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-16 17:50 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tT1Fv-1ZE-9@gated-at.bofh.it>
In reply to#1667740
On Fri, Jun 16, 2017 at 3:02 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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:
>> >> > -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?

I don't think gcc takes stack size into account when making the inlining
decisions. Without the address sanitizer, inlining won't normally have
any negative effects on the overall stack size, and may even help save
a few bytes for the caller-saved registers.

You could argue that the gcc inlining algorithm is buggy in combination
with kasan, but what does that help you? In most instances of this
problem, we actually force the inlining (see the other patches in this
series), so making gcc smarter would not help much either.

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

The 'tb' and 'change' variables don't hurt here, they just get optimized
away. The problem are the 'ch' and 'flag' variables that are passed into
tty_insert_flip_char by value, and from there into
tty_insert_flip_string_flags by reference.  In this case, kasan tries
to detect whether tty_insert_flip_string_flags() does any out-of-bounds
access on the pointers and adds 64 bytes redzone around each of
the two variables.

gcc-6.3.1 happens to inline 16 calls of tty_insert_flip_char() into
kbd_keycode(), so the stack size grows from 168 bytes to
168+(16*2*64) = 2216 bytes. There are 10 calls to put_queue()
in to_utf8(), 12 in emulate_raw() and another 4 in kbd_keycode()
itself. On ARM64, it happens to decide differently and presumably
doesn't inline tty_insert_flip_char() and kbd_keycode() into
kbd_keycode(), so the maximum stack size isn't as bad, but
the problem still exists.

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

Of course kasan is the problem, but it really just does whatever we
asked it to do, and cannot do any better as long as we inline many
copies of tty_insert_flip_char() into kbd_keycode().

       Arnd

[toc] | [prev] | [next] | [standalone]

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

FromSamuel Thibault <samuel.thibault@ens-lyon.org>
Date2017-06-16 18:00 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tT1Pb-23z-1@gated-at.bofh.it>
In reply to#1667841
Arnd Bergmann, on ven. 16 juin 2017 17:41:47 +0200, wrote:
> The problem are the 'ch' and 'flag' variables that are passed into
> tty_insert_flip_char by value, and from there into
> tty_insert_flip_string_flags by reference.  In this case, kasan tries
> to detect whether tty_insert_flip_string_flags() does any out-of-bounds
> access on the pointers and adds 64 bytes redzone around each of
> the two variables.

Ouch.

> gcc-6.3.1 happens to inline 16 calls of tty_insert_flip_char() into
> kbd_keycode(), so the stack size grows from 168 bytes to
> 168+(16*2*64) = 2216 bytes. There are 10 calls to put_queue()
> in to_utf8(), 12 in emulate_raw() and another 4 in kbd_keycode()
> itself.

That's why I agreed for put_queue :)

I'm however afraid we'd have to mark a lot of static functions that way,
depending on the aggressivity of gcc... I'd indeed really argue that gcc
should consider stack usage when inlining.

static int f(int foo) {
	char c[256];
	g(c, foo);
}

is really not something that I'd want to see the compiler to inline.

> > And no, we shouldn't need to do this.  It sounds like ksan is the
> > problem here...
> 
> Of course kasan is the problem, but it really just does whatever we
> asked it to do, and cannot do any better as long as we inline many
> copies of tty_insert_flip_char() into kbd_keycode().

We didn't ask to inline put_queue into kbd_keycode.

Samuel

[toc] | [prev] | [next] | [standalone]

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

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2017-06-16 19:30 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tT3ei-393-13@gated-at.bofh.it>
In reply to#1667842
On Fri, Jun 16, 2017 at 8:58 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Arnd Bergmann, on ven. 16 juin 2017 17:41:47 +0200, wrote:
>> The problem are the 'ch' and 'flag' variables that are passed into
>> tty_insert_flip_char by value, and from there into
>> tty_insert_flip_string_flags by reference.  In this case, kasan tries
>> to detect whether tty_insert_flip_string_flags() does any out-of-bounds
>> access on the pointers and adds 64 bytes redzone around each of
>> the two variables.
>
> Ouch.
>
>> gcc-6.3.1 happens to inline 16 calls of tty_insert_flip_char() into

I wonder if we should stop marking tty_insert_flip_char() as inline.

>> kbd_keycode(), so the stack size grows from 168 bytes to
>> 168+(16*2*64) = 2216 bytes. There are 10 calls to put_queue()
>> in to_utf8(), 12 in emulate_raw() and another 4 in kbd_keycode()
>> itself.
>
> That's why I agreed for put_queue :)
>
> I'm however afraid we'd have to mark a lot of static functions that way,
> depending on the aggressivity of gcc... I'd indeed really argue that gcc
> should consider stack usage when inlining.
>
> static int f(int foo) {
>         char c[256];
>         g(c, foo);
> }
>
> is really not something that I'd want to see the compiler to inline.

Why would not we want it be inlined? What we do not want us several
calls having _separate_ instances of 'c' generated on the stack, all
inlined calls should share 'c'. And of course if we have f1, f2, and
f3 with c1, c2, and c3, GCC should not blow up the stack inlining and
allocating stack for all 3 of them beforehand.

But this all seems to me issue that should be solved in toolchain, not
trying to play whack-a-mole with kernel sources.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

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

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-16 23:00 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tT6vv-5g1-1@gated-at.bofh.it>
In reply to#1667914
On Fri, Jun 16, 2017 at 7:29 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jun 16, 2017 at 8:58 AM, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
>> Arnd Bergmann, on ven. 16 juin 2017 17:41:47 +0200, wrote:
>>> The problem are the 'ch' and 'flag' variables that are passed into
>>> tty_insert_flip_char by value, and from there into
>>> tty_insert_flip_string_flags by reference.  In this case, kasan tries
>>> to detect whether tty_insert_flip_string_flags() does any out-of-bounds
>>> access on the pointers and adds 64 bytes redzone around each of
>>> the two variables.
>>
>> Ouch.
>>
>>> gcc-6.3.1 happens to inline 16 calls of tty_insert_flip_char() into
>
> I wonder if we should stop marking tty_insert_flip_char() as inline.

That would be an easy solution, yes. tty_insert_flip_char() was
apparently meant to be optimized for the fast path to completely
avoid calling into another function, but that fast path got a bit more
complex with commit acc0f67f307f ("tty: Halve flip buffer
GFP_ATOMIC memory consumption").

If we move it out of line, the fast path optimization goes away and
we could just have a simple implementation like


int tty_insert_flip_char(struct tty_port *port, unsigned char ch, char flag)
{
        struct tty_buffer *tb = port->buf.tail;
        int flags = (flag == TTY_NORMAL) ? TTYB_NORMAL : 0;

        if (!__tty_buffer_request_room(port, 1, flags))
                return 0;

        if (~tb->flags & TTYB_NORMAL)
                *flag_buf_ptr(tb, tb->used) = flag;
        *char_buf_ptr(tb, tb->used++) = ch;

        return 1;
}

One rather simple change I found would actually avoid the warning
and would seem to actually give us better runtime behavior even
without KASAN:

diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index c28dd523f96e..15d03a14ad0f 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -26,7 +26,7 @@ static inline int tty_insert_flip_char(struct tty_port *port,
                *char_buf_ptr(tb, tb->used++) = ch;
                return 1;
        }
-       return tty_insert_flip_string_flags(port, &ch, &flag, 1);
+       return tty_insert_flip_string_fixed_flag(port, &ch, flag, 1);
 }

 static inline int tty_insert_flip_string(struct tty_port *port,

This reduces the stack frame size for kbd_event() to 1256 bytes,
which is well within the limit, and it lets us keep the flag-less
buffers across a 'tb->used >= tb->size' condition. Calling
into tty_insert_flip_string_flags() today will allocate a flag buffer
if there isn't already one, even when it is not needed.

>> I'm however afraid we'd have to mark a lot of static functions that way,
>> depending on the aggressivity of gcc... I'd indeed really argue that gcc
>> should consider stack usage when inlining.
>>
>> static int f(int foo) {
>>         char c[256];
>>         g(c, foo);
>> }
>>
>> is really not something that I'd want to see the compiler to inline.
>
> Why would not we want it be inlined? What we do not want us several
> calls having _separate_ instances of 'c' generated on the stack, all
> inlined calls should share 'c'. And of course if we have f1, f2, and
> f3 with c1, c2, and c3, GCC should not blow up the stack inlining and
> allocating stack for all 3 of them beforehand.
>
> But this all seems to me issue that should be solved in toolchain, not
> trying to play whack-a-mole with kernel sources.

The problem for the Samuel's example is that

a) the "--param asan-stack=1" option in KASAN does blow up the
   stack, which is why the annotation is now called 'noinline_if_stackbloat'.

b) The toolchain cannot solve the problem, as most instances of the
   problem (unlike kbd_put_queue) force the inlining unless you build
   with the x86-specific CONFIG_OPTIMIZE_INLINING.

        Arnd

[toc] | [prev] | [next] | [standalone]

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

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2017-06-16 23:10 +0200
SubjectRe: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN
Message-ID<tT6Fb-5za-1@gated-at.bofh.it>
In reply to#1668056
On Fri, Jun 16, 2017 at 1:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 16, 2017 at 7:29 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Fri, Jun 16, 2017 at 8:58 AM, Samuel Thibault
>> <samuel.thibault@ens-lyon.org> wrote:
>>> I'm however afraid we'd have to mark a lot of static functions that way,
>>> depending on the aggressivity of gcc... I'd indeed really argue that gcc
>>> should consider stack usage when inlining.
>>>
>>> static int f(int foo) {
>>>         char c[256];
>>>         g(c, foo);
>>> }
>>>
>>> is really not something that I'd want to see the compiler to inline.
>>
>> Why would not we want it be inlined? What we do not want us several
>> calls having _separate_ instances of 'c' generated on the stack, all
>> inlined calls should share 'c'. And of course if we have f1, f2, and
>> f3 with c1, c2, and c3, GCC should not blow up the stack inlining and
>> allocating stack for all 3 of them beforehand.
>>
>> But this all seems to me issue that should be solved in toolchain, not
>> trying to play whack-a-mole with kernel sources.
>
> The problem for the Samuel's example is that
>
> a) the "--param asan-stack=1" option in KASAN does blow up the
>    stack, which is why the annotation is now called 'noinline_if_stackbloat'.
>
> b) The toolchain cannot solve the problem, as most instances of the
>    problem (unlike kbd_put_queue) force the inlining unless you build
>    with the x86-specific CONFIG_OPTIMIZE_INLINING.

If inlining done right there should be no change in stack size,
because if calls are not inlined then stack storage is "shared"
between calls, and it should similarly be shared when calls are
inlined. And that is toolchain issue.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1666235 — [PATCH v2 04/11] rocker: mark rocker_tlv_put_* functions as noinline_if_stackbloat

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:20 +0200
Subject[PATCH v2 04/11] rocker: mark rocker_tlv_put_* functions as noinline_if_stackbloat
Message-ID<tSnRL-1U4-7@gated-at.bofh.it>
In reply to#1666233
Inlining these functions creates lots of stack variables when KASAN is
enabled, leading to this warning about potential stack overflow:

drivers/net/ethernet/rocker/rocker_ofdpa.c: In function 'ofdpa_cmd_flow_tbl_add':
drivers/net/ethernet/rocker/rocker_ofdpa.c:621:1: error: the frame size of 2752 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

This marks all of them noinline_if_stackbloat, which solves the problem by
keeping the redzone inside of the separate stack frames.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/rocker/rocker_tlv.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker_tlv.h b/drivers/net/ethernet/rocker/rocker_tlv.h
index a63ef82e7c72..8970a414eb5b 100644
--- a/drivers/net/ethernet/rocker/rocker_tlv.h
+++ b/drivers/net/ethernet/rocker/rocker_tlv.h
@@ -139,38 +139,38 @@ rocker_tlv_start(struct rocker_desc_info *desc_info)
 int rocker_tlv_put(struct rocker_desc_info *desc_info,
 		   int attrtype, int attrlen, const void *data);
 
-static inline int rocker_tlv_put_u8(struct rocker_desc_info *desc_info,
-				    int attrtype, u8 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_u8(struct rocker_desc_info *desc_info, int attrtype, u8 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(u8), &value);
 }
 
-static inline int rocker_tlv_put_u16(struct rocker_desc_info *desc_info,
-				     int attrtype, u16 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_u16(struct rocker_desc_info *desc_info, int attrtype, u16 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(u16), &value);
 }
 
-static inline int rocker_tlv_put_be16(struct rocker_desc_info *desc_info,
-				      int attrtype, __be16 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_be16(struct rocker_desc_info *desc_info, int attrtype, __be16 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(__be16), &value);
 }
 
-static inline int rocker_tlv_put_u32(struct rocker_desc_info *desc_info,
-				     int attrtype, u32 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_u32(struct rocker_desc_info *desc_info, int attrtype, u32 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(u32), &value);
 }
 
-static inline int rocker_tlv_put_be32(struct rocker_desc_info *desc_info,
-				      int attrtype, __be32 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_be32(struct rocker_desc_info *desc_info, int attrtype, __be32 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(__be32), &value);
 }
 
-static inline int rocker_tlv_put_u64(struct rocker_desc_info *desc_info,
-				     int attrtype, u64 value)
+static noinline_if_stackbloat int
+rocker_tlv_put_u64(struct rocker_desc_info *desc_info, int attrtype, u64 value)
 {
 	return rocker_tlv_put(desc_info, attrtype, sizeof(u64), &value);
 }
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1666236 — [PATCH v2 01/11] compiler: introduce noinline_if_stackbloat annotation

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:20 +0200
Subject[PATCH v2 01/11] compiler: introduce noinline_if_stackbloat annotation
Message-ID<tSnRL-1U4-11@gated-at.bofh.it>
In reply to#1666233
When CONFIG_KASAN is set, we can run into some code that uses incredible
amounts of kernel stack:

drivers/staging/dgnc/dgnc_neo.c:1056:1: error: the frame size of 11112 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
drivers/media/i2c/cx25840/cx25840-core.c:4960:1: error: the frame size of 94000 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
drivers/media/dvb-frontends/stv090x.c:3430:1: error: the frame size of 5312 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]

This happens when a sanitizer uses stack memory each time an inline function
gets called. This introduces a new annotation for those functions to make
them either 'inline' or 'noinline' depending on the CONFIG_KASAN symbol.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/compiler.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 219f82f3ec1a..a402c43c07d2 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -412,6 +412,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 #define noinline_for_stack noinline
 
+/*
+ * CONFIG_KASAN can lead to extreme stack usage with certain patterns when
+ * one function gets inlined many times and each instance requires a stack
+ * ckeck.
+ */
+#ifdef CONFIG_KASAN
+#define noinline_if_stackbloat noinline __maybe_unused
+#else
+#define noinline_if_stackbloat inline
+#endif
+
 #ifndef __always_inline
 #define __always_inline inline
 #endif
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1666238 — [PATCH v2 09/11] brcmsmac: split up wlc_phy_workarounds_nphy

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:30 +0200
Subject[PATCH v2 09/11] brcmsmac: split up wlc_phy_workarounds_nphy
Message-ID<tSo1r-1Xi-1@gated-at.bofh.it>
In reply to#1666233
The stack consumption in this driver is still relatively high, with one
remaining warning if the warning level is lowered to 1536 bytes:

drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:17135:1: error: the frame size of 1880 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

The affected function is actually a collection of three separate implementations,
and each of them is fairly large by itself. Splitting them up is done easily
and improves readability at the same time.

I'm leaving the original indentation to make the review easier.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 .../broadcom/brcm80211/brcmsmac/phy/phy_n.c        | 178 ++++++++++++---------
 1 file changed, 104 insertions(+), 74 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
index ef685465f80a..ed409a80f3d2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
@@ -16061,52 +16061,8 @@ static void wlc_phy_workarounds_nphy_gainctrl(struct brcms_phy *pi)
 	}
 }
 
-static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
+static void wlc_phy_workarounds_nphy_rev7(struct brcms_phy *pi)
 {
-	static const u8 rfseq_rx2tx_events[] = {
-		NPHY_RFSEQ_CMD_NOP,
-		NPHY_RFSEQ_CMD_RXG_FBW,
-		NPHY_RFSEQ_CMD_TR_SWITCH,
-		NPHY_RFSEQ_CMD_CLR_HIQ_DIS,
-		NPHY_RFSEQ_CMD_RXPD_TXPD,
-		NPHY_RFSEQ_CMD_TX_GAIN,
-		NPHY_RFSEQ_CMD_EXT_PA
-	};
-	u8 rfseq_rx2tx_dlys[] = { 8, 6, 6, 2, 4, 60, 1 };
-	static const u8 rfseq_tx2rx_events[] = {
-		NPHY_RFSEQ_CMD_NOP,
-		NPHY_RFSEQ_CMD_EXT_PA,
-		NPHY_RFSEQ_CMD_TX_GAIN,
-		NPHY_RFSEQ_CMD_RXPD_TXPD,
-		NPHY_RFSEQ_CMD_TR_SWITCH,
-		NPHY_RFSEQ_CMD_RXG_FBW,
-		NPHY_RFSEQ_CMD_CLR_HIQ_DIS
-	};
-	static const u8 rfseq_tx2rx_dlys[] = { 8, 6, 2, 4, 4, 6, 1 };
-	static const u8 rfseq_tx2rx_events_rev3[] = {
-		NPHY_REV3_RFSEQ_CMD_EXT_PA,
-		NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
-		NPHY_REV3_RFSEQ_CMD_TX_GAIN,
-		NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
-		NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
-		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
-		NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
-		NPHY_REV3_RFSEQ_CMD_END
-	};
-	static const u8 rfseq_tx2rx_dlys_rev3[] = { 8, 4, 2, 2, 4, 4, 6, 1 };
-	u8 rfseq_rx2tx_events_rev3[] = {
-		NPHY_REV3_RFSEQ_CMD_NOP,
-		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
-		NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
-		NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
-		NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
-		NPHY_REV3_RFSEQ_CMD_TX_GAIN,
-		NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
-		NPHY_REV3_RFSEQ_CMD_EXT_PA,
-		NPHY_REV3_RFSEQ_CMD_END
-	};
-	u8 rfseq_rx2tx_dlys_rev3[] = { 8, 6, 6, 4, 4, 18, 42, 1, 1 };
-
 	static const u8 rfseq_rx2tx_events_rev3_ipa[] = {
 		NPHY_REV3_RFSEQ_CMD_NOP,
 		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
@@ -16120,29 +16076,15 @@ static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
 	};
 	static const u8 rfseq_rx2tx_dlys_rev3_ipa[] = { 8, 6, 6, 4, 4, 16, 43, 1, 1 };
 	static const u16 rfseq_rx2tx_dacbufpu_rev7[] = { 0x10f, 0x10f };
-
-	s16 alpha0, alpha1, alpha2;
-	s16 beta0, beta1, beta2;
-	u32 leg_data_weights, ht_data_weights, nss1_data_weights,
-	    stbc_data_weights;
+	u32 leg_data_weights;
 	u8 chan_freq_range = 0;
 	static const u16 dac_control = 0x0002;
 	u16 aux_adc_vmid_rev7_core0[] = { 0x8e, 0x96, 0x96, 0x96 };
 	u16 aux_adc_vmid_rev7_core1[] = { 0x8f, 0x9f, 0x9f, 0x96 };
-	u16 aux_adc_vmid_rev4[] = { 0xa2, 0xb4, 0xb4, 0x89 };
-	u16 aux_adc_vmid_rev3[] = { 0xa2, 0xb4, 0xb4, 0x89 };
-	u16 *aux_adc_vmid;
 	u16 aux_adc_gain_rev7[] = { 0x02, 0x02, 0x02, 0x02 };
-	u16 aux_adc_gain_rev4[] = { 0x02, 0x02, 0x02, 0x00 };
-	u16 aux_adc_gain_rev3[] = { 0x02, 0x02, 0x02, 0x00 };
-	u16 *aux_adc_gain;
-	static const u16 sk_adc_vmid[] = { 0xb4, 0xb4, 0xb4, 0x24 };
-	static const u16 sk_adc_gain[] = { 0x02, 0x02, 0x02, 0x02 };
 	s32 min_nvar_val = 0x18d;
 	s32 min_nvar_offset_6mbps = 20;
 	u8 pdetrange;
-	u8 triso;
-	u16 regval;
 	u16 afectrl_adc_ctrl1_rev7 = 0x20;
 	u16 afectrl_adc_ctrl2_rev7 = 0x0;
 	u16 rfseq_rx2tx_lpf_h_hpc_rev7 = 0x77;
@@ -16171,17 +16113,6 @@ static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
 	u16 freq;
 	int coreNum;
 
-	if (CHSPEC_IS5G(pi->radio_chanspec))
-		wlc_phy_classifier_nphy(pi, NPHY_ClassifierCtrl_cck_en, 0);
-	else
-		wlc_phy_classifier_nphy(pi, NPHY_ClassifierCtrl_cck_en, 1);
-
-	if (pi->phyhang_avoid)
-		wlc_phy_stay_in_carriersearch_nphy(pi, true);
-
-	or_phy_reg(pi, 0xb1, NPHY_IQFlip_ADC1 | NPHY_IQFlip_ADC2);
-
-	if (NREV_GE(pi->pubpi.phy_rev, 7)) {
 
 		if (NREV_IS(pi->pubpi.phy_rev, 7)) {
 			mod_phy_reg(pi, 0x221, (0x1 << 4), (1 << 4));
@@ -16703,8 +16634,62 @@ static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
 					 &aux_adc_gain_rev7);
 		wlc_phy_table_write_nphy(pi, NPHY_TBL_ID_AFECTRL, 4, 0x1c, 16,
 					 &aux_adc_gain_rev7);
+}
 
-	} else if (NREV_GE(pi->pubpi.phy_rev, 3)) {
+static void wlc_phy_workarounds_nphy_rev3(struct brcms_phy *pi)
+{
+	static const u8 rfseq_tx2rx_events_rev3[] = {
+		NPHY_REV3_RFSEQ_CMD_EXT_PA,
+		NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
+		NPHY_REV3_RFSEQ_CMD_TX_GAIN,
+		NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
+		NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
+		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
+		NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
+		NPHY_REV3_RFSEQ_CMD_END
+	};
+	static const u8 rfseq_tx2rx_dlys_rev3[] = { 8, 4, 2, 2, 4, 4, 6, 1 };
+	u8 rfseq_rx2tx_events_rev3[] = {
+		NPHY_REV3_RFSEQ_CMD_NOP,
+		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
+		NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
+		NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
+		NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
+		NPHY_REV3_RFSEQ_CMD_TX_GAIN,
+		NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
+		NPHY_REV3_RFSEQ_CMD_EXT_PA,
+		NPHY_REV3_RFSEQ_CMD_END
+	};
+	u8 rfseq_rx2tx_dlys_rev3[] = { 8, 6, 6, 4, 4, 18, 42, 1, 1 };
+	static const u8 rfseq_rx2tx_events_rev3_ipa[] = {
+		NPHY_REV3_RFSEQ_CMD_NOP,
+		NPHY_REV3_RFSEQ_CMD_RXG_FBW,
+		NPHY_REV3_RFSEQ_CMD_TR_SWITCH,
+		NPHY_REV3_RFSEQ_CMD_CLR_HIQ_DIS,
+		NPHY_REV3_RFSEQ_CMD_RXPD_TXPD,
+		NPHY_REV3_RFSEQ_CMD_TX_GAIN,
+		NPHY_REV3_RFSEQ_CMD_CLR_RXRX_BIAS,
+		NPHY_REV3_RFSEQ_CMD_INT_PA_PU,
+		NPHY_REV3_RFSEQ_CMD_END
+	};
+	static const u8 rfseq_rx2tx_dlys_rev3_ipa[] = { 8, 6, 6, 4, 4, 16, 43, 1, 1 };
+	s16 alpha0, alpha1, alpha2;
+	s16 beta0, beta1, beta2;
+	u32 leg_data_weights, ht_data_weights, nss1_data_weights,
+	    stbc_data_weights;
+	u8 chan_freq_range = 0;
+	static const u16 dac_control = 0x0002;
+	u16 aux_adc_vmid_rev4[] = { 0xa2, 0xb4, 0xb4, 0x89 };
+	u16 aux_adc_vmid_rev3[] = { 0xa2, 0xb4, 0xb4, 0x89 };
+	u16 *aux_adc_vmid;
+	u16 aux_adc_gain_rev4[] = { 0x02, 0x02, 0x02, 0x00 };
+	u16 aux_adc_gain_rev3[] = { 0x02, 0x02, 0x02, 0x00 };
+	u16 *aux_adc_gain;
+	static const u16 sk_adc_vmid[] = { 0xb4, 0xb4, 0xb4, 0x24 };
+	static const u16 sk_adc_gain[] = { 0x02, 0x02, 0x02, 0x02 };
+	s32 min_nvar_val = 0x18d;
+	u8 pdetrange;
+	u8 triso;
 
 		write_phy_reg(pi, 0x23f, 0x1f8);
 		write_phy_reg(pi, 0x240, 0x1f8);
@@ -17030,7 +17015,33 @@ static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
 					      MHF4_BPHY_TXCORE0,
 					      MHF4_BPHY_TXCORE0, BRCM_BAND_ALL);
 		}
-	} else {
+}
+
+void wlc_phy_workarounds_nphy_rev1(struct brcms_phy *pi)
+{
+	static const u8 rfseq_rx2tx_events[] = {
+		NPHY_RFSEQ_CMD_NOP,
+		NPHY_RFSEQ_CMD_RXG_FBW,
+		NPHY_RFSEQ_CMD_TR_SWITCH,
+		NPHY_RFSEQ_CMD_CLR_HIQ_DIS,
+		NPHY_RFSEQ_CMD_RXPD_TXPD,
+		NPHY_RFSEQ_CMD_TX_GAIN,
+		NPHY_RFSEQ_CMD_EXT_PA
+	};
+	u8 rfseq_rx2tx_dlys[] = { 8, 6, 6, 2, 4, 60, 1 };
+	static const u8 rfseq_tx2rx_events[] = {
+		NPHY_RFSEQ_CMD_NOP,
+		NPHY_RFSEQ_CMD_EXT_PA,
+		NPHY_RFSEQ_CMD_TX_GAIN,
+		NPHY_RFSEQ_CMD_RXPD_TXPD,
+		NPHY_RFSEQ_CMD_TR_SWITCH,
+		NPHY_RFSEQ_CMD_RXG_FBW,
+		NPHY_RFSEQ_CMD_CLR_HIQ_DIS
+	};
+	static const u8 rfseq_tx2rx_dlys[] = { 8, 6, 2, 4, 4, 6, 1 };
+	s16 alpha0, alpha1, alpha2;
+	s16 beta0, beta1, beta2;
+	u16 regval;
 
 		if (pi->sh->boardflags2 & BFL2_SKWRKFEM_BRD ||
 		    (pi->sh->boardtype == 0x8b)) {
@@ -17128,7 +17139,26 @@ static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
 			mod_phy_reg(pi, 0x221,
 				    NPHY_FORCESIG_DECODEGATEDCLKS,
 				    NPHY_FORCESIG_DECODEGATEDCLKS);
-	}
+}
+
+static void wlc_phy_workarounds_nphy(struct brcms_phy *pi)
+{
+	if (CHSPEC_IS5G(pi->radio_chanspec))
+		wlc_phy_classifier_nphy(pi, NPHY_ClassifierCtrl_cck_en, 0);
+	else
+		wlc_phy_classifier_nphy(pi, NPHY_ClassifierCtrl_cck_en, 1);
+
+	if (pi->phyhang_avoid)
+		wlc_phy_stay_in_carriersearch_nphy(pi, true);
+
+	or_phy_reg(pi, 0xb1, NPHY_IQFlip_ADC1 | NPHY_IQFlip_ADC2);
+
+	if (NREV_GE(pi->pubpi.phy_rev, 7))
+		wlc_phy_workarounds_nphy_rev7(pi);
+	else if (NREV_GE(pi->pubpi.phy_rev, 3))
+		wlc_phy_workarounds_nphy_rev3(pi);
+	else
+		wlc_phy_workarounds_nphy_rev1(pi);
 
 	if (pi->phyhang_avoid)
 		wlc_phy_stay_in_carriersearch_nphy(pi, false);
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1666239 — [PATCH v2 06/11] dvb-frontends: reduce stack size in i2c access

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:30 +0200
Subject[PATCH v2 06/11] dvb-frontends: reduce stack size in i2c access
Message-ID<tSo1r-1Xi-3@gated-at.bofh.it>
In reply to#1666233
A typical code fragment was copied across many dvb-frontend
drivers and causes large stack frames when built with
-fsanitize-address-use-after-scope, e.g.

drivers/media/dvb-frontends/cxd2841er.c:3225:1: error: the frame size of 3992 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
drivers/media/dvb-frontends/cxd2841er.c:3404:1: error: the frame size of 3136 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
drivers/media/dvb-frontends/stv0367.c:3143:1: error: the frame size of 4016 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
drivers/media/dvb-frontends/stv090x.c:3430:1: error: the frame size of 5312 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
drivers/media/dvb-frontends/stv090x.c:4248:1: error: the frame size of 4872 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]

By marking the register access functions as noinline_if_stackbloat,
we can completely avoid this problem.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/dvb-frontends/ascot2e.c       |  3 ++-
 drivers/media/dvb-frontends/cxd2841er.c     |  4 ++--
 drivers/media/dvb-frontends/drx39xyj/drxj.c | 14 +++++++-------
 drivers/media/dvb-frontends/helene.c        |  4 ++--
 drivers/media/dvb-frontends/horus3a.c       |  2 +-
 drivers/media/dvb-frontends/itd1000.c       |  2 +-
 drivers/media/dvb-frontends/mt312.c         |  2 +-
 drivers/media/dvb-frontends/si2165.c        | 14 +++++++-------
 drivers/media/dvb-frontends/stb0899_drv.c   |  2 +-
 drivers/media/dvb-frontends/stb6100.c       |  2 +-
 drivers/media/dvb-frontends/stv0367.c       |  2 +-
 drivers/media/dvb-frontends/stv090x.c       |  2 +-
 drivers/media/dvb-frontends/stv6110.c       |  2 +-
 drivers/media/dvb-frontends/stv6110x.c      |  2 +-
 drivers/media/dvb-frontends/tda8083.c       |  2 +-
 drivers/media/dvb-frontends/zl10039.c       |  2 +-
 16 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/media/dvb-frontends/ascot2e.c b/drivers/media/dvb-frontends/ascot2e.c
index 0ee0df53b91b..da1d1fc03c5e 100644
--- a/drivers/media/dvb-frontends/ascot2e.c
+++ b/drivers/media/dvb-frontends/ascot2e.c
@@ -153,7 +153,8 @@ static int ascot2e_write_regs(struct ascot2e_priv *priv,
 	return 0;
 }
 
-static int ascot2e_write_reg(struct ascot2e_priv *priv, u8 reg, u8 val)
+static noinline_if_stackbloat int ascot2e_write_reg(struct ascot2e_priv *priv,
+						u8 reg, u8 val)
 {
 	return ascot2e_write_regs(priv, reg, &val, 1);
 }
diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c
index ce37dc2e89c7..6b851a948ce0 100644
--- a/drivers/media/dvb-frontends/cxd2841er.c
+++ b/drivers/media/dvb-frontends/cxd2841er.c
@@ -258,7 +258,7 @@ static int cxd2841er_write_regs(struct cxd2841er_priv *priv,
 	return 0;
 }
 
-static int cxd2841er_write_reg(struct cxd2841er_priv *priv,
+static noinline_if_stackbloat int cxd2841er_write_reg(struct cxd2841er_priv *priv,
 			       u8 addr, u8 reg, u8 val)
 {
 	return cxd2841er_write_regs(priv, addr, reg, &val, 1);
@@ -306,7 +306,7 @@ static int cxd2841er_read_regs(struct cxd2841er_priv *priv,
 	return 0;
 }
 
-static int cxd2841er_read_reg(struct cxd2841er_priv *priv,
+static noinline_if_stackbloat int cxd2841er_read_reg(struct cxd2841er_priv *priv,
 			      u8 addr, u8 reg, u8 *val)
 {
 	return cxd2841er_read_regs(priv, addr, reg, val, 1);
diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c b/drivers/media/dvb-frontends/drx39xyj/drxj.c
index 14040c915dbb..ec5b13ca630b 100644
--- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
+++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
@@ -1516,7 +1516,7 @@ static int drxdap_fasi_read_block(struct i2c_device_addr *dev_addr,
 *
 ******************************/
 
-static int drxdap_fasi_read_reg16(struct i2c_device_addr *dev_addr,
+static noinline_if_stackbloat int drxdap_fasi_read_reg16(struct i2c_device_addr *dev_addr,
 					 u32 addr,
 					 u16 *data, u32 flags)
 {
@@ -1549,7 +1549,7 @@ static int drxdap_fasi_read_reg16(struct i2c_device_addr *dev_addr,
 *
 ******************************/
 
-static int drxdap_fasi_read_reg32(struct i2c_device_addr *dev_addr,
+static noinline_if_stackbloat int drxdap_fasi_read_reg32(struct i2c_device_addr *dev_addr,
 					 u32 addr,
 					 u32 *data, u32 flags)
 {
@@ -1722,7 +1722,7 @@ static int drxdap_fasi_write_block(struct i2c_device_addr *dev_addr,
 *
 ******************************/
 
-static int drxdap_fasi_write_reg16(struct i2c_device_addr *dev_addr,
+static noinline_if_stackbloat int drxdap_fasi_write_reg16(struct i2c_device_addr *dev_addr,
 					  u32 addr,
 					  u16 data, u32 flags)
 {
@@ -1795,7 +1795,7 @@ static int drxdap_fasi_read_modify_write_reg16(struct i2c_device_addr *dev_addr,
 *
 ******************************/
 
-static int drxdap_fasi_write_reg32(struct i2c_device_addr *dev_addr,
+static noinline_if_stackbloat int drxdap_fasi_write_reg32(struct i2c_device_addr *dev_addr,
 					  u32 addr,
 					  u32 data, u32 flags)
 {
@@ -2172,7 +2172,7 @@ int drxj_dap_atomic_read_write_block(struct i2c_device_addr *dev_addr,
 * \fn int drxj_dap_atomic_read_reg32()
 * \brief Atomic read of 32 bits words
 */
-static
+static noinline_if_stackbloat
 int drxj_dap_atomic_read_reg32(struct i2c_device_addr *dev_addr,
 				     u32 addr,
 				     u32 *data, u32 flags)
@@ -4192,7 +4192,7 @@ int drxj_dap_scu_atomic_read_write_block(struct i2c_device_addr *dev_addr, u32 a
 * \fn int DRXJ_DAP_AtomicReadReg16()
 * \brief Atomic read of 16 bits words
 */
-static
+static noinline_if_stackbloat
 int drxj_dap_scu_atomic_read_reg16(struct i2c_device_addr *dev_addr,
 					 u32 addr,
 					 u16 *data, u32 flags)
@@ -4220,7 +4220,7 @@ int drxj_dap_scu_atomic_read_reg16(struct i2c_device_addr *dev_addr,
 * \fn int drxj_dap_scu_atomic_write_reg16()
 * \brief Atomic read of 16 bits words
 */
-static
+static noinline_if_stackbloat
 int drxj_dap_scu_atomic_write_reg16(struct i2c_device_addr *dev_addr,
 					  u32 addr,
 					  u16 data, u32 flags)
diff --git a/drivers/media/dvb-frontends/helene.c b/drivers/media/dvb-frontends/helene.c
index 4bf5a551ba40..849a18a837d0 100644
--- a/drivers/media/dvb-frontends/helene.c
+++ b/drivers/media/dvb-frontends/helene.c
@@ -329,7 +329,7 @@ static int helene_write_regs(struct helene_priv *priv,
 	return 0;
 }
 
-static int helene_write_reg(struct helene_priv *priv, u8 reg, u8 val)
+static noinline_if_stackbloat int helene_write_reg(struct helene_priv *priv, u8 reg, u8 val)
 {
 	return helene_write_regs(priv, reg, &val, 1);
 }
@@ -374,7 +374,7 @@ static int helene_read_regs(struct helene_priv *priv,
 	return 0;
 }
 
-static int helene_read_reg(struct helene_priv *priv, u8 reg, u8 *val)
+static noinline_if_stackbloat int helene_read_reg(struct helene_priv *priv, u8 reg, u8 *val)
 {
 	return helene_read_regs(priv, reg, val, 1);
 }
diff --git a/drivers/media/dvb-frontends/horus3a.c b/drivers/media/dvb-frontends/horus3a.c
index 68d759c4c52e..f879af6c3188 100644
--- a/drivers/media/dvb-frontends/horus3a.c
+++ b/drivers/media/dvb-frontends/horus3a.c
@@ -87,7 +87,7 @@ static int horus3a_write_regs(struct horus3a_priv *priv,
 	return 0;
 }
 
-static int horus3a_write_reg(struct horus3a_priv *priv, u8 reg, u8 val)
+static noinline_if_stackbloat int horus3a_write_reg(struct horus3a_priv *priv, u8 reg, u8 val)
 {
 	return horus3a_write_regs(priv, reg, &val, 1);
 }
diff --git a/drivers/media/dvb-frontends/itd1000.c b/drivers/media/dvb-frontends/itd1000.c
index 5bb1e73a10b4..8bd6d04362cc 100644
--- a/drivers/media/dvb-frontends/itd1000.c
+++ b/drivers/media/dvb-frontends/itd1000.c
@@ -93,7 +93,7 @@ static int itd1000_read_reg(struct itd1000_state *state, u8 reg)
 	return val;
 }
 
-static inline int itd1000_write_reg(struct itd1000_state *state, u8 r, u8 v)
+static noinline_if_stackbloat int itd1000_write_reg(struct itd1000_state *state, u8 r, u8 v)
 {
 	int ret = itd1000_write_regs(state, r, &v, 1);
 	state->shadow[r] = v;
diff --git a/drivers/media/dvb-frontends/mt312.c b/drivers/media/dvb-frontends/mt312.c
index 961b9a2508e0..d7a701da598a 100644
--- a/drivers/media/dvb-frontends/mt312.c
+++ b/drivers/media/dvb-frontends/mt312.c
@@ -139,7 +139,7 @@ static inline int mt312_readreg(struct mt312_state *state,
 	return mt312_read(state, reg, val, 1);
 }
 
-static inline int mt312_writereg(struct mt312_state *state,
+static noinline_if_stackbloat int mt312_writereg(struct mt312_state *state,
 				 const enum mt312_reg_addr reg, const u8 val)
 {
 	return mt312_write(state, reg, &val, 1);
diff --git a/drivers/media/dvb-frontends/si2165.c b/drivers/media/dvb-frontends/si2165.c
index 528b82a5dd46..8b1ac134f9d8 100644
--- a/drivers/media/dvb-frontends/si2165.c
+++ b/drivers/media/dvb-frontends/si2165.c
@@ -140,7 +140,7 @@ static int si2165_read(struct si2165_state *state,
 	return 0;
 }
 
-static int si2165_readreg8(struct si2165_state *state,
+static noinline_if_stackbloat int si2165_readreg8(struct si2165_state *state,
 		       const u16 reg, u8 *val)
 {
 	unsigned int val_tmp;
@@ -150,7 +150,7 @@ static int si2165_readreg8(struct si2165_state *state,
 	return ret;
 }
 
-static int si2165_readreg16(struct si2165_state *state,
+static noinline_if_stackbloat int si2165_readreg16(struct si2165_state *state,
 		       const u16 reg, u16 *val)
 {
 	u8 buf[2];
@@ -161,26 +161,26 @@ static int si2165_readreg16(struct si2165_state *state,
 	return ret;
 }
 
-static int si2165_writereg8(struct si2165_state *state, const u16 reg, u8 val)
+static noinline_if_stackbloat int si2165_writereg8(struct si2165_state *state, const u16 reg, u8 val)
 {
 	return regmap_write(state->regmap, reg, val);
 }
 
-static int si2165_writereg16(struct si2165_state *state, const u16 reg, u16 val)
+static noinline_if_stackbloat int si2165_writereg16(struct si2165_state *state, const u16 reg, u16 val)
 {
 	u8 buf[2] = { val & 0xff, (val >> 8) & 0xff };
 
 	return si2165_write(state, reg, buf, 2);
 }
 
-static int si2165_writereg24(struct si2165_state *state, const u16 reg, u32 val)
+static noinline_if_stackbloat int si2165_writereg24(struct si2165_state *state, const u16 reg, u32 val)
 {
 	u8 buf[3] = { val & 0xff, (val >> 8) & 0xff, (val >> 16) & 0xff };
 
 	return si2165_write(state, reg, buf, 3);
 }
 
-static int si2165_writereg32(struct si2165_state *state, const u16 reg, u32 val)
+static noinline_if_stackbloat int si2165_writereg32(struct si2165_state *state, const u16 reg, u32 val)
 {
 	u8 buf[4] = {
 		val & 0xff,
@@ -191,7 +191,7 @@ static int si2165_writereg32(struct si2165_state *state, const u16 reg, u32 val)
 	return si2165_write(state, reg, buf, 4);
 }
 
-static int si2165_writereg_mask8(struct si2165_state *state, const u16 reg,
+static noinline_if_stackbloat int si2165_writereg_mask8(struct si2165_state *state, const u16 reg,
 				 u8 val, u8 mask)
 {
 	if (mask != 0xff) {
diff --git a/drivers/media/dvb-frontends/stb0899_drv.c b/drivers/media/dvb-frontends/stb0899_drv.c
index 02347598277a..9258085b8d35 100644
--- a/drivers/media/dvb-frontends/stb0899_drv.c
+++ b/drivers/media/dvb-frontends/stb0899_drv.c
@@ -537,7 +537,7 @@ int stb0899_write_regs(struct stb0899_state *state, unsigned int reg, u8 *data,
 	return 0;
 }
 
-int stb0899_write_reg(struct stb0899_state *state, unsigned int reg, u8 data)
+noinline_if_stackbloat int stb0899_write_reg(struct stb0899_state *state, unsigned int reg, u8 data)
 {
 	return stb0899_write_regs(state, reg, &data, 1);
 }
diff --git a/drivers/media/dvb-frontends/stb6100.c b/drivers/media/dvb-frontends/stb6100.c
index 17a955d0031b..675dffe1ef20 100644
--- a/drivers/media/dvb-frontends/stb6100.c
+++ b/drivers/media/dvb-frontends/stb6100.c
@@ -224,7 +224,7 @@ static int stb6100_write_reg_range(struct stb6100_state *state, u8 buf[], int st
 	return 0;
 }
 
-static int stb6100_write_reg(struct stb6100_state *state, u8 reg, u8 data)
+static noinline_if_stackbloat int stb6100_write_reg(struct stb6100_state *state, u8 reg, u8 data)
 {
 	if (unlikely(reg >= STB6100_NUMREGS)) {
 		dprintk(verbose, FE_ERROR, 1, "Invalid register offset 0x%x", reg);
diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
index fd49c436a36d..2316c0bb3e21 100644
--- a/drivers/media/dvb-frontends/stv0367.c
+++ b/drivers/media/dvb-frontends/stv0367.c
@@ -798,7 +798,7 @@ int stv0367_writeregs(struct stv0367_state *state, u16 reg, u8 *data, int len)
 	return (ret != 1) ? -EREMOTEIO : 0;
 }
 
-static int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
+static noinline_if_stackbloat int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
 {
 	return stv0367_writeregs(state, reg, &data, 1);
 }
diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 7ef469c0c866..8afecc2e3637 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -753,7 +753,7 @@ static int stv090x_write_regs(struct stv090x_state *state, unsigned int reg, u8
 	return 0;
 }
 
-static int stv090x_write_reg(struct stv090x_state *state, unsigned int reg, u8 data)
+static noinline_if_stackbloat int stv090x_write_reg(struct stv090x_state *state, unsigned int reg, u8 data)
 {
 	return stv090x_write_regs(state, reg, &data, 1);
 }
diff --git a/drivers/media/dvb-frontends/stv6110.c b/drivers/media/dvb-frontends/stv6110.c
index e4fd9c1b0560..ddef3a912615 100644
--- a/drivers/media/dvb-frontends/stv6110.c
+++ b/drivers/media/dvb-frontends/stv6110.c
@@ -137,7 +137,7 @@ static int stv6110_read_regs(struct dvb_frontend *fe, u8 regs[],
 	return 0;
 }
 
-static int stv6110_read_reg(struct dvb_frontend *fe, int start)
+static noinline_if_stackbloat int stv6110_read_reg(struct dvb_frontend *fe, int start)
 {
 	u8 buf[] = { 0 };
 	stv6110_read_regs(fe, buf, start, 1);
diff --git a/drivers/media/dvb-frontends/stv6110x.c b/drivers/media/dvb-frontends/stv6110x.c
index 66eba38f1014..80c7024971de 100644
--- a/drivers/media/dvb-frontends/stv6110x.c
+++ b/drivers/media/dvb-frontends/stv6110x.c
@@ -95,7 +95,7 @@ static int stv6110x_write_regs(struct stv6110x_state *stv6110x, int start, u8 da
 	return 0;
 }
 
-static int stv6110x_write_reg(struct stv6110x_state *stv6110x, u8 reg, u8 data)
+static noinline_if_stackbloat int stv6110x_write_reg(struct stv6110x_state *stv6110x, u8 reg, u8 data)
 {
 	return stv6110x_write_regs(stv6110x, reg, &data, 1);
 }
diff --git a/drivers/media/dvb-frontends/tda8083.c b/drivers/media/dvb-frontends/tda8083.c
index aa3200d3c352..646f22aab24e 100644
--- a/drivers/media/dvb-frontends/tda8083.c
+++ b/drivers/media/dvb-frontends/tda8083.c
@@ -88,7 +88,7 @@ static int tda8083_readregs (struct tda8083_state* state, u8 reg1, u8 *b, u8 len
 	return ret == 2 ? 0 : -1;
 }
 
-static inline u8 tda8083_readreg (struct tda8083_state* state, u8 reg)
+static noinline_if_stackbloat u8 tda8083_readreg (struct tda8083_state* state, u8 reg)
 {
 	u8 val;
 
diff --git a/drivers/media/dvb-frontends/zl10039.c b/drivers/media/dvb-frontends/zl10039.c
index 623355fc2666..0075725cb161 100644
--- a/drivers/media/dvb-frontends/zl10039.c
+++ b/drivers/media/dvb-frontends/zl10039.c
@@ -130,7 +130,7 @@ static inline int zl10039_readreg(struct zl10039_state *state,
 	return zl10039_read(state, reg, val, 1);
 }
 
-static inline int zl10039_writereg(struct zl10039_state *state,
+static noinline_if_stackbloat int zl10039_writereg(struct zl10039_state *state,
 				const enum zl10039_reg_addr reg,
 				const u8 val)
 {
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1666241 — [PATCH v2 11/11] kasan: rework Kconfig settings

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-14 23:30 +0200
Subject[PATCH v2 11/11] kasan: rework Kconfig settings
Message-ID<tSo1r-1Xi-13@gated-at.bofh.it>
In reply to#1666233
We get a lot of very large stack frames using gcc-7.0.1 with the default
-fsanitize-address-use-after-scope --param asan-stack=1 options, which
can easily cause an overflow of the kernel stack, e.g.

drivers/acpi/nfit/core.c:2686:1: warning: the frame size of 4080 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/gpu/drm/amd/amdgpu/si.c:1756:1: warning: the frame size of 7304 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/gpu/drm/i915/gvt/handlers.c:2200:1: warning: the frame size of 43752 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:952:1: warning: the frame size of 6032 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/isdn/hardware/avm/b1.c:637:1: warning: the frame size of 13200 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/media/dvb-frontends/stv090x.c:3089:1: warning: the frame size of 5880 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/media/i2c/cx25840/cx25840-core.c:4964:1: warning: the frame size of 93992 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:4994:1: warning: the frame size of 23928 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/staging/dgnc/dgnc_tty.c:2788:1: warning: the frame size of 7072 bytes is larger than 2048 bytes [-Wframe-larger-than=]
fs/ntfs/mft.c:2762:1: warning: the frame size of 7432 bytes is larger than 2048 bytes [-Wframe-larger-than=]
lib/atomic64_test.c:242:1: warning: the frame size of 12648 bytes is larger than 2048 bytes [-Wframe-larger-than=]

To reduce this risk, -fsanitize-address-use-after-scope is now split out
into a separate Kconfig option, vhich cannot be selected at the same
time as KMEMCHECK, leading to stack frames that are smaller than 2
kilobytes most of the time on x86_64. An earlier version of this
patch also prevented combining KASAN_EXTRA with KASAN_INLINE, but that
is no longer necessary with gcc-7.0.1.

A lot of warnings with KASAN_EXTRA go away if we disable KMEMCHECK,
as -fsanitize-address-use-after-scope seems to understand the builtin
memcpy, but adds checking code around an extern memcpy call. I had
to work around a circular dependency, as DEBUG_SLAB/SLUB depended
on !KMEMCHECK, while KASAN did it the other way round. Now we handle
both the same way.

All patches to get the frame size below 2048 bytes with CONFIG_KASAN=y
and CONFIG_KASAN_EXTRA=n have been submitted along with this patch,
so we can bring back that default now. KASAN_EXTRA=y still causes lots
of warnings but now defaults to !COMPILE_TEST to disable it in
allmodconfig, and it remains disabled in all other defconfigs since
it is a new option.

This reverts parts of commit commit 3f181b4 ("lib/Kconfig.debug:
disable -Wframe-larger-than warnings with KASAN=y").

I experimented a bit more with smaller stack frames and have another
follow-up series that reduces the warning limit for 64-bit architectures
to 1280 bytes and 1536 when CONFIG_KASAN (but not KASAN_EXTRA) is
enabled, this requires another ~25 patches to address the additional
warnings. I also have patches for all KASAN_EXTRA warnings, but we
should look at those separately and then decide whether to remove
it completely, leaving out -fsanitize-address-use-after-scope.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/Kconfig.debug      |  4 ++--
 lib/Kconfig.kasan      | 11 ++++++++++-
 lib/Kconfig.kmemcheck  |  1 +
 scripts/Makefile.kasan |  3 +++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ddbef2cac189..02ec4a4da7b1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -217,7 +217,7 @@ config ENABLE_MUST_CHECK
 config FRAME_WARN
 	int "Warn for stack frames larger than (needs gcc 4.4)"
 	range 0 8192
-	default 0 if KASAN
+	default 3072 if KASAN_EXTRA
 	default 2048 if GCC_PLUGIN_LATENT_ENTROPY
 	default 1024 if !64BIT
 	default 2048 if 64BIT
@@ -500,7 +500,7 @@ config DEBUG_OBJECTS_ENABLE_DEFAULT
 
 config DEBUG_SLAB
 	bool "Debug slab memory allocations"
-	depends on DEBUG_KERNEL && SLAB && !KMEMCHECK
+	depends on DEBUG_KERNEL && SLAB && !KMEMCHECK && !KASAN
 	help
 	  Say Y here to have the kernel do limited verification on memory
 	  allocation as well as poisoning memory on free to catch use of freed
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index bd38aab05929..4d17a8f4742f 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -5,7 +5,7 @@ if HAVE_ARCH_KASAN
 
 config KASAN
 	bool "KASan: runtime memory debugger"
-	depends on SLUB || (SLAB && !DEBUG_SLAB)
+	depends on SLUB || SLAB
 	select CONSTRUCTORS
 	select STACKDEPOT
 	help
@@ -20,6 +20,15 @@ config KASAN
 	  Currently CONFIG_KASAN doesn't work with CONFIG_DEBUG_SLAB
 	  (the resulting kernel does not boot).
 
+config KASAN_EXTRA
+	bool "KAsan: extra checks"
+	depends on KASAN && !COMPILE_TEST
+	help
+	  This enables further checks in the kernel address sanitizer, for now
+	  it only includes the address-use-after-scope check that can lead
+	  to excessive kernel stack usage, frame size warnings and longer
+	  compile time.
+
 choice
 	prompt "Instrumentation type"
 	depends on KASAN
diff --git a/lib/Kconfig.kmemcheck b/lib/Kconfig.kmemcheck
index 846e039a86b4..58b9f3f81dc8 100644
--- a/lib/Kconfig.kmemcheck
+++ b/lib/Kconfig.kmemcheck
@@ -7,6 +7,7 @@ menuconfig KMEMCHECK
 	bool "kmemcheck: trap use of uninitialized memory"
 	depends on DEBUG_KERNEL
 	depends on !X86_USE_3DNOW
+	depends on !KASAN_EXTRA
 	depends on SLUB || SLAB
 	depends on !CC_OPTIMIZE_FOR_SIZE
 	depends on !FUNCTION_TRACER
diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
index 9576775a86f6..3b3148faf866 100644
--- a/scripts/Makefile.kasan
+++ b/scripts/Makefile.kasan
@@ -29,5 +29,8 @@ else
     endif
 endif
 
+ifdef CONFIG_KASAN_EXTRA
 CFLAGS_KASAN += $(call cc-option, -fsanitize-address-use-after-scope)
 endif
+
+endif
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1666501 — Re: [PATCH v2 11/11] kasan: rework Kconfig settings

FromDmitry Vyukov <dvyukov@google.com>
Date2017-06-15 09:10 +0200
SubjectRe: [PATCH v2 11/11] kasan: rework Kconfig settings
Message-ID<tSx4J-7Qq-1@gated-at.bofh.it>
In reply to#1666241
On Wed, Jun 14, 2017 at 11:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> We get a lot of very large stack frames using gcc-7.0.1 with the default
> -fsanitize-address-use-after-scope --param asan-stack=1 options, which
> can easily cause an overflow of the kernel stack, e.g.
>
> drivers/acpi/nfit/core.c:2686:1: warning: the frame size of 4080 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/gpu/drm/amd/amdgpu/si.c:1756:1: warning: the frame size of 7304 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/gpu/drm/i915/gvt/handlers.c:2200:1: warning: the frame size of 43752 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:952:1: warning: the frame size of 6032 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/isdn/hardware/avm/b1.c:637:1: warning: the frame size of 13200 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/media/dvb-frontends/stv090x.c:3089:1: warning: the frame size of 5880 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/media/i2c/cx25840/cx25840-core.c:4964:1: warning: the frame size of 93992 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:4994:1: warning: the frame size of 23928 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> drivers/staging/dgnc/dgnc_tty.c:2788:1: warning: the frame size of 7072 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> fs/ntfs/mft.c:2762:1: warning: the frame size of 7432 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> lib/atomic64_test.c:242:1: warning: the frame size of 12648 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>
> To reduce this risk, -fsanitize-address-use-after-scope is now split out
> into a separate Kconfig option, vhich cannot be selected at the same
> time as KMEMCHECK, leading to stack frames that are smaller than 2
> kilobytes most of the time on x86_64. An earlier version of this
> patch also prevented combining KASAN_EXTRA with KASAN_INLINE, but that
> is no longer necessary with gcc-7.0.1.
>
> A lot of warnings with KASAN_EXTRA go away if we disable KMEMCHECK,
> as -fsanitize-address-use-after-scope seems to understand the builtin
> memcpy, but adds checking code around an extern memcpy call. I had
> to work around a circular dependency, as DEBUG_SLAB/SLUB depended
> on !KMEMCHECK, while KASAN did it the other way round. Now we handle
> both the same way.
>
> All patches to get the frame size below 2048 bytes with CONFIG_KASAN=y
> and CONFIG_KASAN_EXTRA=n have been submitted along with this patch,
> so we can bring back that default now. KASAN_EXTRA=y still causes lots
> of warnings but now defaults to !COMPILE_TEST to disable it in
> allmodconfig, and it remains disabled in all other defconfigs since
> it is a new option.
>
> This reverts parts of commit commit 3f181b4 ("lib/Kconfig.debug:
> disable -Wframe-larger-than warnings with KASAN=y").
>
> I experimented a bit more with smaller stack frames and have another
> follow-up series that reduces the warning limit for 64-bit architectures
> to 1280 bytes and 1536 when CONFIG_KASAN (but not KASAN_EXTRA) is
> enabled, this requires another ~25 patches to address the additional
> warnings. I also have patches for all KASAN_EXTRA warnings, but we
> should look at those separately and then decide whether to remove
> it completely, leaving out -fsanitize-address-use-after-scope.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/Kconfig.debug      |  4 ++--
>  lib/Kconfig.kasan      | 11 ++++++++++-
>  lib/Kconfig.kmemcheck  |  1 +
>  scripts/Makefile.kasan |  3 +++
>  4 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ddbef2cac189..02ec4a4da7b1 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -217,7 +217,7 @@ config ENABLE_MUST_CHECK
>  config FRAME_WARN
>         int "Warn for stack frames larger than (needs gcc 4.4)"
>         range 0 8192
> -       default 0 if KASAN
> +       default 3072 if KASAN_EXTRA
>         default 2048 if GCC_PLUGIN_LATENT_ENTROPY
>         default 1024 if !64BIT
>         default 2048 if 64BIT
> @@ -500,7 +500,7 @@ config DEBUG_OBJECTS_ENABLE_DEFAULT
>
>  config DEBUG_SLAB
>         bool "Debug slab memory allocations"
> -       depends on DEBUG_KERNEL && SLAB && !KMEMCHECK
> +       depends on DEBUG_KERNEL && SLAB && !KMEMCHECK && !KASAN
>         help
>           Say Y here to have the kernel do limited verification on memory
>           allocation as well as poisoning memory on free to catch use of freed
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index bd38aab05929..4d17a8f4742f 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -5,7 +5,7 @@ if HAVE_ARCH_KASAN
>
>  config KASAN
>         bool "KASan: runtime memory debugger"
> -       depends on SLUB || (SLAB && !DEBUG_SLAB)
> +       depends on SLUB || SLAB
>         select CONSTRUCTORS
>         select STACKDEPOT
>         help
> @@ -20,6 +20,15 @@ config KASAN
>           Currently CONFIG_KASAN doesn't work with CONFIG_DEBUG_SLAB
>           (the resulting kernel does not boot).
>
> +config KASAN_EXTRA
> +       bool "KAsan: extra checks"
> +       depends on KASAN && !COMPILE_TEST
> +       help
> +         This enables further checks in the kernel address sanitizer, for now
> +         it only includes the address-use-after-scope check that can lead
> +         to excessive kernel stack usage, frame size warnings and longer
> +         compile time.
> +
>  choice
>         prompt "Instrumentation type"
>         depends on KASAN
> diff --git a/lib/Kconfig.kmemcheck b/lib/Kconfig.kmemcheck
> index 846e039a86b4..58b9f3f81dc8 100644
> --- a/lib/Kconfig.kmemcheck
> +++ b/lib/Kconfig.kmemcheck
> @@ -7,6 +7,7 @@ menuconfig KMEMCHECK
>         bool "kmemcheck: trap use of uninitialized memory"
>         depends on DEBUG_KERNEL
>         depends on !X86_USE_3DNOW
> +       depends on !KASAN_EXTRA


KASAN is not meant to work with KMEMCHECK. I am not sure if it even
works, and it's definitely unmaintained. Even if it works now, I am
not sure what could be the intention of somebody running that
combination.
So I think we should do: depends on !KASAN
Will it make things simper for you?


>         depends on SLUB || SLAB
>         depends on !CC_OPTIMIZE_FOR_SIZE
>         depends on !FUNCTION_TRACER
> diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
> index 9576775a86f6..3b3148faf866 100644
> --- a/scripts/Makefile.kasan
> +++ b/scripts/Makefile.kasan
> @@ -29,5 +29,8 @@ else
>      endif
>  endif
>
> +ifdef CONFIG_KASAN_EXTRA
>  CFLAGS_KASAN += $(call cc-option, -fsanitize-address-use-after-scope)
>  endif
> +
> +endif
> --
> 2.9.0

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web