On Apr 1, 2009, at 4:07 PM, Andrew Morton wrote:
(cc stable)
On Wed, 1 Apr 2009 15:50:51 -0400
Chuck Lever <chuck.lever@xxxxxxxxxx> 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 <mans@xxxxxxxxx>
AuthorDate: Sat Mar 28 19:55:20 2009 +0000
Commit: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
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 <mans@xxxxxxxxx>
Reviewed-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
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 <linux/sunrpc/svc.h>
#include <linux/lockd/lockd.h>
+#include <asm/unaligned.h>
+
#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.
but did not mention that it fixed an oops.
That was a fairly significant failing. Guys, please please do
remember
that we're also maintaining the stable kernels. When merging a patch
please always think whether it needs to be backported.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
