| Message ID | 1415969446-26356-2-git-send-email-a.ryabinin@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2014-11-14 at 15:50 +0300, Andrey Ryabinin wrote: > On architectures that have support for efficient unaligned access > struct printk_log has 4-byte alignment. > Specify alignment attribute in type declaration. > > The whole point of this patch is to fix deadlock which happening > when UBSan detects unaligned access in printk() thus UBSan recursively > calls printk() with logbuf_lock held by top printk() call. [] > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c [] > @@ -223,7 +223,11 @@ struct printk_log { > u8 facility; /* syslog facility */ > u8 flags:5; /* internal record flags */ > u8 level:3; /* syslog level */ > -}; > +} > +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > +__packed __aligned(4) > +#endif Why is adding __packed useful or __aligned(4) useful? The struct is naturally aligned on u64 and should be the size of 2 u64s. struct printk_log { u64 ts_nsec; /* timestamp in nanoseconds */ u16 len; /* length of entire record */ u16 text_len; /* length of text buffer */ u16 dict_len; /* length of dictionary buffer */ u8 facility; /* syslog facility */ u8 flags:5; /* internal record flags */ u8 level:3; /* syslog level */ }; Is there any case when it's not sizeof(u64) * 2? -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-11-14 20:22 GMT+03:00 Joe Perches <joe@perches.com>: > On Fri, 2014-11-14 at 15:50 +0300, Andrey Ryabinin wrote: >> On architectures that have support for efficient unaligned access >> struct printk_log has 4-byte alignment. >> Specify alignment attribute in type declaration. >> >> The whole point of this patch is to fix deadlock which happening >> when UBSan detects unaligned access in printk() thus UBSan recursively >> calls printk() with logbuf_lock held by top printk() call. > [] >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > [] >> @@ -223,7 +223,11 @@ struct printk_log { >> u8 facility; /* syslog facility */ >> u8 flags:5; /* internal record flags */ >> u8 level:3; /* syslog level */ >> -}; >> +} >> +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS >> +__packed __aligned(4) >> +#endif > > Why is adding __packed useful or __aligned(4) useful? > On x86_64, from compiler's point of view, 'struct printk_log' should be 8-byte aligned. But it stored in log buffer only with 4-byte alignment (LOG_ALIGN define). This inconsistency makes UBSAN unhappy. UBSAN uses printk to report errors, thus any access to 4-byte aligned 'struct printk_log' causing recursive printk() call from ubsan handler. And if logbuf_lock was taken we end up with deadlock. Specifying alignment removes this inconsistency. Now compiler knows that 'printk_log' actually aligned on 4, and it makes UBSAN happy and silent. Attribute 'aligned' without attribute 'packed' can only increase alignment. So __packed is used here because we need to decrease alignment from 8 to 4. > The struct is naturally aligned on u64 and should be Not, always. On 32-bits it will be 4-bytes aligned, on 64-bits - 8-bytes aligned > the size of 2 u64s. > > struct printk_log { > u64 ts_nsec; /* timestamp in nanoseconds */ > u16 len; /* length of entire record */ > u16 text_len; /* length of text buffer */ > u16 dict_len; /* length of dictionary buffer */ > u8 facility; /* syslog facility */ > u8 flags:5; /* internal record flags */ > u8 level:3; /* syslog level */ > }; > > Is there any case when it's not sizeof(u64) * 2? > I think no. As I said __packed used for decreasing alignment only. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index ced2b84..39be027 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -223,7 +223,11 @@ struct printk_log { u8 facility; /* syslog facility */ u8 flags:5; /* internal record flags */ u8 level:3; /* syslog level */ -}; +} +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS +__packed __aligned(4) +#endif +; /* * The logbuf_lock protects kmsg buffer, indices, counters. This can be taken @@ -261,11 +265,7 @@ static u32 clear_idx; #define LOG_LINE_MAX (1024 - PREFIX_MAX) /* record buffer */ -#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) -#define LOG_ALIGN 4 -#else #define LOG_ALIGN __alignof__(struct printk_log) -#endif #define __LOG_BUF_LEN (1 << CONFIG_LOG_BUF_SHIFT) static char __log_buf[__LOG_BUF_LEN] __aligned(LOG_ALIGN); static char *log_buf = __log_buf;
On architectures that have support for efficient unaligned access struct printk_log has 4-byte alignment. Specify alignment attribute in type declaration. The whole point of this patch is to fix deadlock which happening when UBSan detects unaligned access in printk() thus UBSan recursively calls printk() with logbuf_lock held by top printk() call. Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- kernel/printk/printk.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)