Re: [PATCH] NFSD: Fix nfs4_verifier memory alignment

["J. Bruce Fields" <bfields@redhat.com>]

On Mon, Mar 12, 2012 at 10:57:32AM -0400, Chuck Lever wrote:
> 
> On Mar 12, 2012, at 10:20 AM, J. Bruce Fields wrote:
> 
> > On Fri, Mar 02, 2012 at 05:13:50PM -0500, Chuck Lever wrote:
> >> Clean up due to code review.
> >> 
> >> The nfs4_verifier's data field is not guaranteed to be u32-aligned.
> >> Casting an array of chars to a u32 * is considered generally
> >> hazardous.
> >> 
> >> We can fix most of this by using a __be32 array to generate the
> >> verifier's contents and then byte-copying it into the verifier field.
> >> 
> >> However, there is one spot where there is a backwards compatibility
> >> constraint: the do_nfsd_create() call expects a verifier which is
> >> 32-bit aligned.  Fix this spot by forcing the alignment of the create
> >> verifier in the nfsd4_open args structure.
> >> 
> >> Also, sizeof(nfs4_verifer) is the size of the in-core verifier data
> >> structure, but NFS4_VERIFIER_SIZE is the number of octets in an XDR'd
> >> verifier.  The two are not interchangeable, even if they happen to
> >> have the same value.
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> 
> >> Hi Bruce-
> >> 
> >> Compile-tested only.  Does this look reasonable?
> > 
> > Looks fine, but the setclientid verifier stuff belongs in a separate
> > patch.
> 
> Thanks for the review.  I'm not clear on exactly which hunks you would like split.

This:

> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index c5cddd6..9f0e139 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1138,12 +1138,12 @@ static void gen_clid(struct nfs4_client *clp)
> >> 
> >> static void gen_confirm(struct nfs4_client *clp)
> >> {
> >> +	__be32 verf[2];
> >> 	static u32 i;
> >> -	u32 *p;
> >> 
> >> -	p = (u32 *)clp->cl_confirm.data;
> >> -	*p++ = get_seconds();
> >> -	*p++ = i++;
> >> +	verf[0] = (__be32)get_seconds();
> >> +	verf[1] = (__be32)i++;
> >> +	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));

This cl_confirm verifier really has nothing to do with the write
verifier (though maybe it has a similar problem).

--b.