Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN

[Joe Perches]

On Thu, 2017-03-02 at 23:59 +0100, Arnd Bergmann wrote:
> KASAN decides that passing a pointer to _m into an extern function
> (_mlog_printk) is potentially dangerous, as that function might
> keep a reference to that pointer after it goes out of scope,
> or it might not know the correct length of the stack object pointed to.
> 
> We can see from looking at the __mlog_printk() function definition
> that it's actually safe, but the compiler cannot see that when looking
> at another source file.

OK, thanks.

btw:

changing __mlog_printk can save ~11% (90+KB) of object text size
by removing __func__ and __LINE__ and using vsprintf pointer extension
%pS, __builtin_return_address(0) as it is already used in dlmmaster.

(defconfig x86-64, with ocfs2)

$ size fs/ocfs2/built-in.o*
   textdata bss dec hex filename
 759791  111373  105688  976852   ee7d4 fs/ocfs2/built-in.o.new
 852959  111373  105688 1070020  1053c4 fs/ocfs2/built-in.o.old

It's nearly the same output.

---

 fs/ocfs2/cluster/masklog.c | 8 
 fs/ocfs2/cluster/masklog.h | 8 +++-
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index d331c2386b94..a3f080f37108 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -64,8 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, 
size_t count)
return count;
 }
 
-void __mlog_printk(const u64 *mask, const char *func, int line,
-  const char *fmt, ...)
+void __mlog_printk(const u64 *mask, const char *fmt, ...)
 {
struct va_format vaf;
va_list args;
@@ -90,9 +89,10 @@ void __mlog_printk(const u64 *mask, const char *func, int 
line,
vaf.fmt = fmt;
vaf.va = 
 
-   printk("%s(%s,%u,%u):%s:%d %s%pV",
+   printk("%s(%s,%u,%u):%pS %s%pV",
   level, current->comm, task_pid_nr(current),
-  raw_smp_processor_id(), func, line, prefix, );
+  raw_smp_processor_id(), __builtin_return_address(0),
+  prefix, );
 
va_end(args);
 }
diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
index 3c16da69605d..56ba5baf625b 100644
--- a/fs/ocfs2/cluster/masklog.h
+++ b/fs/ocfs2/cluster/masklog.h
@@ -162,9 +162,8 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
 
 #endif
 
-__printf(4, 5) __nocapture(2)
-void __mlog_printk(const u64 *m, const char *func, int line,
-  const char *fmt, ...);
+__printf(2, 3) __nocapture(2)
+void __mlog_printk(const u64 *m, const char *fmt, ...);
 
 /*
  * Testing before the __mlog_printk call lets the compiler eliminate the
@@ -174,8 +173,7 @@ void __mlog_printk(const u64 *m, const char *func, int line,
 do {   \
u64 _m = MLOG_MASK_PREFIX | (mask); \
if (_m & ML_ALLOWED_BITS)   \
-   __mlog_printk(&_m, __func__, __LINE__, fmt, \
- ##__VA_ARGS__);   \
+   __mlog_printk(&_m, fmt, ##__VA_ARGS__); \
 } while (0)
 
 #define mlog_errno(st) ({  \

Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN

[Arnd Bergmann]

On Thu, Mar 2, 2017 at 11:40 PM, Joe Perches  wrote:
> On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote:
>> On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches  wrote:
>> > On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote:
>> > > The internal logging infrastructure in ocfs2 causes special warning code 
>> > > to be
>> > > used with KASAN, which produces rather large stack frames:
>> > > fs/ocfs2/super.c: In function 'ocfs2_fill_super':
>> > > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger 
>> > > than 3072 bytes [-Werror=frame-larger-than=]
>> >
>> > At least by default it doesn't seem to.
>> >
>> > gcc 6.2 allyesconfig, CONFIG_KASAN=y
>> > with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE
>> >
>> > gcc doesn't emit a stack warning
>>
>> The warning is disabled until patch 26/26. which picks the 3072 default.
>> The 3264 number was with gcc-7, which is worse than gcc-6 since it enables
>> an extra check.
>>
>> > > By simply passing the mask by value instead of reference, we can avoid 
>> > > the
>> > > problem completely.
>> >
>> > Any idea why that's so?
>>
>> With KASAN, every time we inline the function, the compiler has to allocate
>> space for another copy of the variable plus a redzone to detect whether
>> passing it by reference into another function causes an overflow at runtime.
>
> These logging functions aren't inlined.

Sorry, my mistake. In this case mlog() is a macro, not an inline functions.
The effect is the same though.

> You're referring to the stack frame?

The stack frame of the function that calls mlog(), yes.
>
> Still doesn't make sense to me.
>
> None of the logging functions are inlined as they are all
> EXPORT_SYMBOL.

mlog() is placed in the calling function.

> This just changes a pointer to a u64, which is the same
> size on x86-64 (and is of course larger on x86-32).

KASAN decides that passing a pointer to _m into an extern function
(_mlog_printk) is potentially dangerous, as that function might
keep a reference to that pointer after it goes out of scope,
or it might not know the correct length of the stack object pointed to.

We can see from looking at the __mlog_printk() function definition
that it's actually safe, but the compiler cannot see that when looking
at another source file.

> Perhaps KASAN has the odd behavior and working around
> KASAN's behavior may not be the proper thing to do.

Turning off KASAN fixes the problem, but the entire purpose of
KASAN is to identify code that is potentially dangerous.

> Maybe if CONFIG_KASAN is set, the minimum stack should
> be increased via THREAD_SIZE_ORDER or some such.

This is what happened in 3f181b4d8652 ("lib/Kconfig.debug:
disable -Wframe-larger-than warnings with KASAN=y").

I'm trying to revert that patch so we actually get warnings
again about functions that are still dangerous. I picked 3072
as an arbitrary limit, as there are only a handful of files
that use larger stack frames in the worst case, but we can
only use that limit after fixing up all the warnings it shows.

   Arnd

Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN

[Joe Perches]

On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote:
> On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches  wrote:
> > On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote:
> > > The internal logging infrastructure in ocfs2 causes special warning code 
> > > to be
> > > used with KASAN, which produces rather large stack frames:
> > > fs/ocfs2/super.c: In function 'ocfs2_fill_super':
> > > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger 
> > > than 3072 bytes [-Werror=frame-larger-than=]
> > 
> > At least by default it doesn't seem to.
> > 
> > gcc 6.2 allyesconfig, CONFIG_KASAN=y
> > with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE
> > 
> > gcc doesn't emit a stack warning
> 
> The warning is disabled until patch 26/26. which picks the 3072 default.
> The 3264 number was with gcc-7, which is worse than gcc-6 since it enables
> an extra check.
> 
> > > By simply passing the mask by value instead of reference, we can avoid the
> > > problem completely.
> > 
> > Any idea why that's so?
> 
> With KASAN, every time we inline the function, the compiler has to allocate
> space for another copy of the variable plus a redzone to detect whether
> passing it by reference into another function causes an overflow at runtime.

These logging functions aren't inlined.
You're referring to the stack frame?

> > >  On 64-bit architectures, this is also more efficient,
> > 
> > Efficient true, but the same overall stack no?
> 
> Here is what I see with CONFIG_FRAME_WARN=300 and x86_64-linux-gcc-6.3.1:
> 
> before:
[]
> fs/ocfs2/super.c:1219:1: error: the frame size of 552 bytes is larger
> than 300 bytes [-Werror=frame-larger-than=]
> 
> after:
> fs/ocfs2/super.c: In function 'ocfs2_fill_super':
> fs/ocfs2/super.c:1219:1: error: the frame size of 472 bytes is larger
> than 300 bytes [-Werror=frame-larger-than=]
> 
> and with gcc-7.0.1 (including -fsanitize-address-use-after-scope), before:
[]
> fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger
> than 300 bytes [-Werror=frame-larger-than=]
> 
> after:
> fs/ocfs2/super.c: In function 'ocfs2_fill_super':
> fs/ocfs2/super.c:1219:1: error: the frame size of 704 bytes is larger
> than 300 bytes [-Werror=frame-larger-than=]

Still doesn't make sense to me.

None of the logging functions are inlined as they are all
EXPORT_SYMBOL.

This just changes a pointer to a u64, which is the same
size on x86-64 (and is of course larger on x86-32).

Perhaps KASAN has the odd behavior and working around
KASAN's behavior may not be the proper thing to do.

Maybe if CONFIG_KASAN is set, the minimum stack should
be increased via THREAD_SIZE_ORDER or some such.

[PATCH 24/26] ocfs2: reduce stack size with KASAN

[Arnd Bergmann]

The internal logging infrastructure in ocfs2 causes special warning code to be
used with KASAN, which produces rather large stack frames:

fs/ocfs2/super.c: In function 'ocfs2_fill_super':
fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than 
3072 bytes [-Werror=frame-larger-than=]

By simply passing the mask by value instead of reference, we can avoid the
problem completely. On 64-bit architectures, this is also more efficient,
while on the less common (at least among ocfs2 users) 32-bit architectures,
I'm guessing that the resulting code is comparable to what it was before.

The current version was introduced by Joe Perches as an optimization, maybe
he can see if my change regresses compared to his.

Cc: Joe Perches 
Fixes: 7c2bd2f930ae ("ocfs2: reduce object size of mlog uses")
Signed-off-by: Arnd Bergmann 
---
 fs/ocfs2/cluster/masklog.c | 10 +-
 fs/ocfs2/cluster/masklog.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index d331c2386b94..9720c5443e4d 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -64,7 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, 
size_t count)
return count;
 }
 
-void __mlog_printk(const u64 *mask, const char *func, int line,
+void __mlog_printk(const u64 mask, const char *func, int line,
   const char *fmt, ...)
 {
struct va_format vaf;
@@ -72,14 +72,14 @@ void __mlog_printk(const u64 *mask, const char *func, int 
line,
const char *level;
const char *prefix = "";
 
-   if (!__mlog_test_u64(*mask, mlog_and_bits) ||
-   __mlog_test_u64(*mask, mlog_not_bits))
+   if (!__mlog_test_u64(mask, mlog_and_bits) ||
+   __mlog_test_u64(mask, mlog_not_bits))
return;
 
-   if (*mask & ML_ERROR) {
+   if (mask & ML_ERROR) {
level = KERN_ERR;
prefix = "ERROR: ";
-   } else if (*mask & ML_NOTICE) {
+   } else if (mask & ML_NOTICE) {
level = KERN_NOTICE;
} else {
level = KERN_INFO;
diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
index 308ea0eb35fd..0d0f4bf2c3d8 100644
--- a/fs/ocfs2/cluster/masklog.h
+++ b/fs/ocfs2/cluster/masklog.h
@@ -163,7 +163,7 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
 #endif
 
 __printf(4, 5)
-void __mlog_printk(const u64 *m, const char *func, int line,
+void __mlog_printk(const u64 m, const char *func, int line,
   const char *fmt, ...);
 
 /*
@@ -174,7 +174,7 @@ void __mlog_printk(const u64 *m, const char *func, int line,
 do {   \
u64 _m = MLOG_MASK_PREFIX | (mask); \
if (_m & ML_ALLOWED_BITS)   \
-   __mlog_printk(&_m, __func__, __LINE__, fmt, \
+   __mlog_printk(_m, __func__, __LINE__, fmt,  \
  ##__VA_ARGS__);   \
 } while (0)
 
-- 
2.9.0

Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN

[Joe Perches]

On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote:
> The internal logging infrastructure in ocfs2 causes special warning code to be
> used with KASAN, which produces rather large stack frames:

> fs/ocfs2/super.c: In function 'ocfs2_fill_super':
> fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than 
> 3072 bytes [-Werror=frame-larger-than=]

At least by default it doesn't seem to.

gcc 6.2 allyesconfig, CONFIG_KASAN=y
with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE

gcc doesn't emit a stack warning

> By simply passing the mask by value instead of reference, we can avoid the
> problem completely.

Any idea why that's so?
 
>  On 64-bit architectures, this is also more efficient,

Efficient true, but the same overall stack no?

> while on the less common (at least among ocfs2 users) 32-bit architectures,
> I'm guessing that the resulting code is comparable to what it was before.
> 
> The current version was introduced by Joe Perches as an optimization, maybe
> he can see if my change regresses compared to his.

I don't see it.

> Cc: Joe Perches 
> Fixes: 7c2bd2f930ae ("ocfs2: reduce object size of mlog uses")
> Signed-off-by: Arnd Bergmann 
> ---
>  fs/ocfs2/cluster/masklog.c | 10 +-
>  fs/o cfs2/cluster/masklog.h |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
> index d331c2386b94..9720c5443e4d 100644
> --- a/fs/ocfs2/cluster/masklog.c
> +++ b/fs/ocfs2/cluster/masklog.c
> @@ -64,7 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, 
> size_t count)
>   return count;
>  }
>  
> -void __mlog_printk(const u64 *mask, const char *func, int line,
> +void __mlog_printk(const u64 mask, const char *func, int line,
>  const char *fmt, ...)
>  {
>   struct va_format vaf;
> @@ -72,14 +72,14 @@ void __mlog_printk(const u64 *mask, const char *func, int 
> line,
>   const char *level;
>   const char *prefix = "";
>  
> - if (!__mlog_test_u64(*mask, mlog_and_bits) ||
> - __mlog_test_u64(*mask, mlog_not_bits))
> + if (!__mlog_test_u64(mask, mlog_and_bits) ||
> + __mlog_test_u64(mask, mlog_not_bits))
>   return;
>  
> - if (*mask & ML_ERROR) {
> + if (mask & ML_ERROR) {
>   level = KERN_ERR;
>   prefix = "ERROR: ";
> - } else if (*mask & ML_NOTICE) {
> + } else if (mask & ML_NOTICE) {
>   level = KERN_NOTICE;
>   } else {
>   level = KERN_INFO;
> diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
> index 308ea0eb35fd..0d0f4bf2c3d8 100644
> --- a/fs/ocfs2/cluster/masklog.h
> +++ b/fs/ocfs2/cluster/masklog.h
> @@ -163,7 +163,7 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
>  #endif
>  
>  __printf(4, 5)
> -void __mlog_printk(const u64 *m, const char *func, int line,
> +void __mlog_printk(const u64 m, const char *func, int line,
>  const char *fmt, ...);
>  
>  /*
> @@ -174,7 +174,7 @@ void __mlog_printk(const u64 *m, const char *func, int 
> line,
>  do { \
>   u64 _m = MLOG_MASK_PREFIX | (mask); \
>   if (_m & ML_ALLOWED_BITS)   \
> - __mlog_printk(&_m, __func__, __LINE__, fmt, \
> + __mlog_printk(_m, __func__, __LINE__, fmt,  \
> ##__VA_ARGS__);   \
>  } while (0)
>