[PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
stack frames in some functions. This goes unnoticed normally because
CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
KASAN=y").

The kernelci.org build bot however has the warning enabled and that led
me to investigate it a little further, as every build produces these warnings:

net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-Wframe-larger-than=]
net/wireless/nl80211.c:1895:1: warning: the frame size of 3776 bytes is larger than 2048 bytes [-Wframe-larger-than=]
net/wireless/nl80211.c:1410:1: warning: the frame size of 2208 bytes is larger than 2048 bytes [-Wframe-larger-than=]
net/bridge/br_netlink.c:1282:1: warning: the frame size of 2544 bytes is larger than 2048 bytes [-Wframe-larger-than=]

It turns out that there is a relatively simple workaround for the netlink
users that currently use a local variable in order to do the type conversion:
Moving the three functions (for each of the typical sizes) to lib/nlattr.c
avoids using local variables in the caller, which drastically reduces the
stack usage for nl80211 and br_netlink.

It would be good if we could enable the frame size check after that again,
but that should be a separate patch and it requires some more testing
to see which the largest acceptable frame size should be.

Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: kasan-dev@googlegroups.com
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/net/netlink.h | 23 +++++++----------------
 lib/nlattr.c          | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b239fcd33d80..48b117e80509 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -755,10 +755,7 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
  * @attrtype: attribute type
  * @value: numeric value
  */
-static inline int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
-{
-	return nla_put(skb, attrtype, sizeof(u8), &value);
-}
+extern int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value);
 
 /**
  * nla_put_u16 - Add a u16 netlink attribute to a socket buffer
@@ -766,10 +763,7 @@ static inline int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
  * @attrtype: attribute type
  * @value: numeric value
  */
-static inline int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
-{
-	return nla_put(skb, attrtype, sizeof(u16), &value);
-}
+extern int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value);
 
 /**
  * nla_put_be16 - Add a __be16 netlink attribute to a socket buffer
@@ -779,7 +773,7 @@ static inline int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
  */
 static inline int nla_put_be16(struct sk_buff *skb, int attrtype, __be16 value)
 {
-	return nla_put(skb, attrtype, sizeof(__be16), &value);
+	return nla_put_u16(skb, attrtype, (u16 __force)value);
 }
 
 /**
@@ -801,7 +795,7 @@ static inline int nla_put_net16(struct sk_buff *skb, int attrtype, __be16 value)
  */
 static inline int nla_put_le16(struct sk_buff *skb, int attrtype, __le16 value)
 {
-	return nla_put(skb, attrtype, sizeof(__le16), &value);
+	return nla_put_u16(skb, attrtype, (u16 __force)value);
 }
 
 /**
@@ -810,10 +804,7 @@ static inline int nla_put_le16(struct sk_buff *skb, int attrtype, __le16 value)
  * @attrtype: attribute type
  * @value: numeric value
  */
-static inline int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
-{
-	return nla_put(skb, attrtype, sizeof(u32), &value);
-}
+int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value);
 
 /**
  * nla_put_be32 - Add a __be32 netlink attribute to a socket buffer
@@ -823,7 +814,7 @@ static inline int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
  */
 static inline int nla_put_be32(struct sk_buff *skb, int attrtype, __be32 value)
 {
-	return nla_put(skb, attrtype, sizeof(__be32), &value);
+	return nla_put_u32(skb, attrtype, (u32 __force)value);
 }
 
 /**
@@ -845,7 +836,7 @@ static inline int nla_put_net32(struct sk_buff *skb, int attrtype, __be32 value)
  */
 static inline int nla_put_le32(struct sk_buff *skb, int attrtype, __le32 value)
 {
-	return nla_put(skb, attrtype, sizeof(__le32), &value);
+	return nla_put_u32(skb, attrtype, (u32 __force)value);
 }
 
 /**
diff --git a/lib/nlattr.c b/lib/nlattr.c
index b42b8577fc23..2988b08a7e4d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -548,6 +548,24 @@ int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 }
 EXPORT_SYMBOL(nla_put);
 
+int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
+{
+	return nla_put(skb, attrtype, sizeof(u8), &value);
+}
+EXPORT_SYMBOL(nla_put_u8);
+
+int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
+{
+	return nla_put(skb, attrtype, sizeof(u16), &value);
+}
+EXPORT_SYMBOL(nla_put_u16);
+
+int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
+{
+	return nla_put(skb, attrtype, sizeof(u32), &value);
+}
+EXPORT_SYMBOL(nla_put_u32);
+
 /**
  * nla_put_64bit - Add a netlink attribute to a socket buffer and align it
  * @skb: socket buffer to add attribute to
-- 
2.9.0

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Dmitry Vyukov]

On Wed, Feb 8, 2017 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
> stack frames in some functions. This goes unnoticed normally because
> CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
> 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
> KASAN=y").
>
> The kernelci.org build bot however has the warning enabled and that led
> me to investigate it a little further, as every build produces these warnings:
>
> net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/wireless/nl80211.c:1895:1: warning: the frame size of 3776 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/wireless/nl80211.c:1410:1: warning: the frame size of 2208 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/bridge/br_netlink.c:1282:1: warning: the frame size of 2544 bytes is larger than 2048 bytes [-Wframe-larger-than=]


This may be a good thing in itself, I don't know. But I don't think we
need to do this for KASAN.
KASAN does increase stack frames, but it also doubles stack size. So
at the very least we need to also double CONFIG_FRAME_WARN under KASAN
to 4096, which would auto-fix your warnings. Enabling 4096 warning for
KASAN may be moderately useful (catch cases of extremely large frames,
because KASAN frame increase can be much larger than 2x). But I am
watching these "used greatest stack depth" messages and also my test
bots and never seen anything worryingly low.

I've built kernel with KASAN+KCOV and there is 381 case throughout the codebase:
https://gist.githubusercontent.com/dvyukov/038426827b369f62d56e50cb39db29df/raw/96cdd758f06751339428c05fda3bc09c6beb700b/gistfile1.txt
Changing code for each of them looks like lots of work, and can
introduce bugs. 4096 would leave about 50 of them, but it's still
lots.


> It turns out that there is a relatively simple workaround for the netlink
> users that currently use a local variable in order to do the type conversion:
> Moving the three functions (for each of the typical sizes) to lib/nlattr.c
> avoids using local variables in the caller, which drastically reduces the
> stack usage for nl80211 and br_netlink.
>
> It would be good if we could enable the frame size check after that again,
> but that should be a separate patch and it requires some more testing
> to see which the largest acceptable frame size should be.
>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: kasan-dev@googlegroups.com
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/net/netlink.h | 23 +++++++----------------
>  lib/nlattr.c          | 18 ++++++++++++++++++
>  2 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index b239fcd33d80..48b117e80509 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -755,10 +755,7 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
>   * @attrtype: attribute type
>   * @value: numeric value
>   */
> -static inline int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
> -{
> -       return nla_put(skb, attrtype, sizeof(u8), &value);
> -}
> +extern int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value);
>
>  /**
>   * nla_put_u16 - Add a u16 netlink attribute to a socket buffer
> @@ -766,10 +763,7 @@ static inline int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
>   * @attrtype: attribute type
>   * @value: numeric value
>   */
> -static inline int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
> -{
> -       return nla_put(skb, attrtype, sizeof(u16), &value);
> -}
> +extern int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value);
>
>  /**
>   * nla_put_be16 - Add a __be16 netlink attribute to a socket buffer
> @@ -779,7 +773,7 @@ static inline int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
>   */
>  static inline int nla_put_be16(struct sk_buff *skb, int attrtype, __be16 value)
>  {
> -       return nla_put(skb, attrtype, sizeof(__be16), &value);
> +       return nla_put_u16(skb, attrtype, (u16 __force)value);
>  }
>
>  /**
> @@ -801,7 +795,7 @@ static inline int nla_put_net16(struct sk_buff *skb, int attrtype, __be16 value)
>   */
>  static inline int nla_put_le16(struct sk_buff *skb, int attrtype, __le16 value)
>  {
> -       return nla_put(skb, attrtype, sizeof(__le16), &value);
> +       return nla_put_u16(skb, attrtype, (u16 __force)value);
>  }
>
>  /**
> @@ -810,10 +804,7 @@ static inline int nla_put_le16(struct sk_buff *skb, int attrtype, __le16 value)
>   * @attrtype: attribute type
>   * @value: numeric value
>   */
> -static inline int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
> -{
> -       return nla_put(skb, attrtype, sizeof(u32), &value);
> -}
> +int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value);
>
>  /**
>   * nla_put_be32 - Add a __be32 netlink attribute to a socket buffer
> @@ -823,7 +814,7 @@ static inline int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
>   */
>  static inline int nla_put_be32(struct sk_buff *skb, int attrtype, __be32 value)
>  {
> -       return nla_put(skb, attrtype, sizeof(__be32), &value);
> +       return nla_put_u32(skb, attrtype, (u32 __force)value);
>  }
>
>  /**
> @@ -845,7 +836,7 @@ static inline int nla_put_net32(struct sk_buff *skb, int attrtype, __be32 value)
>   */
>  static inline int nla_put_le32(struct sk_buff *skb, int attrtype, __le32 value)
>  {
> -       return nla_put(skb, attrtype, sizeof(__le32), &value);
> +       return nla_put_u32(skb, attrtype, (u32 __force)value);
>  }
>
>  /**
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index b42b8577fc23..2988b08a7e4d 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -548,6 +548,24 @@ int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>  }
>  EXPORT_SYMBOL(nla_put);
>
> +int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
> +{
> +       return nla_put(skb, attrtype, sizeof(u8), &value);
> +}
> +EXPORT_SYMBOL(nla_put_u8);
> +
> +int nla_put_u16(struct sk_buff *skb, int attrtype, u16 value)
> +{
> +       return nla_put(skb, attrtype, sizeof(u16), &value);
> +}
> +EXPORT_SYMBOL(nla_put_u16);
> +
> +int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
> +{
> +       return nla_put(skb, attrtype, sizeof(u32), &value);
> +}
> +EXPORT_SYMBOL(nla_put_u32);
> +
>  /**
>   * nla_put_64bit - Add a netlink attribute to a socket buffer and align it
>   * @skb: socket buffer to add attribute to
> --
> 2.9.0
>

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

On Thu, Feb 9, 2017 at 12:28 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Feb 8, 2017 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
>> stack frames in some functions. This goes unnoticed normally because
>> CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
>> 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
>> KASAN=y").
>>
>> The kernelci.org build bot however has the warning enabled and that led
>> me to investigate it a little further, as every build produces these warnings:
>>
>> net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>> net/wireless/nl80211.c:1895:1: warning: the frame size of 3776 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>> net/wireless/nl80211.c:1410:1: warning: the frame size of 2208 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>> net/bridge/br_netlink.c:1282:1: warning: the frame size of 2544 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>
>
> This may be a good thing in itself, I don't know. But I don't think we
> need to do this for KASAN.
> KASAN does increase stack frames, but it also doubles stack size. So
> at the very least we need to also double CONFIG_FRAME_WARN under KASAN
> to 4096, which would auto-fix your warnings. Enabling 4096 warning for
> KASAN may be moderately useful (catch cases of extremely large frames,
> because KASAN frame increase can be much larger than 2x). But I am
> watching these "used greatest stack depth" messages and also my test
> bots and never seen anything worryingly low.

I'm playing with this on my autobuilder some more, and so far
have only encountered a handful of functions that use more
than 2048 bytes of stack on x86-64 with KASAN enabled, and
for each one of them I could come up with a patch that improved
the object code while making the stack smaller.

A "noinline_for_kasan" annotation would be sufficient to address most
of them.

> I've built kernel with KASAN+KCOV and there is 381 case throughout the codebase:
> https://gist.githubusercontent.com/dvyukov/038426827b369f62d56e50cb39db29df/raw/96cdd758f06751339428c05fda3bc09c6beb700b/gistfile1.txt
> Changing code for each of them looks like lots of work, and can
> introduce bugs. 4096 would leave about 50 of them, but it's still
> lots.

Thanks for the list, that looks very helpful. The ones I found myself
seem to be a strict (and small) subset of those, using gcc-7.0.1 on x86-64
with allmodconfig and a few hundred randconfig builds. Which compiler
version did you use for your testing? If new versions are better than old
ones, we could start by fixing the ones that are still present in gcc-6 and
gcc-7, and making the warning conditionally on the compiler version.

Another idea might be to separate out asan_stack=1 into a separate
Kconfig option and warn if that is enabled with compilers that are known
to be relatively bad it keeping the stack small.

    Arnd

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Dmitry Vyukov]

On Thu, Feb 9, 2017 at 12:55 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Feb 9, 2017 at 12:28 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Wed, Feb 8, 2017 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
>>> stack frames in some functions. This goes unnoticed normally because
>>> CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
>>> 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
>>> KASAN=y").
>>>
>>> The kernelci.org build bot however has the warning enabled and that led
>>> me to investigate it a little further, as every build produces these warnings:
>>>
>>> net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>>> net/wireless/nl80211.c:1895:1: warning: the frame size of 3776 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>>> net/wireless/nl80211.c:1410:1: warning: the frame size of 2208 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>>> net/bridge/br_netlink.c:1282:1: warning: the frame size of 2544 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>>
>>
>> This may be a good thing in itself, I don't know. But I don't think we
>> need to do this for KASAN.
>> KASAN does increase stack frames, but it also doubles stack size. So
>> at the very least we need to also double CONFIG_FRAME_WARN under KASAN
>> to 4096, which would auto-fix your warnings. Enabling 4096 warning for
>> KASAN may be moderately useful (catch cases of extremely large frames,
>> because KASAN frame increase can be much larger than 2x). But I am
>> watching these "used greatest stack depth" messages and also my test
>> bots and never seen anything worryingly low.
>
> I'm playing with this on my autobuilder some more, and so far
> have only encountered a handful of functions that use more
> than 2048 bytes of stack on x86-64 with KASAN enabled, and
> for each one of them I could come up with a patch that improved
> the object code while making the stack smaller.
>
> A "noinline_for_kasan" annotation would be sufficient to address most
> of them.
>
>> I've built kernel with KASAN+KCOV and there is 381 case throughout the codebase:
>> https://gist.githubusercontent.com/dvyukov/038426827b369f62d56e50cb39db29df/raw/96cdd758f06751339428c05fda3bc09c6beb700b/gistfile1.txt
>> Changing code for each of them looks like lots of work, and can
>> introduce bugs. 4096 would leave about 50 of them, but it's still
>> lots.
>
> Thanks for the list, that looks very helpful. The ones I found myself
> seem to be a strict (and small) subset of those, using gcc-7.0.1 on x86-64
> with allmodconfig and a few hundred randconfig builds. Which compiler
> version did you use for your testing? If new versions are better than old
> ones, we could start by fixing the ones that are still present in gcc-6 and
> gcc-7, and making the warning conditionally on the compiler version.
>
> Another idea might be to separate out asan_stack=1 into a separate
> Kconfig option and warn if that is enabled with compilers that are known
> to be relatively bad it keeping the stack small.


Mine is gcc version 7.0.0 20161208. Make sure you enable KASAN_INLINE
and I also enabled CONFIG_KCOV. Other than that I just did
allyesconfig + make -k.

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

On Thu, Feb 9, 2017 at 1:46 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Feb 9, 2017 at 12:55 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Feb 9, 2017 at 12:28 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Wed, Feb 8, 2017 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> Thanks for the list, that looks very helpful. The ones I found myself
>> seem to be a strict (and small) subset of those, using gcc-7.0.1 on x86-64
>> with allmodconfig and a few hundred randconfig builds. Which compiler
>> version did you use for your testing? If new versions are better than old
>> ones, we could start by fixing the ones that are still present in gcc-6 and
>> gcc-7, and making the warning conditionally on the compiler version.
>>
>> Another idea might be to separate out asan_stack=1 into a separate
>> Kconfig option and warn if that is enabled with compilers that are known
>> to be relatively bad it keeping the stack small.
>
>
> Mine is gcc version 7.0.0 20161208. Make sure you enable KASAN_INLINE
> and I also enabled CONFIG_KCOV. Other than that I just did
> allyesconfig + make -k.

Ok, I see my mistake now: On the allmodconfig build, I had KASAN_INLINE
disabled, which made the problem go away for almost all files (almost all
frame sizes are below 2048 bytes, except for the two issues I posted patches
for (hisilicon ethernet driver, and nla_put_* users).

On the randconfig build test, I have a long series of patches applied that
address all known warnings, including my earlier "kasan: turn off
-fsanitize-address-use-after-scope for now" (see
https://patchwork.kernel.org/patch/9442323/). Without
-fsanitize-address-use-after-scope, the problem is also mostly gone
(a few cases show up in drivers/media/, and also in block/sed-opal.c
and drivers/tty/vt/keyboard.c). I now have initial patches for all of
them to bring the stack size below 2048 bytes.

As far as I can see, the remaining problems with scary stack frame sizes
you found only show up with the combination of all three: KASAN_INLINE,
asan_stack=1 and -fsanitize-address-use-after-scope. If all three were
separately configurable and we merge the patches I made, I think we could
enable the normal 2048 byte warning in all configurations that have at least
one of the turned off, but I don't know which of those combinations would
actually be sensible for production kernels.

      Arnd

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Dmitry Vyukov]

On Thu, Feb 9, 2017 at 3:33 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> Thanks for the list, that looks very helpful. The ones I found myself
>>> seem to be a strict (and small) subset of those, using gcc-7.0.1 on x86-64
>>> with allmodconfig and a few hundred randconfig builds. Which compiler
>>> version did you use for your testing? If new versions are better than old
>>> ones, we could start by fixing the ones that are still present in gcc-6 and
>>> gcc-7, and making the warning conditionally on the compiler version.
>>>
>>> Another idea might be to separate out asan_stack=1 into a separate
>>> Kconfig option and warn if that is enabled with compilers that are known
>>> to be relatively bad it keeping the stack small.
>>
>>
>> Mine is gcc version 7.0.0 20161208. Make sure you enable KASAN_INLINE
>> and I also enabled CONFIG_KCOV. Other than that I just did
>> allyesconfig + make -k.
>
> Ok, I see my mistake now: On the allmodconfig build, I had KASAN_INLINE
> disabled, which made the problem go away for almost all files (almost all
> frame sizes are below 2048 bytes, except for the two issues I posted patches
> for (hisilicon ethernet driver, and nla_put_* users).
>
> On the randconfig build test, I have a long series of patches applied that
> address all known warnings, including my earlier "kasan: turn off
> -fsanitize-address-use-after-scope for now" (see
> https://patchwork.kernel.org/patch/9442323/). Without
> -fsanitize-address-use-after-scope, the problem is also mostly gone
> (a few cases show up in drivers/media/, and also in block/sed-opal.c
> and drivers/tty/vt/keyboard.c). I now have initial patches for all of
> them to bring the stack size below 2048 bytes.
>
> As far as I can see, the remaining problems with scary stack frame sizes
> you found only show up with the combination of all three: KASAN_INLINE,
> asan_stack=1 and -fsanitize-address-use-after-scope. If all three were
> separately configurable and we merge the patches I made, I think we could
> enable the normal 2048 byte warning in all configurations that have at least
> one of the turned off, but I don't know which of those combinations would
> actually be sensible for production kernels.

FWIW I use all of them (+KCOV which is additional instrumentation).
If you ask which is the least important one, it is
-fsanitize-address-use-after-scope. I have not see any single report
due to it (probably due to kernel coding style).

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

On Thursday, February 9, 2017 4:51:25 PM CET Dmitry Vyukov wrote:
> On Thu, Feb 9, 2017 at 3:33 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>
> >>> Thanks for the list, that looks very helpful. The ones I found myself
> >>> seem to be a strict (and small) subset of those, using gcc-7.0.1 on x86-64
> >>> with allmodconfig and a few hundred randconfig builds. Which compiler
> >>> version did you use for your testing? If new versions are better than old
> >>> ones, we could start by fixing the ones that are still present in gcc-6 and
> >>> gcc-7, and making the warning conditionally on the compiler version.
> >>>
> >>> Another idea might be to separate out asan_stack=1 into a separate
> >>> Kconfig option and warn if that is enabled with compilers that are known
> >>> to be relatively bad it keeping the stack small.
> >>
> >>
> >> Mine is gcc version 7.0.0 20161208. Make sure you enable KASAN_INLINE
> >> and I also enabled CONFIG_KCOV. Other than that I just did
> >> allyesconfig + make -k.
> >
> > Ok, I see my mistake now: On the allmodconfig build, I had KASAN_INLINE
> > disabled, which made the problem go away for almost all files (almost all
> > frame sizes are below 2048 bytes, except for the two issues I posted patches
> > for (hisilicon ethernet driver, and nla_put_* users).
> >
> > On the randconfig build test, I have a long series of patches applied that
> > address all known warnings, including my earlier "kasan: turn off
> > -fsanitize-address-use-after-scope for now" (see
> > https://patchwork.kernel.org/patch/9442323/). Without
> > -fsanitize-address-use-after-scope, the problem is also mostly gone
> > (a few cases show up in drivers/media/, and also in block/sed-opal.c
> > and drivers/tty/vt/keyboard.c). I now have initial patches for all of
> > them to bring the stack size below 2048 bytes.
> >
> > As far as I can see, the remaining problems with scary stack frame sizes
> > you found only show up with the combination of all three: KASAN_INLINE,
> > asan_stack=1 and -fsanitize-address-use-after-scope. If all three were
> > separately configurable and we merge the patches I made, I think we could
> > enable the normal 2048 byte warning in all configurations that have at least
> > one of the turned off, but I don't know which of those combinations would
> > actually be sensible for production kernels.
> 
> FWIW I use all of them (+KCOV which is additional instrumentation).
> If you ask which is the least important one, it is
> -fsanitize-address-use-after-scope. I have not see any single report
> due to it (probably due to kernel coding style).

Ok, I'm testing with this patch now and will let you know if I run
into any other warnings on my x86 randconfig build. I'll also try
to get some results from other compiler versions and architectures.

Let me know what you think of this approach.

	Arnd

>From 119968322aa83f8039531d704e5f1cf24e680680 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, 9 Feb 2017 17:45:59 +0100
Subject: [PATCH] kasan: make use-after-scope sanitizer optional

We get a lot of very large stack frames when combining CONFIG_KASAN_INLINE
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, which cannot be selected at the same time
as CONFIG_KASAN_INLINE, leading to stack frames that are smaller than 2
kilobytes most of the time on x86_64. Now we can turn on the warning again
that was disabled in commit 3f181b4 ("lib/Kconfig.debug: disable
-Wframe-larger-than warnings with KASAN=y").

The hope is that we can fix all code that still produces warnings, so far
I have found four areas that are still affected (netlink, hisi-hns,
dvb and tty/keyboard), and I have patches for all of them.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 554f4c37e72d..7a1657ed9183 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -216,7 +216,6 @@ 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 2048 if GCC_PLUGIN_LATENT_ENTROPY
 	default 1024 if !64BIT
 	default 2048 if 64BIT
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index bd38aab05929..0731c945c85a 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -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
+	help
+	  This enables further checks in the kernel address sanitizer, for now
+	  it only includes the address-use-after-scope check which requires the
+	  use of KASAN_OUTLINE to avoid excessive kernel stack frame sizes that
+	  might lead to stack overflows.
+
 choice
 	prompt "Instrumentation type"
 	depends on KASAN
@@ -36,6 +45,7 @@ config KASAN_OUTLINE
 
 config KASAN_INLINE
 	bool "Inline instrumentation"
+	depends on !KASAN_EXTRA
 	help
 	  Compiler directly inserts code checking shadow memory before
 	  memory accesses. This is faster than outline (in some workloads
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

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed,  8 Feb 2017 22:18:26 +0100

> When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
> stack frames in some functions. This goes unnoticed normally because
> CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
> 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
> KASAN=y").
> 
> The kernelci.org build bot however has the warning enabled and that led
> me to investigate it a little further, as every build produces these warnings:
> 
> net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/wireless/nl80211.c:1895:1: warning: the frame size of 3776 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/wireless/nl80211.c:1410:1: warning: the frame size of 2208 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> net/bridge/br_netlink.c:1282:1: warning: the frame size of 2544 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> 
> It turns out that there is a relatively simple workaround for the netlink
> users that currently use a local variable in order to do the type conversion:
> Moving the three functions (for each of the typical sizes) to lib/nlattr.c
> avoids using local variables in the caller, which drastically reduces the
> stack usage for nl80211 and br_netlink.
> 
> It would be good if we could enable the frame size check after that again,
> but that should be a separate patch and it requires some more testing
> to see which the largest acceptable frame size should be.
> 
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: kasan-dev@googlegroups.com
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

You should only extern these things when KASAN is enabled.

The reason is that uninlining these routines makes attribute emission
more expensive and for some applications performance of this matters.

RE: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[David Laight]

From: David Miller
> Sent: 09 February 2017 21:31
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed,  8 Feb 2017 22:18:26 +0100
> 
> > When CONFIG_KASAN is enabled, the "--param asan-stack=1" causes rather large
> > stack frames in some functions. This goes unnoticed normally because
> > CONFIG_FRAME_WARN is disabled with CONFIG_KASAN by default as of commit
> > 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with
> > KASAN=y").
> >
> > The kernelci.org build bot however has the warning enabled and that led
> > me to investigate it a little further, as every build produces these warnings:
> >
> > net/wireless/nl80211.c:4389:1: warning: the frame size of 2240 bytes is larger than 2048 bytes [-
...
> >
> > It turns out that there is a relatively simple workaround for the netlink
> > users that currently use a local variable in order to do the type conversion:
> > Moving the three functions (for each of the typical sizes) to lib/nlattr.c
> > avoids using local variables in the caller, which drastically reduces the
> > stack usage for nl80211 and br_netlink.
> >
> > It would be good if we could enable the frame size check after that again,
> > but that should be a separate patch and it requires some more testing
> > to see which the largest acceptable frame size should be.
...
> You should only extern these things when KASAN is enabled.
> 
> The reason is that uninlining these routines makes attribute emission
> more expensive and for some applications performance of this matters.

If performance of nla_put() matters, then adding 1, 2 and 4 byte attributes
ought to be doable without writing the values to memory and later doing
(I presume) a memcpy().

I also can't help feeling that the gcc KASAN stuff needs some way to
annotate an 'extern' to say that a value passed by reference isn't
treated as an array.
Otherwise I suspect you get a lot of bloat all over the place.

	David

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

On Thu, Feb 9, 2017 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> To reduce this risk, -fsanitize-address-use-after-scope is now split out
> into a separate Kconfig option, which cannot be selected at the same time
> as CONFIG_KASAN_INLINE, leading to stack frames that are smaller than 2
> kilobytes most of the time on x86_64. Now we can turn on the warning again
> that was disabled in commit 3f181b4 ("lib/Kconfig.debug: disable
> -Wframe-larger-than warnings with KASAN=y").
>
> The hope is that we can fix all code that still produces warnings, so far
> I have found four areas that are still affected (netlink, hisi-hns,
> dvb and tty/keyboard), and I have patches for all of them.

scratch that, my randconfig tests found too many remaining problems
with asan-stack=1 even when only one of CONFIG_KASAN_INLINE
and -fsanitize-address-use-after-scope is set.

I actually get results as bad as
fs/direct-io.c: In function 'do_direct_IO':
fs/direct-io.c:1057:1: error: the frame size of 7240 bytes is larger
than 2048 bytes [-Werror=frame-larger-than=]

with KASAN_OUTLINE=y and KASAN_EXTRA=n.

I need to investigate further to see if I can narrow it down to some
other configuration options.

       Arnd

Re: [PATCH] netlink: move nla_put_{u8,u16,u32} out of line

[Arnd Bergmann]

On Fri, Feb 10, 2017 at 2:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Feb 9, 2017 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> To reduce this risk, -fsanitize-address-use-after-scope is now split out
>> into a separate Kconfig option, which cannot be selected at the same time
>> as CONFIG_KASAN_INLINE, leading to stack frames that are smaller than 2
>> kilobytes most of the time on x86_64. Now we can turn on the warning again
>> that was disabled in commit 3f181b4 ("lib/Kconfig.debug: disable
>> -Wframe-larger-than warnings with KASAN=y").
>>
>> The hope is that we can fix all code that still produces warnings, so far
>> I have found four areas that are still affected (netlink, hisi-hns,
>> dvb and tty/keyboard), and I have patches for all of them.
>
> scratch that, my randconfig tests found too many remaining problems
> with asan-stack=1 even when only one of CONFIG_KASAN_INLINE
> and -fsanitize-address-use-after-scope is set.
>
> I actually get results as bad as
> fs/direct-io.c: In function 'do_direct_IO':
> fs/direct-io.c:1057:1: error: the frame size of 7240 bytes is larger
> than 2048 bytes [-Werror=frame-larger-than=]
>
> with KASAN_OUTLINE=y and KASAN_EXTRA=n.
>
> I need to investigate further to see if I can narrow it down to some
> other configuration options.

Another update while randconfig build updates are coming in: I had
inconsistent results earlier because I was using two different x86
compiler versions: 7.0.0 dated 20161201 and 7.0.1 dated 20170124.

The good news is that most of the extreme frame sizes are gone with
gcc-7.0.1 and my earlier five patches (which addressed mostly the problems I
saw on arm64), this is what I currently see beyond that in randconfig builds,
down from around 500 files with >2048 stack frames I had with gcc-7.0.0:

arch/x86/kernel/cpu/mshyperv.c:234:1: error: the frame size of 2128
bytes is larger than 2048 bytes
arch/x86/kernel/traps.c:1019:1: error: the frame size of 2264 bytes is
larger than 2048 bytes
drivers/acpi/nfit/core.c:2686:1: error: the frame size of 4048 bytes
is larger than 2048 bytes
drivers/gpu/drm/nouveau/nvkm/subdev/i2c/anx9805.c:189:1: error: the
frame size of 2400 bytes is larger than 2048 bytes
drivers/iio/common/ssp_sensors/ssp_spi.c:452:1: error: the frame size
of 2720 bytes is larger than 2048 bytes
drivers/input/mouse/cyapa_gen5.c:2434:1: error: the frame size of 2448
bytes is larger than 2048 bytes
drivers/media/i2c/saa6752hs.c:555:1: error: the frame size of 2232
bytes is larger than 2048 bytes
drivers/media/pci/saa7134/saa7134-cards.c:8068:1: error: the frame
size of 2144 bytes is larger than 2048 bytes
drivers/media/tuners/tda8290.c:310:1: error: the frame size of 3168
bytes is larger than 2048 bytes
drivers/mtd/mtdchar.c:1056:1: error: the frame size of 2104 bytes is
larger than 2048 bytes
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:16065:1:
error: the frame size of 3200 bytes is larger than 2048 bytes
drivers/rapidio/devices/rio_mport_cdev.c:2207:1: error: the frame size
of 2992 bytes is larger than 2048 bytes
drivers/scsi/mpt3sas/mpt3sas_scsih.c:7933:1: error: the frame size of
2056 bytes is larger than 2048 bytes
drivers/scsi/snic/snic_debugfs.c:355:1: error: the frame size of 3200
bytes is larger than 2048 bytes
fs/btrfs/backref.c:1464:1: error: the frame size of 2088 bytes is
larger than 2048 bytes
fs/btrfs/disk-io.c:3316:1: error: the frame size of 2744 bytes is
larger than 2048 bytes
fs/btrfs/extent-tree.c:5232:1: error: the frame size of 2056 bytes is
larger than 2048 bytes
fs/btrfs/relocation.c:1193:1: error: the frame size of 4208 bytes is
larger than 2048 bytes
fs/btrfs/scrub.c:3435:1: error: the frame size of 2144 bytes is larger
than 2048 bytes
fs/btrfs/tree-log.c:3007:1: error: the frame size of 2496 bytes is
larger than 2048 bytes
fs/cachefiles/rdwr.c:669:1: error: the frame size of 2384 bytes is
larger than 2048 bytes
fs/direct-io.c:1057:1: error: the frame size of 2896 bytes is larger
than 2048 bytes
fs/direct-io.c:1348:1: error: the frame size of 2144 bytes is larger
than 2048 bytes
fs/nilfs2/segment.c:1277:1: error: the frame size of 2832 bytes is
larger than 2048 bytes
fs/nilfs2/segment.c:2111:1: error: the frame size of 2240 bytes is
larger than 2048 bytes
fs/xfs/libxfs/xfs_alloc.c:1338:1: error: the frame size of 2504 bytes
is larger than 2048 bytes
fs/xfs/libxfs/xfs_bmap.c:2183:1: error: the frame size of 5104 bytes
is larger than 2048 bytes
fs/xfs/xfs_log_recover.c:1963:1: error: the frame size of 2272 bytes
is larger than 2048 bytes
kernel/rcu/tree.c:2370:1: error: the frame size of 4688 bytes is
larger than 2048 bytes
kernel/rcu/tree_exp.h:618:1: error: the frame size of 3312 bytes is
larger than 2048 bytes
lib/atomic64_test.c:243:1: error: the frame size of 12688 bytes is
larger than 2048 bytes
lib/rbtree.c:447:1: error: the frame size of 2512 bytes is larger than
2048 bytes
mm/khugepaged.c:1559:1: error: the frame size of 2232 bytes is larger
than 2048 bytes
mm/ksm.c:1537:1: error: the frame size of 2232 bytes is larger than 2048 bytes
mm/migrate.c:1357:1: error: the frame size of 2360 bytes is larger
than 2048 bytes
mm/page_alloc.c:3061:1: error: the frame size of 2056 bytes is larger
than 2048 bytes
mm/vmscan.c:1333:1: error: the frame size of 2368 bytes is larger than
2048 bytes

It will take some time to fix all the worst offenders, but it seems
manageable so we can
hopefully turn the warning back on for both x86 and arm64 in the
future.  On arm64,
a warning limit of 2048 (even as low as 1280 when KASAN is disabled) seems
reasonable, on x86-64 we probably want to set it a little higher and rely on the
larger stacks we already use with KASAN.

The suggested "noinline_for_kasan" annotation should be able to fix
the majority of
the affected files (including nla_put_*) with very little risk of
regressions otherwise.

I also still have to go back to gcc-6 and gcc-5, if I get results like
the earlier gcc-7.0.0,
I would probably suggest leaving using asan-stack=0 with any affected
release, to
avoid silliness like the worst case I found so far:
drivers/media/i2c/cx25840/cx25840-core.c:4960:1: error: the frame size
of 94000 bytes is larger than 2048 bytes

    Arnd