| Message ID | 20170302163834.2273519-25-arnd@arndb.de |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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 --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)
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(-)