On Mar 12, 2012, at 11:04 AM, J. Bruce Fields wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>>
>>>> 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).
The patch fixes nfs4_verifiers, of which cl_confirm is one. We can't guarantee access to an nfs4_verifier field, which is an array of char, using (u32 *). It just happens to work now on architectures we test regularly.
This seems perfectly relevant to the patch description to me. Do you still want this hunk split into a separate patch?
--
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
