From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [Bugme-new] [Bug 12995] New: NFS mount from avr32 platform crashes on 2.6.29 Date: Wed, 1 Apr 2009 13:46:27 -0700 Message-ID: <20090401134627.cb950b07.akpm@linux-foundation.org> References: <20090401124113.9026ea09.akpm@linux-foundation.org> <20090401130726.8a305e43.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Cc: brm-EoLPgbz1DBzQT0dZR+AlfA@public.gmane.org, bugme-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r@public.gmane.org, okir@suse.de, linux-nfs@vger.kernel.org, stable@kernel.org To: Chuck Lever Return-path: Received: from smtp1.linux-foundation.org ([140.211.169.13]:46240 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752363AbZDAUvO (ORCPT ); Wed, 1 Apr 2009 16:51:14 -0400 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, 1 Apr 2009 16:34:53 -0400 Chuck Lever wrote: > On Apr 1, 2009, at 4:07 PM, Andrew Morton wrote: > > (cc stable) > > > > On Wed, 1 Apr 2009 15:50:51 -0400 > > Chuck Lever wrote: > > > >>>> > >>>> Perhaps the solution is to malloc the nsm_private data and sm_priv > >>>> is then a > >>>> pointer to this data. This would guarantee the correct alignment. > >>>> > >>> > >>> nsm_private is: > >>> > >>> struct nsm_private { > >>> unsigned char data[SM_PRIV_SIZE]; > >>> }; > >>> > >>> so the compiler is permitted to byte-align this. > >>> > >>> I assume that some code somewhere is accessing this with a > >>> larger-than-one-byte typecast? > >> > >> nsm_init_private(). > >> > >> A patch just went upstream for 2.6.30 that addresses that > > > > commit ad5b365c1266b0c9e8e254a3c1cc4ef66bf33cba > > Author: Mans Rullgard > > AuthorDate: Sat Mar 28 19:55:20 2009 +0000 > > Commit: Trond Myklebust > > CommitDate: Wed Apr 1 13:24:14 2009 -0400 > > > > NSM: Fix unaligned accesses in nsm_init_private() > > > > This fixes unaligned accesses in nsm_init_private() when > > creating nlm_reboot keys. > > > > Signed-off-by: Mans Rullgard > > Reviewed-by: Chuck Lever > > Signed-off-by: Trond Myklebust > > > > diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c > > index 5e2c4d5..6d5d4a4 100644 > > --- a/fs/lockd/mon.c > > +++ b/fs/lockd/mon.c > > @@ -16,6 +16,8 @@ > > #include > > #include > > > > +#include > > + > > #define NLMDBG_FACILITY NLMDBG_MONITOR > > #define NSM_PROGRAM 100024 > > #define NSM_VERSION 1 > > @@ -274,10 +276,12 @@ static void nsm_init_private(struct nsm_handle > > *nsm) > > { > > u64 *p = (u64 *)&nsm->sm_priv.data; > > struct timespec ts; > > + s64 ns; > > > > ktime_get_ts(&ts); > > - *p++ = timespec_to_ns(&ts); > > - *p = (unsigned long)nsm; > > + ns = timespec_to_ns(&ts); > > + put_unaligned(ns, p); > > + put_unaligned((unsigned long)nsm, p + 1); > > } > > > > static struct nsm_handle *nsm_create_handle(const struct sockaddr > > *sap, > > > > Is this the best way of fixing it? We'll crash again if some other > > code starts to access .data with non-byte-sized accesses. What makes > > this especially risky is that the code will pass testing on x86. > > > > __aligned would be one way. But it would be far far cleaner to just > > do > > > > > > --- a/include/linux/lockd/xdr.h~a > > +++ a/include/linux/lockd/xdr.h > > @@ -17,7 +17,7 @@ > > #define SM_PRIV_SIZE 16 > > > > struct nsm_private { > > - unsigned char data[SM_PRIV_SIZE]; > > + u64 data[SM_PRIV_SIZE/sizeof(u64)]; > > }; > > > > struct svc_rqst; > > > > all the typecasting and the put_unaligned()s just go away then. > > True enough. It should be noted, however, that opaque data types are > defined as character arrays in most RPC implementations, and by > automated code generators like rpcgen. The size of an RPC opaque type > is usually expressed by a number of octets. > > In general it may be best to keep that tradition (due to a large body > of legacy code) and always use __aligned, simply because that clearly > documents the problem that is being fixed. > struct nsm_private { union { unsigned char data[SM_PRIV_SIZE]; u64 aligned_data[SM_PRIV_SIZE/sizeof(u64)]; }; }; :)