* [PATCH] NFS: Allow NULL nameidata in d_revalidate and create @ 2011-05-05 15:35 Tyler Hicks 2011-05-05 15:57 ` Tyler Hicks 0 siblings, 1 reply; 6+ messages in thread From: Tyler Hicks @ 2011-05-05 15:35 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, ecryptfs-devel, ecryptfs 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> --- fs/nfs/dir.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 7237672..03c6ab2 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1095,7 +1095,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd) struct nfs_fattr *fattr = NULL; int error; - if (nd->flags & LOOKUP_RCU) + if (nd && nd->flags & LOOKUP_RCU) return -ECHILD; parent = dget_parent(dentry); @@ -1493,7 +1493,7 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd) struct nfs_open_context *ctx; int openflags, ret = 0; - if (nd->flags & LOOKUP_RCU) + if (nd && nd->flags & LOOKUP_RCU) return -ECHILD; inode = dentry->d_inode; @@ -1583,7 +1583,7 @@ static int nfs_open_create(struct inode *dir, struct dentry *dentry, int mode, attr.ia_mode = mode; attr.ia_valid = ATTR_MODE; - if ((nd->flags & LOOKUP_CREATE) != 0) { + if (nd && (nd->flags & LOOKUP_CREATE) != 0) { open_flags = nd->intent.open.flags; ctx = nameidata_to_nfs_open_context(dentry, nd); @@ -1673,7 +1673,7 @@ static int nfs_create(struct inode *dir, struct dentry *dentry, int mode, attr.ia_mode = mode; attr.ia_valid = ATTR_MODE; - if ((nd->flags & LOOKUP_CREATE) != 0) + if (nd && (nd->flags & LOOKUP_CREATE) != 0) open_flags = nd->intent.open.flags; error = NFS_PROTO(dir)->create(dir, dentry, &attr, open_flags, NULL); -- 1.7.4.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create 2011-05-05 15:35 [PATCH] NFS: Allow NULL nameidata in d_revalidate and create Tyler Hicks @ 2011-05-05 15:57 ` Tyler Hicks 2011-05-05 16:36 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Tyler Hicks @ 2011-05-05 15:57 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, ecryptfs-devel 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. Tyler > fs/nfs/dir.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index 7237672..03c6ab2 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1095,7 +1095,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd) > struct nfs_fattr *fattr = NULL; > int error; > > - if (nd->flags & LOOKUP_RCU) > + if (nd && nd->flags & LOOKUP_RCU) > return -ECHILD; > > parent = dget_parent(dentry); > @@ -1493,7 +1493,7 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd) > struct nfs_open_context *ctx; > int openflags, ret = 0; > > - if (nd->flags & LOOKUP_RCU) > + if (nd && nd->flags & LOOKUP_RCU) > return -ECHILD; > > inode = dentry->d_inode; > @@ -1583,7 +1583,7 @@ static int nfs_open_create(struct inode *dir, struct dentry *dentry, int mode, > attr.ia_mode = mode; > attr.ia_valid = ATTR_MODE; > > - if ((nd->flags & LOOKUP_CREATE) != 0) { > + if (nd && (nd->flags & LOOKUP_CREATE) != 0) { > open_flags = nd->intent.open.flags; > > ctx = nameidata_to_nfs_open_context(dentry, nd); > @@ -1673,7 +1673,7 @@ static int nfs_create(struct inode *dir, struct dentry *dentry, int mode, > attr.ia_mode = mode; > attr.ia_valid = ATTR_MODE; > > - if ((nd->flags & LOOKUP_CREATE) != 0) > + if (nd && (nd->flags & LOOKUP_CREATE) != 0) > open_flags = nd->intent.open.flags; > > error = NFS_PROTO(dir)->create(dir, dentry, &attr, open_flags, NULL); > -- > 1.7.4.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create 2011-05-05 15:57 ` Tyler Hicks @ 2011-05-05 16:36 ` Trond Myklebust 2011-05-05 18:58 ` Tyler Hicks 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2011-05-05 16:36 UTC (permalink / raw) To: Tyler Hicks; +Cc: linux-nfs, ecryptfs-devel 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? 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). NACKed... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create 2011-05-05 16:36 ` Trond Myklebust @ 2011-05-05 18:58 ` Tyler Hicks 2011-05-05 19:08 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Tyler Hicks @ 2011-05-05 18:58 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, ecryptfs-devel 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(). > > 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. Tyler > > NACKed... > > -- > Trond Myklebust > Linux NFS client maintainer > > NetApp > Trond.Myklebust@netapp.com > www.netapp.com > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create 2011-05-05 18:58 ` Tyler Hicks @ 2011-05-05 19:08 ` Trond Myklebust 2011-05-05 22:02 ` Tyler Hicks 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2011-05-05 19:08 UTC (permalink / raw) To: Tyler Hicks; +Cc: linux-nfs, ecryptfs-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NFS: Allow NULL nameidata in d_revalidate and create 2011-05-05 19:08 ` Trond Myklebust @ 2011-05-05 22:02 ` Tyler Hicks 0 siblings, 0 replies; 6+ messages in thread From: Tyler Hicks @ 2011-05-05 22:02 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, ecryptfs-devel On Thu May 05, 2011 at 03:08:16PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > 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? I don't believe so. > > > > > > > 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. I acknowledge that eCryptfs won't work on top of NFSv4 in its current form. The lookup, create, and open paths in eCryptfs will need some rewriting, as you've mentioned. This patch simply allows eCryptfs to be mounted on top of NFSv3, which is the first step in allowing client-side file encryption. This is an eCryptfs feature that is asked for quite often and I hope you'll reconsider merging the patch until I can make the changes needed to support NFSv4. Tyler > > -- > Trond Myklebust > Linux NFS client maintainer > > NetApp > Trond.Myklebust@netapp.com > www.netapp.com > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-05 22:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-05 15:35 [PATCH] NFS: Allow NULL nameidata in d_revalidate and create Tyler Hicks 2011-05-05 15:57 ` Tyler Hicks 2011-05-05 16:36 ` Trond Myklebust 2011-05-05 18:58 ` Tyler Hicks 2011-05-05 19:08 ` Trond Myklebust 2011-05-05 22:02 ` Tyler Hicks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).