On Wed, 30 Jan 2013 23:19:20 +0000 "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote: > > -----Original Message----- > > From: linux-nfs-owner@xxxxxxxxxxxxxxx [mailto:linux-nfs- > > owner@xxxxxxxxxxxxxxx] On Behalf Of Chuck Lever > > Sent: Thursday, January 24, 2013 11:14 AM > > To: NeilBrown > > Cc: Kevin Coffman; J.Bruce Fields; Steve Dickson; NFS > > Subject: Re: Corrupted RPC_GSS_PROC_DESTROY packets coming from Linux > > servers. > > > > > > On Jan 23, 2013, at 9:02 PM, NeilBrown <neilb@xxxxxxx> wrote: > > > > > > > > Hi peoples, > > > > > > this issue has appeared on the mailing list before (particularly > > > around July > > > 2011) but hasn't been resolved yet and it just bit me again so I > > > figure it is time it got fixed. > > > > > > > > > If you tcpdump the network connection while mounting an NFS filesystem > > > using kerberos - or while the client is establishing a new context > > > because e.g. the server rebooted - you will see a NULL RPC with an > > > RPC_GSS_PROC_DESTROY credential but no verifier. The lack of a > > > verifier makes the packet corrupt so the server ignores it, but people > > > see it and think something is wrong. > > > > > > It is good that the server ignores it as it really shouldn't be there. > > > What happens is that the NFS client calls up to rpc.gssd to request a > > > credential. rpc.gssd then establishes a connection directly with the > > > server, including the establishment of the security context. Then it > > > gathers the context details and passed them down to the kernel. > > > Then it closes the connection part of which involves calling > > > AUTH_DESTROY(auth) - necessary to free up data structures and not leak > > > memory. > > > This AUTH_DESTROY tries to destroy the context completely, including > > > telling the server that it has been destroyed! But it hasn't, it has > > > just been passed down to the kernel for use on a different connection. > > > > > > So there are two issues here: > > > - why is the GSS_PROC_DESTROY packet missing a verifier > > > - how can we get AUTH_DESTROY to *not* try to destroy the context on > > the > > > server - as that would be a bad thing. > > > > > > The first I cannot completely answer. I do know that in libtirpc, in > > > auth_gss.c, in authgss_marshal(), gss_get_mic is failing because it doesn't > > > think it has a valid context. I don't know why it thinks that, and I don't > > > really care. > > > > > > > > > The second question is more interesting and I see two possible options. > > > > > > 1/ If we knew why gss_get_mic failed and had good reason to believe it > > > would keep on failing, we could consider changing clnt_vc_call to > > > respond to an error from AUTH_MARSHALL not by sending a truncated > > > packet, but by purging the current message and not sending it at all. > > > This should be possible but might be messy. > > > > > > 2/ Make libtirpc behave more like librpcsecgss. > > > In libtirpc, the authgss_get_private_data() function just hands over > > > a pointer to the private data, but keeps its own pointer so it can > > > free it when the client is finally destroyed. > > > > > > In librpcsecgss, since commit > > > 07fce317cac267509b944a8191cafa8e49b5e328 > > > (thanks Kevin), authgss_get_private_data() hands the data over to the > > > caller and doesn't keep it's own reference to it. So the caller has > > > to call > > > authgss_free_private_data() when it has finished with the data. > > > As the library no longer has the credential, it doesn't even bother > > > trying to send a GSS_PROC_DESTROY request. > > > > > > When Chuck noticed this difference between the two libraries, he > > > resolved it - in commit 336f8bca825416082d62ef38314f3e0b7e8f5cc2 as > > follow: > > > > > > if (token.value) > > > free(token.value); > > > +#ifndef HAVE_LIBTIRPC > > > if (pd.pd_ctx_hndl.length != 0) > > > authgss_free_private_data(&pd); > > > +#endif > > > > > > Clearly to significance of this difference was not obvious, and this > > > was the easiest fix. > > > > > > If we were to "fix" this properly, we would need to add a commit like > > > the one from Kevin to libtirpc, and remove that #ifndef from nfs-utils. > > > co-ordinating this might be tricking. nfs-utils could presumably > > > test if libtirpc provided the function (at configure time) and call > > > it if it does, > > > > This seems to me like the best approach for 2. > > > > > However is someone updates libtirpc without updating or recompiling > > > nfs-utils they would get a memory leak. May it would be slow enough > > > not to be serious, and if anyone noticed that could just upgrade and get a > > fix. > > > > Telling people to upgrade for a fix is what we do for a living. In all > > seriousness, though, in the common case, people will be using nfs-utils and > > libtirpc built by distributions, and we expect the distros will get the fix > > dependency right over time. > > > > > Does this seem reasonable? How is maintaining libtirpc these days? > > > Could we get the fix into 0.2.3, or would we need a minor version > > > bump to 0.3.0?? > > > > A minor version bump shouldn't be necessary if we're not changing the > > synopsis of a published API, nor are we removing a published API. > > > > > 3/ there is actually a third option. We could change > > > authgss_get_private_data() to set gc.gc_ctx.length to 0, but not free > > > the buffer. Then aithgss_destroy_context() could notice that the > > > length is zero and the buffer is not NULL, and could free the buffer but not > > try to send > > > the context_destroy request. It's an ugly hack though and I think I'd > > > rather not. > > 4/ Have authgss_get_private_data() consume the 'auth' argument. > > Reusing the auth in an RPC call after we've transferred the context to the kernel is in any case a bug, so why allow it at all? > This is exactly the same as '2' - though stated much more succinctly. It looks like it is the approach that everyone prefers. I'll send some patches. Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
