Date: Mon, 2 Apr 2012 11:45:10 -0400 (EDT) From: Rick Macklem <rmacklem@uoguelph.ca> To: John Baldwin <jhb@freebsd.org> Cc: alc@freebsd.org, freebsd-fs@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: <1615948459.2113806.1333381510230.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201204020827.46722.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
John Baldwin wrote: > 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. > Yes, the cases where chmod, chown or setuid on the process that mmap'd the file aren't handled and I don't think they can be. See below. > 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. > There is a question w.r.t. how far you take this. You could have a list of all credentials that mmap'd the file and try them in turn. (Personally, I think that's ovekill for a rare case, but could be done. See below w.r.t. the more general case of write credentials.) > > 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 think there is a question w.r.t. should you use credentials that weren't ones that mmap'd the file with PROT_WRITE. For example, the extreme case of this would be to just VOP_GETATTR() the file and use credentials with uid == va_uid to do the writes, if there are EACCES failures. This assumes that the client "should always be able to write the file, if it was mmap'd". This trick hasn't been used for non-mmap'd writes, presumably because it was felt that the per-wrire RPC access check was a good thing that the client shouldn't be circumventing. Btw, Joel had a rather long list of alternative fixes. Unfortunately this was done offlist (well actually mostly on re@). Maybe he could re-post those or give me permission to do so? > > 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. > Joel argued that the credential reference in vm_object_t wasn't NFS specific for the same reason. (ie. He felt it might be useful for other filesystems, such as smbfs. He had at least one other one, but I can't remember which one;-) Thanks for the comments, rick > -- > John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1615948459.2113806.1333381510230.JavaMail.root>