linux-kernel - Re: [PATCH v2 1/2] kernel: printk: specify alignment for struct printk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPAsAGwCJqBeoQRb5AwNdkFDRLvqk8WhE3tKcAYXWs5W_r-fDA@mail.gmail.com>
Date:	Sat, 15 Nov 2014 00:54:24 +0400
From:	Andrey Ryabinin <ryabinin.a.a@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Andrey Ryabinin <a.ryabinin@...sung.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Sasha Levin <sasha.levin@...cle.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Jonathan Corbet <corbet@....net>,
	Michal Marek <mmarek@...e.cz>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Yury Gribov <y.gribov@...sung.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Konstantin Khlebnikov <koct9i@...il.com>,
	"x86@...nel.org" <x86@...nel.org>, linux-doc@...r.kernel.org,
	linux-kbuild@...r.kernel.org
Subject: Re: [PATCH v2 1/2] kernel: printk: specify alignment for struct printk_log

2014-11-14 20:22 GMT+03:00 Joe Perches <joe@...ches.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/