Date: Mon, 2 Apr 2012 08:27:46 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-fs@freebsd.org Cc: alc@freebsd.org, Joel Ray Holveck <joelh@juniper.net>, David Wolfskill <david@catwhisker.org> Subject: Re: RFC: which patch is preferred for fix to PR#165923 Message-ID: <201204020827.46722.jhb@freebsd.org> In-Reply-To: <1184428460.2076443.1333328057090.JavaMail.root@erie.cs.uoguelph.ca> References: <1184428460.2076443.1333328057090.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday, April 01, 2012 8:54:17 pm Rick Macklem wrote: > First, a little background w.r.t. an offlist discussion of > PR# 165923. A critical part of the problem reported is the > credentials used to to the Write RPCs in the NFS client's > VOP_PUTPAGES(). Currently the code simply uses the credentials > of the thread that called VOP_PUTPAGES(), which is often incorrect, > especially when the caller is "root" and the NFS server is doing > "root squashing" (ie mapping root->nobody or similar). Can this in theory affect READ RPCs sent by VOP_GETPAGES() as well in the case of root squashing? Or are VOP_GETPAGES() synchronous with respect to faults such that in practice you always have a valid credential in curthread->td_ucred? > Although there is no correct answer w.r.t. what credentials should > be used, there seemed to be rough consensus that the credentials of > the process that did the mmap() call would be best. (If there are > multiple writers, you could use either the 1st or most recent mmap(), > but since all writers had write permissions at open(), I don't think > it matters much which you use, but others can comment on this.) The one problem I see with this approach is suppose someone changes the permissions on the file. Now a user that previously had write access might not have it any longer, but we might be sending RPCs as a different user so they will still work. However, I don't think we can solve that, and presumbly we already have that issue now. Hmm, there might be a variant of this though that is new. Suppose user A and user B both mmap the file. At the time, both users have write access to the file. The file is then chown'd or chmod'd such that only user B has write access to the file, but we are using user A's credential as the cached 'write' credential. Then subsequent writes via mmap() by user B will be lost because they will be sent to the server as user A. Previously you couldn't really get into this case if root squashing was off and all writes were sent as root. Not sure if this is easily solvable either. You could perhaps maintain a list of known-valid write credentials and if a write fails with a permission check, toss it that credential and try the next one in the list. > However, there seems to be some disagreement as to how a patch to > use the mmap() credentials should be done. > > putpages-a.patch - This one captures the credentials during the mmap() > call by using the property that only mmap() calls vnode_pager_alloc() > with "prot != 0" and references them in a new field in vm_object_t. > > putpages-b.patch - This one adds a new VOP_PUTPAGES_NOTIFY() call, which > is done by mmap(). For all file systems except NFS clients, it is a > null op. For the NFS clients, they reference the credentials in a > new field of the NFS specific part of the vnode. > > - I don't have a patch for the third one, but I believe Joel was proposing > something similar to putpages-a.patch, except that the credentials are > passed as an extra argument to VOP_PUTPAGES(), instead of the NFS client's > VOP_PUTPAGES() looking in the vnode's vm_object_t for the cred. I think what I would prefer for the longterm is option 3) (and possibly doing the same for VOP_GETPAGES(). Also, I would call the new field in vm_object something like 'writecred'. For MFC purposes you could use putpages-a.patch instead and only make the change to the VOPs in HEAD. However, I wonder if you can't manage this all at the vnode layer? If all you need is a credential that is allowed to write, can't you cache the credential used for a successful VOP_ACCESS() check with write support in the nfsnode and just use that for write RPCs? The same thing would work for reads, yes? I know for NFSv4 you already maintain a cache of credentials from open()'s that you use for other RPC's already, so this would seem to be similar to that case. This would also let you do the "list of credentials" idea if needed and even let you add new write-capable credentials to the list via VOP_ACCESS() calls after the file was mmap()'d or open()'d. > I've attached the first 2, in case anyone wants to look at them. > > Here's my take on the advantages/disadvantages of each patch: > Advantages Disadvantages > patch-a simple, doesn't require VOP > changes and, therefore, can > be MFC'd easily saves a largely NFS specific item > in vm_object_t > depends on the fact that only > mmap() calls vnode_pager_alloc() with > prot != 0 Only one more note here, this problem is not necessarily unique to NFS. It is probably just as relevant for any other networked file systems such as smbfs. Or just about any other filesystem with storage that requires authorization. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204020827.46722.jhb>