On Fri, Apr 12, 2013 at 06:21:11PM +0000, Myklebust, Trond wrote: > > -----Original Message----- > > From: J. Bruce Fields [mailto:bfields@xxxxxxxxxxxx] > > Sent: Friday, April 12, 2013 2:12 PM > > To: Myklebust, Trond > > Cc: J. Bruce Fields; linux-nfs@xxxxxxxxxxxxxxx; chuck.lever@xxxxxxxxxx; > > simo@xxxxxxxxxx > > Subject: Re: [PATCH 5/6] SUNRPC: Add RPC based upcall mechanism for > > RPCGSS auth > > > > Trond, apologies, I'm just getting back to this and had a couple > > questions: > > > > On Thu, Feb 21, 2013 at 06:35:46PM +0000, Myklebust, Trond wrote: > > > On Thu, 2013-02-21 at 11:38 -0500, J. Bruce Fields wrote: > > > > +static int gssx_dec_buffer(struct xdr_stream *xdr, > > > > + gssx_buffer *buf) > > > > +{ > > > > + u32 length; > > > > + __be32 *p; > > > > + > > > > + p = xdr_inline_decode(xdr, 4); > > > > + if (unlikely(p == NULL)) > > > > + return -ENOSPC; > > > > + > > > > + length = be32_to_cpup(p); > > > > + p = xdr_inline_decode(xdr, length); > > > > > > combine for efficiency with the 4 byte allocation above. > > > > This was just a think-o on your part, right? There's no way to decode the > > whole thing in one gulp without knowing the length ahead of time. > > Or am I missing something? > > No. You are right, that was a brain-fart... > > > > > +static int gssx_dec_option_array(struct xdr_stream *xdr, > > > > + struct gssx_option_array *oa) > > > > +{ > > > > + struct svc_cred *creds; > > > > + u32 count, i; > > > > + __be32 *p; > > > > + int err; > > > > + > > > > + p = xdr_inline_decode(xdr, 4); > > > > + if (unlikely(p == NULL)) > > > > + return -ENOSPC; > > > > + count = be32_to_cpup(p++); > > > > + if (count != 0) { > > > > + /* we recognize only 1 currently: CREDS_VALUE */ > > > > + oa->count = 1; > > > > + > > > > + oa->data = kmalloc(sizeof(struct gssx_option), GFP_KERNEL); > > > > > > Sleeping kmallocs inside XDR encode/decode routines is strongly > > > discouraged. Particularly so for something that can be preallocated. > > > > And here: I don't mind doing that, but: there's no real problem here as long > > as we're only calling this stuff synchronously, right? > > If you can guarantee that it won't ever be called from rpciod, then fine. > However anything that might end up being called from a rpciod context must at the very least guarantee that there are no __GFP_FS allocations, since those can deadlock the NFS client. Got it, thanks --b. -- 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
