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;
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.