| Message ID | 20181230162906.GI27785@darkstar.musicnaut.iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug report: unaligned access with ext4 encryption | expand |
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
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
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.
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
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.
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;