On Wed, 1 Apr 2009 16:34:53 -0400
Chuck Lever <chuck.lever@xxxxxxxxxx> 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 <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.
>
struct nsm_private {
union {
unsigned char data[SM_PRIV_SIZE];
u64 aligned_data[SM_PRIV_SIZE/sizeof(u64)];
};
};
:)
--
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
