* Bug report: unaligned access with ext4 encryption
@ 2018-12-30 16:29 Aaro Koskinen
2019-01-03 17:17 ` Eric Biggers
2019-01-04 17:28 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: Aaro Koskinen @ 2018-12-30 16:29 UTC (permalink / raw)
To: Theodore Y. Ts'o, Jaegeuk Kim, David Howells
Cc: linux-fscrypt, linux-kernel, keyrings
Hi,
When using ext4 encryption on SPARC, there's plenty of dmesg noise about
unaligned access:
[ 167.269526] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
[ 167.270152] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
[ 181.087451] log_unaligned: 5 callbacks suppressed
[ 181.087511] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
[ 181.092435] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
[ 181.095816] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
And also seen on an ARM machine:
$ cat /proc/cpu/alignment
User: 0
System: 1028193 (find_and_lock_process_key+0x84/0x10c)
Skipped: 0
Half: 0
Word: 1028193
DWord: 0
Multi: 0
User faults: 0 (ignored)
Looks like user_key_payload layout is not optimal when data address
is used for fscrypt_key... I tried the below change and got rid of the
messages. Not sure what the proper fix should be?
A.
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index e098cbe27db5..6495ffcfe510 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -31,7 +31,7 @@
struct user_key_payload {
struct rcu_head rcu; /* RCU destructor */
unsigned short datalen; /* length of this data */
- char data[0]; /* actual data */
+ char data[0] __aligned(4); /* actual data */
};
extern struct key_type key_type_user;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Bug report: unaligned access with ext4 encryption
2018-12-30 16:29 Bug report: unaligned access with ext4 encryption Aaro Koskinen
@ 2019-01-03 17:17 ` Eric Biggers
2019-01-04 17:28 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2019-01-03 17:17 UTC (permalink / raw)
To: Aaro Koskinen, David Howells
Cc: Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt, linux-kernel, keyrings
On Sun, Dec 30, 2018 at 06:29:06PM +0200, Aaro Koskinen wrote:
> Hi,
>
> When using ext4 encryption on SPARC, there's plenty of dmesg noise about
> unaligned access:
>
> [ 167.269526] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
> [ 167.270152] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
> [ 181.087451] log_unaligned: 5 callbacks suppressed
> [ 181.087511] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
> [ 181.092435] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
> [ 181.095816] Kernel unaligned access at TPC[5497a0] find_and_lock_process_key+0x80/0x120
>
> And also seen on an ARM machine:
>
> $ cat /proc/cpu/alignment
> User: 0
> System: 1028193 (find_and_lock_process_key+0x84/0x10c)
> Skipped: 0
> Half: 0
> Word: 1028193
> DWord: 0
> Multi: 0
> User faults: 0 (ignored)
>
> Looks like user_key_payload layout is not optimal when data address
> is used for fscrypt_key... I tried the below change and got rid of the
> messages. Not sure what the proper fix should be?
>
> A.
>
> diff --git a/include/keys/user-type.h b/include/keys/user-type.h
> index e098cbe27db5..6495ffcfe510 100644
> --- a/include/keys/user-type.h
> +++ b/include/keys/user-type.h
> @@ -31,7 +31,7 @@
> struct user_key_payload {
> struct rcu_head rcu; /* RCU destructor */
> unsigned short datalen; /* length of this data */
> - char data[0]; /* actual data */
> + char data[0] __aligned(4); /* actual data */
> };
>
> extern struct key_type key_type_user;
>
Hi Aaro, thanks for the bug report! I think you're on the right track; it makes
much more sense to have the keyrings subsystem store the payload with better
alignment, than to work around the 2-byte alignment in fscrypt.
But how about '__aligned(__alignof__(u64))' instead? 4 bytes may not be enough.
David, what do you think?
- Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: unaligned access with ext4 encryption
2018-12-30 16:29 Bug report: unaligned access with ext4 encryption Aaro Koskinen
2019-01-03 17:17 ` Eric Biggers
@ 2019-01-04 17:28 ` David Howells
2019-01-10 22:29 ` Aaro Koskinen
1 sibling, 1 reply; 6+ messages in thread
From: David Howells @ 2019-01-04 17:28 UTC (permalink / raw)
To: Eric Biggers
Cc: dhowells, Aaro Koskinen, Theodore Y. Ts'o, Jaegeuk Kim,
linux-fscrypt, linux-kernel, keyrings
Eric Biggers <ebiggers@kernel.org> wrote:
> Hi Aaro, thanks for the bug report! I think you're on the right track; it makes
> much more sense to have the keyrings subsystem store the payload with better
> alignment, than to work around the 2-byte alignment in fscrypt.
>
> But how about '__aligned(__alignof__(u64))' instead? 4 bytes may not be enough.
>
> David, what do you think?
Does that even work?
Might be better to just insert 6 bytes of padding with a comment, but yes I
agree that it's probably better to align it to at least machine word size.
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: unaligned access with ext4 encryption
2019-01-04 17:28 ` David Howells
@ 2019-01-10 22:29 ` Aaro Koskinen
2019-01-10 23:01 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Aaro Koskinen @ 2019-01-10 22:29 UTC (permalink / raw)
To: David Howells
Cc: Eric Biggers, Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt,
linux-kernel, keyrings
Hi,
On Fri, Jan 04, 2019 at 05:28:02PM +0000, David Howells wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> > Hi Aaro, thanks for the bug report! I think you're on the right track; it makes
> > much more sense to have the keyrings subsystem store the payload with better
> > alignment, than to work around the 2-byte alignment in fscrypt.
> >
> > But how about '__aligned(__alignof__(u64))' instead? 4 bytes may not be enough.
> >
> > David, what do you think?
>
> Does that even work?
That should work.
> Might be better to just insert 6 bytes of padding with a comment, but yes I
> agree that it's probably better to align it to at least machine word size.
Padding is fragile, e.g. if struct rcu_head changes. Using __aligned should
make it always right automatically.
A.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: unaligned access with ext4 encryption
2019-01-10 22:29 ` Aaro Koskinen
@ 2019-01-10 23:01 ` Eric Biggers
2019-01-10 23:35 ` Aaro Koskinen
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2019-01-10 23:01 UTC (permalink / raw)
To: Aaro Koskinen
Cc: David Howells, Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt,
linux-kernel, keyrings
On Fri, Jan 11, 2019 at 12:29:28AM +0200, Aaro Koskinen wrote:
> Hi,
>
> On Fri, Jan 04, 2019 at 05:28:02PM +0000, David Howells wrote:
> > Eric Biggers <ebiggers@kernel.org> wrote:
> > > Hi Aaro, thanks for the bug report! I think you're on the right track; it makes
> > > much more sense to have the keyrings subsystem store the payload with better
> > > alignment, than to work around the 2-byte alignment in fscrypt.
> > >
> > > But how about '__aligned(__alignof__(u64))' instead? 4 bytes may not be enough.
> > >
> > > David, what do you think?
> >
> > Does that even work?
>
> That should work.
>
> > Might be better to just insert 6 bytes of padding with a comment, but yes I
> > agree that it's probably better to align it to at least machine word size.
>
> Padding is fragile, e.g. if struct rcu_head changes. Using __aligned should
> make it always right automatically.
>
> A.
I agree that __aligned is better. It should work; see 'struct crypto_tfm' in
include/linux/crypto.h for another example of a struct that uses __aligned on a
flexible array at the end.
Aaro, can you send a formal patch? If you don't I'll do so, but I figure I'll
ask first.
Thanks,
- Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: unaligned access with ext4 encryption
2019-01-10 23:01 ` Eric Biggers
@ 2019-01-10 23:35 ` Aaro Koskinen
0 siblings, 0 replies; 6+ messages in thread
From: Aaro Koskinen @ 2019-01-10 23:35 UTC (permalink / raw)
To: Eric Biggers
Cc: David Howells, Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt,
linux-kernel, keyrings
Hi,
On Thu, Jan 10, 2019 at 03:01:14PM -0800, Eric Biggers wrote:
> On Fri, Jan 11, 2019 at 12:29:28AM +0200, Aaro Koskinen wrote:
> > Hi,
> >
> > On Fri, Jan 04, 2019 at 05:28:02PM +0000, David Howells wrote:
> > > Eric Biggers <ebiggers@kernel.org> wrote:
> > > > Hi Aaro, thanks for the bug report! I think you're on the right track; it makes
> > > > much more sense to have the keyrings subsystem store the payload with better
> > > > alignment, than to work around the 2-byte alignment in fscrypt.
> > > >
> > > > But how about '__aligned(__alignof__(u64))' instead? 4 bytes may not be enough.
> > > >
> > > > David, what do you think?
> > >
> > > Does that even work?
> >
> > That should work.
> >
> > > Might be better to just insert 6 bytes of padding with a comment, but yes I
> > > agree that it's probably better to align it to at least machine word size.
> >
> > Padding is fragile, e.g. if struct rcu_head changes. Using __aligned should
> > make it always right automatically.
> >
> > A.
>
> I agree that __aligned is better. It should work; see 'struct crypto_tfm' in
> include/linux/crypto.h for another example of a struct that uses __aligned on a
> flexible array at the end.
>
> Aaro, can you send a formal patch? If you don't I'll do so, but I figure I'll
> ask first.
Please go ahead; I'd prefer if you send the patch, I will then test it
on SPARC and reply with Tested-by (if it works :).
A.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-10 23:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-30 16:29 Bug report: unaligned access with ext4 encryption Aaro Koskinen
2019-01-03 17:17 ` Eric Biggers
2019-01-04 17:28 ` David Howells
2019-01-10 22:29 ` Aaro Koskinen
2019-01-10 23:01 ` Eric Biggers
2019-01-10 23:35 ` Aaro Koskinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).