Date: Tue, 3 Apr 2012 08:53:34 -0400 From: John Baldwin <jhb@freebsd.org> To: Rick Macklem <rmacklem@uoguelph.ca> 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: <201204030853.34267.jhb@freebsd.org> In-Reply-To: <394500062.2166073.1333418575016.JavaMail.root@erie.cs.uoguelph.ca> References: <394500062.2166073.1333418575016.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, April 02, 2012 10:02:55 pm Rick Macklem wrote: > John Baldwin wrote: > > On Monday, April 02, 2012 11:45:10 am Rick Macklem wrote: > > > > > 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. > > > > But you already are potentially (in that you might mismatch > > permissions > > for a given WRITE since the page might have been dirtied by a > > different > > user than the credential we use). You have to open() the file with > > write > > permissions before you mmap() it, so all mmap()'s are going to do a > > VOP_ACCESS() prior, so if you just cache whatever credential last > > worked > > for write access to VOP_ACCESS() that would more or less work, and be > > about as reliable while not requiring any changes outside of the NFS > > client itself. > > > Taking a reference to the cred passed to the most recent VOP_OPEN() with > FWRITE for the vnode and keeping that in the nfs specific vnode sounds ok > to me. Ok. > The only issue is that the NFS client will do this for many vnodes that > never get mmap'd, but that just means more credentials may be allocated > for longer. (As I understand it, the crfree() can't be done until the > NFS VOP_INACTIVE()/VOP_RECLAIM(), since writing to mmap'd files can happen > after the file descriptor is closed.) In practice the credential is already referenced by the open file handle most of the time anyway, plus many processes and files may all share the same credential, so I think this would be fine in practice. > This would avoid any new/modified VOPs and any changes to vm_object_t. Yes. If others are ok with this approach I think it is the simplest fix. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204030853.34267.jhb>