Bug report: unaligned access with ext4 encryption
diff mbox series

Message ID 20181230162906.GI27785@darkstar.musicnaut.iki.fi
State Not Applicable
Headers show
Series
  • Bug report: unaligned access with ext4 encryption
Related show

Commit Message

Aaro Koskinen Dec. 30, 2018, 4:29 p.m. UTC
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.

Comments

Eric Biggers Jan. 3, 2019, 5:17 p.m. UTC | #1
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
David Howells Jan. 4, 2019, 5:28 p.m. UTC | #2
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
Aaro Koskinen Jan. 10, 2019, 10:29 p.m. UTC | #3
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.
Eric Biggers Jan. 10, 2019, 11:01 p.m. UTC | #4
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
Aaro Koskinen Jan. 10, 2019, 11:35 p.m. UTC | #5
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.

Patch
diff mbox series

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;