On Thu, Jan 24, 2013 at 11:13:51AM -0500, Chuck Lever wrote: > > 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. Yes, I hate to be lax about library/application compatibility, but looks like the only consequence of the incompatibility here is a small memory leak, and nfs-utils and libtirpc are probably normally upgraded at the same time, so I think we could live with that. --b. > > > 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. > > -- > 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
