2008-09-25 16:21:52

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Thu, 2008-09-25 at 11:57 -0400, 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]>

ACK. I'll take this patch, but should Bruce take the other 2? I believe
he should already have other changes to rpcb_clnt.c in his tree...

Cheers
Trond

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com


2008-09-25 16:58:24

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
> ACK. I'll take this patch, but should Bruce take the other 2? I believe
> he should already have other changes to rpcb_clnt.c in his tree...

Yes I do; I'll take a look. (My goal is to get through my backlog from
Trond this afternoon....)

I recall one remaining uncertainty about the patches already in my
for-2.6.28: they allow building either a kernel that supports nfs/ipv6,
or a kernel that works with older nfs-utils, but not both.

I'd prefer a stricter level of backwards compatibility. The current
approach may be confusing to distributors, early adopters, and testers.
But I'm willing to settle for it and let it be a lesson to us if it
turns out to cause more problems than expected.

Talking to Trond the other day he asked why we couldn't use
PROG_MISMATCH (unsupported program version) error to fall back. Chuck
says in the changelog comment:

"I tried adding some automatic logic to fall back if registering
with a v4 protocol request failed, but there are too many corner
cases."

Which I can believe, though I haven't looked at it myself.

In any case I'd like Trond's ACK or NACK.

--b.

2008-09-25 17:00:53

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Thu, Sep 25, 2008 at 12:58:21PM -0400, bfields wrote:
> On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
> > ACK. I'll take this patch, but should Bruce take the other 2? I believe
> > he should already have other changes to rpcb_clnt.c in his tree...
>
> Yes I do; I'll take a look. (My goal is to get through my backlog from
> Trond this afternoon....)

(err, I meant "Chuck" not "Trond" there. You guys are so hard to tell
apart, ya know.)

--b.

>
> I recall one remaining uncertainty about the patches already in my
> for-2.6.28: they allow building either a kernel that supports nfs/ipv6,
> or a kernel that works with older nfs-utils, but not both.
>
> I'd prefer a stricter level of backwards compatibility. The current
> approach may be confusing to distributors, early adopters, and testers.
> But I'm willing to settle for it and let it be a lesson to us if it
> turns out to cause more problems than expected.
>
> Talking to Trond the other day he asked why we couldn't use
> PROG_MISMATCH (unsupported program version) error to fall back. Chuck
> says in the changelog comment:
>
> "I tried adding some automatic logic to fall back if registering
> with a v4 protocol request failed, but there are too many corner
> cases."
>
> Which I can believe, though I haven't looked at it myself.
>
> In any case I'd like Trond's ACK or NACK.
>
> --b.

2008-09-25 18:16:12

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
> On Thu, Sep 25, 2008 at 12:10:26PM -0400, Trond Myklebust wrote:
>> ACK. I'll take this patch, but should Bruce take the other 2? I
>> believe
>> he should already have other changes to rpcb_clnt.c in his tree...
>
> Yes I do; I'll take a look. (My goal is to get through my backlog
> from
> Trond this afternoon....)
>
>
> I recall one remaining uncertainty about the patches already in my
> for-2.6.28: they allow building either a kernel that supports nfs/
> ipv6,
> or a kernel that works with older nfs-utils, but not both.

That's not quite accurate.

The build option enables support for using rpcbindv4 upcalls.
Otherwise, the kernel will support NFS on IPv6 if CONFIG_IPV6 or
CONFIG_IPV6_MODULE are enabled, but NFSD and lockd won't start if the
rpcbind upcall fails.

> I'd prefer a stricter level of backwards compatibility. The current
> approach may be confusing to distributors, early adopters, and
> testers.

The issues are spelled out in the help text of
CONFIG_SUNRPC_REGISTER_V4, and it defaults to legacy behavior.

> But I'm willing to settle for it and let it be a lesson to us if it
> turns out to cause more problems than expected.

I will be here to fix it if there is a problem. In this case, this
whole NFS/IPv6 thing is so complicated that I'm implementing just what
is needed as we go along. We can fill in the niceties at a later point.

/me is taking his cue from "lazy evaluation."

> Talking to Trond the other day he asked why we couldn't use
> PROG_MISMATCH (unsupported program version) error to fall back. Chuck
> says in the changelog comment:
>
> "I tried adding some automatic logic to fall back if registering
> with a v4 protocol request failed, but there are too many corner
> cases."
>
> Which I can believe, though I haven't looked at it myself.

Here's what I can remember right off the top of my head:

1. The kernel has to detect whether the local rpcbind has an IPv6
listener or not.

2. The kernel has to detect whether user space has configured an IPv6
loopback address.

3. The kernel has to detect whether the local rpcbind speaks rpcbind
v4.

Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register()
can't contact rpcbind's IPv6 listener and issue a v4 SET request, it
fails and the RPC service is shut down.

The only area that might be trouble is when a sysadmin shuts off ALL
IPv6 in her network configuration, even if the kernel is build with
IPv6 support. The network layer should do the right thing and map the
IPv6 loopback address to the IPv4 loopback address automatically, but
I haven't tested this.

I'm also not convinced that people will try to install a 2.6 kernel on
a distribution that was built for 2.4 or earlier kernels. There are
too many missing pieces in the old distributions (like kernel module
utilities) to make it easy. So I'm not trying to make this compatible
with every distribution since the beginning.

> In any case I'd like Trond's ACK or NACK.
>
> --b.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-09-27 00:03:30

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Thu, Sep 25, 2008 at 02:13:11PM -0400, Chuck Lever wrote:
> On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
>> But I'm willing to settle for it and let it be a lesson to us if it
>> turns out to cause more problems than expected.
>
> I will be here to fix it if there is a problem. In this case, this
> whole NFS/IPv6 thing is so complicated that I'm implementing just what
> is needed as we go along. We can fill in the niceties at a later point.
>
> /me is taking his cue from "lazy evaluation."

OK, fair enough, barring any other objections.

> Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register() can't
> contact rpcbind's IPv6 listener and issue a v4 SET request, it fails and
> the RPC service is shut down.
>
> The only area that might be trouble is when a sysadmin shuts off ALL
> IPv6 in her network configuration, even if the kernel is build with IPv6
> support. The network layer should do the right thing and map the IPv6
> loopback address to the IPv4 loopback address automatically, but I
> haven't tested this.
>
> I'm also not convinced that people will try to install a 2.6 kernel on a
> distribution that was built for 2.4 or earlier kernels. There are too
> many missing pieces in the old distributions (like kernel module
> utilities) to make it easy. So I'm not trying to make this compatible
> with every distribution since the beginning.

Is it 2.4-era distributions that's really the issue?

I thought the userspace rpcbind stuff was still a bit experimental--so
that when the switchover is made to a kernel that supports rpcbind v4,
the userspace that's required will be more recent than that.

Just curious.

--b.

2008-10-01 15:45:47

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH 3/3] NFS: SETCLIENTID truncates client ID and netid

On Sep 26, 2008, at Sep 26, 2008, 8:03 PM, J. Bruce Fields wrote:
> On Thu, Sep 25, 2008 at 02:13:11PM -0400, Chuck Lever wrote:
>> On Sep 25, 2008, at 12:58 PM, J. Bruce Fields wrote:
>>> But I'm willing to settle for it and let it be a lesson to us if it
>>> turns out to cause more problems than expected.
>>
>> I will be here to fix it if there is a problem. In this case, this
>> whole NFS/IPv6 thing is so complicated that I'm implementing just
>> what
>> is needed as we go along. We can fill in the niceties at a later
>> point.
>>
>> /me is taking his cue from "lazy evaluation."
>
> OK, fair enough, barring any other objections.
>
>> Today, if CONFIG_SUNRPC_REGISTER_V4 is enabled and svc_register()
>> can't
>> contact rpcbind's IPv6 listener and issue a v4 SET request, it
>> fails and
>> the RPC service is shut down.
>>
>> The only area that might be trouble is when a sysadmin shuts off ALL
>> IPv6 in her network configuration, even if the kernel is build with
>> IPv6
>> support. The network layer should do the right thing and map the
>> IPv6
>> loopback address to the IPv4 loopback address automatically, but I
>> haven't tested this.
>>
>> I'm also not convinced that people will try to install a 2.6 kernel
>> on a
>> distribution that was built for 2.4 or earlier kernels. There are
>> too
>> many missing pieces in the old distributions (like kernel module
>> utilities) to make it easy. So I'm not trying to make this
>> compatible
>> with every distribution since the beginning.
>
> Is it 2.4-era distributions that's really the issue?

I'm merely trying to bound the amount of backwards-compatibility that
we realistically have to worry about. It could be more recent than
that, even.

> I thought the userspace rpcbind stuff was still a bit experimental--so
> that when the switchover is made to a kernel that supports rpcbind v4,
> the userspace that's required will be more recent than that.

You can drop libtirpc, rpcbind, and the latest nfs-utils on older
distributions without much difficulty.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com