Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Thu, 2011-05-05 at 13:58 -0500, Tyler Hicks wrote:
> On Thu May 05, 2011 at 12:36:53PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > On Thu, 2011-05-05 at 10:57 -0500, Tyler Hicks wrote:
> > > On Thu May 05, 2011 at 10:35:41AM -0500, Tyler Hicks <tyhicks@linux.vnet.ibm.com> wrote:
> > > > To add support for eCryptfs mounts on top of NFS client mounts, the NFS
> > > > client must properly handle NULL nameidata pointers in its d_revalidate
> > > > functions.
> > > > 
> > > > NFS clients should also handle NULL nameidata in its create functions,
> > > > although this is not currently required for eCryptfs support.
> > > > 
> > > > Signed-off-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> > > > ---
> > > 
> > > We (eCryptfs) are in the process of switching mailing lists, so I copied
> > > both the old (launchpad.net) and the new (vger.kernel.org), but it
> > > doesn't look like the vger.kernel.org list is accepting mail yet. Sorry
> > > about that, I should have tested it first. Feel free to drop it from
> > > any replies.
> > > 
> > > I should also mention that if/when this patch is merged, eCryptfs will
> > > have basic support of mounting on top of NFSv3 client mounts. I say
> > > basic because I'm sure there are some bugs, I'm not yet confident that
> > > the required cache flushes are there in the eCryptfs layer to have NFSv3
> > > cache consistency, and we have some trouble with silly rename.
> > > 
> > > All files unlinked through eCryptfs get silly renamed in the NFS client
> > > because of the extra reference eCryptfs holds on the NFS dentry.
> > > 
> > > This also seems to come into play when unlinking the last file in a
> > > directory and then immediately removing the directory. nfs_rmdir() will
> > > sometimes return -EBUSY.
> > > 
> > > BTW, I think these are all issues that should be handled in the eCryptfs
> > > layer, but I wanted to provide an update on the status of eCryptfs on
> > > top of NFS.
> > 
> > Why would we want to 'support' ecryptfs in this manner? Can't you set up
> > a proper nameidata with appropriate open intents?
> 
> Sorry, that was bad wording in my commit message. NFS would not be
> "supporting" eCryptfs. eCryptfs supports the filesystem that it is
> mounted on top of. We keep any quirks from the stacking scheme in
> fs/ecryptfs/ and don't expect filesystems to do anything special for
> eCryptfs.
> 
> In this case, I believe it is a bug for NFS to oops on NULL nameidata in
> d_revalidate. It was introduced by 34286d66 and 657e94b6 and has been
> fixed in most other filesystems (9177ada9, 8ce84eeb, 4714e637, 53fe9241,
> 529c5f95, 0eb980e3).
> 
> I saw two potential oopses in the NFS create functions and patched
> those, too. However, in the case of create, eCryptfs does pass a proper
> nameidata to vfs_create().

Does it do so to d_revalidate() when opening a file?

> > 
> > This patch might allow you to look up files on NFS, but without open
> > intents, you certainly won't be able to open them, nor will you be able
> > to create them (as you seem to believe).
> 
> I'm not sure why open and create would not work. My testing shows that
> with this patch applied, I can compile the kernel in an eCryptfs mount
> mounted on top of an NFSv3 client, run fsx-linux, etc.

Please try testing with NFSv4, which requires intents in both lookup()
and d_revalidate() when opening a file.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com