diff mbox

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

Message ID 20170302163834.2273519-25-arnd@arndb.de (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Arnd Bergmann March 2, 2017, 4:38 p.m. UTC
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 <joe@perches.com>
Fixes: 7c2bd2f930ae ("ocfs2: reduce object size of mlog uses")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/ocfs2/cluster/masklog.c | 10 +++++-----
 fs/ocfs2/cluster/masklog.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Arnd Bergmann March 2, 2017, 10:22 p.m. UTC | #1
On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches <joe@perches.com> 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.

>>  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: In function 'ocfs2_parse_options.isra.3':
fs/ocfs2/super.c:1508:1: error: the frame size of 352 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_enable_quotas':
fs/ocfs2/super.c:974:1: error: the frame size of 344 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_fill_super':
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: In function 'ocfs2_check_volume':
fs/ocfs2/super.c:2512:1: error: the frame size of 768 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_statfs':
fs/ocfs2/super.c:1717:1: error: the frame size of 320 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_parse_options.isra.3':
fs/ocfs2/super.c:1508:1: error: the frame size of 464 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_enable_quotas':
fs/ocfs2/super.c:974:1: error: the frame size of 320 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_remount':
fs/ocfs2/super.c:752:1: error: the frame size of 568 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
fs/ocfs2/super.c: In function 'ocfs2_initialize_super.isra.8':
fs/ocfs2/super.c:2339:1: error: the frame size of 1712 bytes is larger
than 300 bytes [-Werror=frame-larger-than=]
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 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=]

     Arnd
Joe Perches March 2, 2017, 10:40 p.m. UTC | #2
On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote:
> On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches <joe@perches.com> 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.
Arnd Bergmann March 2, 2017, 10:59 p.m. UTC | #3
On Thu, Mar 2, 2017 at 11:40 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote:
>> On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches <joe@perches.com> 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
diff mbox

Patch

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)