Chuck Lever wrote:
> The sc_name field is currently 56 bytes long. This is not large enough
> to hold a pair of IPv6 addresses, the authentication type, the protocol
> name, and a uniquifier number. The maximum possible size of the name
> string using IPv6 addresses is just under 110 bytes, so I increased the
> size of the sc_name field to accomodate this maximum.
>
> In addition, the strings in the nfs4_setclientid structure are
> constructed with scnprintf(), which wants to terminate its output with
> '\0'. The sc_netid field was large enough only for a three byte netid
> string and a '\0' so inet6 netids were being truncated. Perhaps we
> don't need the overhead of scnprintf() to do a simple string copy, but
> I fixed this by increasing the size of the buffer by one byte.
>
> Since all three of the string buffers in nfs4_setclientid are
> constructed with scnprintf(), I increased the size of all three by one
> byte to document the requirement, although I don't think either the
> universal address field or the name field will be so small that these
> strings get truncated in this way.
>
> The size of the Linux client's client ID on the wire will be larger
> than before. RFC 3530 suggests the size limit for client IDs is 1024,
> and we are still well below that.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> include/linux/nfs_xdr.h | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 8c77c11..dc34977 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -672,16 +672,16 @@ struct nfs4_rename_res {
> struct nfs_fattr * new_fattr;
> };
>
> -#define NFS4_SETCLIENTID_NAMELEN (56)
> +#define NFS4_SETCLIENTID_NAMELEN (128)
>
Perhaps (127) might have been a better choice here? In the
struct below, the arrays end up being allocated to (128) + 1
plus whatever alignment bytes are valid for the platform.
This wastes a fair amount of space.
ps
> struct nfs4_setclientid {
> const nfs4_verifier * sc_verifier;
> unsigned int sc_name_len;
> - char sc_name[NFS4_SETCLIENTID_NAMELEN];
> + char sc_name[NFS4_SETCLIENTID_NAMELEN + 1];
> u32 sc_prog;
> unsigned int sc_netid_len;
> - char sc_netid[RPCBIND_MAXNETIDLEN];
> + char sc_netid[RPCBIND_MAXNETIDLEN + 1];
> unsigned int sc_uaddr_len;
> - char sc_uaddr[RPCBIND_MAXUADDRLEN];
> + char sc_uaddr[RPCBIND_MAXUADDRLEN + 1];
> u32 sc_cb_ident;
> };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sep 25, 2008, at 1:35 PM, Peter Staubach wrote:
> Chuck Lever wrote:
>> The sc_name field is currently 56 bytes long. This is not large
>> enough
>> to hold a pair of IPv6 addresses, the authentication type, the
>> protocol
>> name, and a uniquifier number. The maximum possible size of the name
>> string using IPv6 addresses is just under 110 bytes, so I increased
>> the
>> size of the sc_name field to accomodate this maximum.
>>
>> In addition, the strings in the nfs4_setclientid structure are
>> constructed with scnprintf(), which wants to terminate its output
>> with
>> '\0'. The sc_netid field was large enough only for a three byte
>> netid
>> string and a '\0' so inet6 netids were being truncated. Perhaps we
>> don't need the overhead of scnprintf() to do a simple string copy,
>> but
>> I fixed this by increasing the size of the buffer by one byte.
>>
>> Since all three of the string buffers in nfs4_setclientid are
>> constructed with scnprintf(), I increased the size of all three by
>> one
>> byte to document the requirement, although I don't think either the
>> universal address field or the name field will be so small that these
>> strings get truncated in this way.
>>
>> The size of the Linux client's client ID on the wire will be larger
>> than before. RFC 3530 suggests the size limit for client IDs is
>> 1024,
>> and we are still well below that.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> include/linux/nfs_xdr.h | 8 ++++----
>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>> index 8c77c11..dc34977 100644
>> --- a/include/linux/nfs_xdr.h
>> +++ b/include/linux/nfs_xdr.h
>> @@ -672,16 +672,16 @@ struct nfs4_rename_res {
>> struct nfs_fattr * new_fattr;
>> };
>> -#define NFS4_SETCLIENTID_NAMELEN (56)
>> +#define NFS4_SETCLIENTID_NAMELEN (128)
>>
>
> Perhaps (127) might have been a better choice here? In the
> struct below, the arrays end up being allocated to (128) + 1
> plus whatever alignment bytes are valid for the platform.
> This wastes a fair amount of space.
Probably the worst alignment padding would be 7 bytes. The struct is
already large, and is only used once on the stack just after mounting,
so I'm not that concerned. I would rather have a little extra here
than not enough.
However, we can trim the length of the name a little. I counted about
105 characters maximum in my simple calculations. We could probably
make it (111) to round out the field to 112 bytes (divisible by 8).
> ps
>
>> struct nfs4_setclientid {
>> const nfs4_verifier * sc_verifier;
>> unsigned int sc_name_len;
>> - char sc_name[NFS4_SETCLIENTID_NAMELEN];
>> + char sc_name[NFS4_SETCLIENTID_NAMELEN + 1];
>> u32 sc_prog;
>> unsigned int sc_netid_len;
>> - char sc_netid[RPCBIND_MAXNETIDLEN];
>> + char sc_netid[RPCBIND_MAXNETIDLEN + 1];
>> unsigned int sc_uaddr_len;
>> - char sc_uaddr[RPCBIND_MAXUADDRLEN];
>> + char sc_uaddr[RPCBIND_MAXUADDRLEN + 1];
>> u32 sc_cb_ident;
>> };
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-
>> nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com