[PATCH] nfs_unlink() again, and trivial nfs_fhget

[PATCH] nfs_unlink() again, and trivial nfs_fhget

[Frank Cusack]

The first one didn't make it into 2.5.71/72, but is necessary. :-)
The second one removes a redundant assignment.

/fc

diff -urp linux-2.5.72/fs/namei.c linux-2.5.72-silly/fs/namei.c
--- linux-2.5.72/fs/namei.c	Mon Jun 16 21:19:57 2003
+++ linux-2.5.72-silly/fs/namei.c	Tue Jun 17 05:09:04 2003
@@ -985,6 +985,8 @@ static inline int check_sticky(struct in
  *  7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
  *  8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
  *  9. We can't remove a root or mountpoint.
+ * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
+ *     nfs_async_unlink().
  */
 static inline int may_delete(struct inode *dir,struct dentry *victim, int isdir)
 {
@@ -1008,6 +1010,8 @@ static inline int may_delete(struct inod
 		return -EISDIR;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
+	if (victim->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
 	return 0;
 }
 
diff -urp linux-2.5.72/fs/nfs/inode.c linux-2.5.72-silly/fs/nfs/inode.c
--- linux-2.5.72/fs/nfs/inode.c	Mon Jun 16 21:20:20 2003
+++ linux-2.5.72-silly/fs/nfs/inode.c	Tue Jun 17 05:03:19 2003
@@ -715,7 +715,6 @@ __nfs_fhget(struct super_block *sb, stru
 		if (fattr->valid & NFS_ATTR_FATTR_V4)
 			nfsi->change_attr = fattr->change_attr;
 		inode->i_size = nfs_size_to_loff_t(fattr->size);
-		inode->i_mode = fattr->mode;
 		inode->i_nlink = fattr->nlink;
 		inode->i_uid = fattr->uid;
 		inode->i_gid = fattr->gid;

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Trond Myklebust]

>>>>> " " == Frank Cusack <fcusack@fcusack.com> writes:

     > The first one didn't make it into 2.5.71/72, but is
     > necessary. :-)

Why are you doing this in the VFS?

There is already code to handle this case in the NFS filesystem code
itself. If you think that code is wrong then make an argument for
changing it, and send me a patch.

Cheers,
  Trond

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Frank Cusack]

On Tue, Jun 17, 2003 at 11:41:18AM -0700, Trond Myklebust wrote:
> >>>>> " " == Frank Cusack <fcusack@fcusack.com> writes:
> 
>      > The first one didn't make it into 2.5.71/72, but is
>      > necessary. :-)
> 
> Why are you doing this in the VFS?

Simple place to fix it.

> There is already code to handle this case in the NFS filesystem code
> itself.

No, there isn't.

> If you think that code is wrong then make an argument for
> changing it, and send me a patch.

I did make an argument, and did send you a patch.  Please see my email
with message id <20030611002226.A19078@google.com>.

/fc

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Frank Cusack]

On Tue, Jun 17, 2003 at 04:55:07PM -0700, Frank Cusack wrote:
> On Tue, Jun 17, 2003 at 11:41:18AM -0700, Trond Myklebust wrote:
> > If you think that code is wrong then make an argument for
> > changing it, and send me a patch.
> 
> I did make an argument, and did send you a patch.  Please see my email
> with message id <20030611002226.A19078@google.com>.

Let me quickly amend that, the referenced message is also a VFS change,
but a worse one than proposed in this thread.  The reason I proposed to
do it in the VFS is that I couldn't get it to work in the NFS code.

After returning from nfs_rename(), I'd promptly get a null pointer deref.

Yes, I did want to suggest an NFS patch, but it became clear that after
the VFS calls ->rename, it expects things to happen which I didn't do.
More specifically, I avoided the 'if (!d_unhashed()) d_drop()' code, and
rather than figure out the requirements I simply figured I'd propose a
VFS change.  Simple just seemed good to me ... I don't think it's so
bad for the VFS to have *small* bits of fs-specific knowledge.  You could
call the flag (say) DCACHE_DONT_UNLINK if it makes it sound less specific.

/fc

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Trond Myklebust]

>>>>> " " == Frank Cusack <fcusack@fcusack.com> writes:

    >> There is already code to handle this case in the NFS filesystem
    >> code itself.

     > No, there isn't.

Yes there is. Look again at nfs_unlink(), following the path

  if (atomic_read(&dentry->d_count) > 1)

which is *always* the case if we have sillyrenamed the file. Then look
at the comment "We don't allow a dentry to be silly-renamed twice."

Cheers,
  Trond

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Frank Cusack]

On Wed, Jun 18, 2003 at 04:40:47PM -0700, Trond Myklebust wrote:
> >>>>> " " == Frank Cusack <fcusack@fcusack.com> writes:
> 
>     >> There is already code to handle this case in the NFS filesystem
>     >> code itself.
> 
>      > No, there isn't.
> 
> Yes there is. Look again at nfs_unlink(), following the path

My orig. message wasn't clear because I've gone through this so many
times that I didn't bother to write a complete essay this time.
In fact, it's less than "wasn't clear"; it was UNCLEAR.

My patch does appear to simply prevent unlink, and yes, that case is
ALREADY taken care of.  Sorry for the confusion there.  My patch isn't
for that purpose.

It's to prevent RENAME of silly-renamed files.  Doing so in VFS is a
one-liner, and I agree that the VFS should be as clean as possible,
but let's face it, the VFS *must* have specific fs knowledge.  eg,
the ALWAYS_REVAL (something like that) patch you recently submitted
is just to treat NFS differently than other fs's.  Just because the
flag doesn't have "NFS" in it doesn't make it generic.  (And so I
repeat my earlier suggestion that might make the change more palatable:
rename the NFSFS_RENAMED flag to DONT_UNLINK.)

Please review the thread which ended with the message I previously cited.
One of the things there is a test case showing the problem.

I could write another essay on why the one-liner I proposed is a decent
patch, but I'd rather not. :-)  If  the patch is accepted, there is a
some simplification that could be done in the NFS code; I will submit a
subsequent patch for that if this first one is accepted.

/fc

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Frank Cusack]

On Sat, Jun 21, 2003 at 06:46:23PM -0700, Frank Cusack wrote:
> It's to prevent RENAME of silly-renamed files.  Doing so in VFS is a
> one-liner, and I agree that the VFS should be as clean as possible,
> but let's face it, the VFS *must* have specific fs knowledge.  eg,

I hate to keep following up myself, but I forgot one point I had in
mind: other code in may_delete() is already fs-specific.

IS_APPEND
IS_IMMUTABLE
check_sticky

These things aren't generic, they require specific support from the fs.
/fc

Re: [PATCH] nfs_unlink() again, and trivial nfs_fhget

[Trond Myklebust]

>>>>> " " == Frank Cusack <fcusack@fcusack.com> writes:

     > It's to prevent RENAME of silly-renamed files.  Doing so in VFS
     > is a one-liner, and I agree that the VFS should be as clean as
     > possible, but let's face it, the VFS *must* have specific fs
     > knowledge.  eg, the ALWAYS_REVAL (something like that) patch
     > you recently submitted is just to treat NFS differently than
     > other fs's.  Just because the flag doesn't have "NFS" in it
     > doesn't make it generic.  (And so I repeat my earlier
     > suggestion that might make the change more palatable: rename
     > the NFSFS_RENAMED flag to DONT_UNLINK.)

On the contrary: The VFS should *avoid* specific fs knowledge
whereever possible.

In this case:

  - we *can* do this test in nfs_rename() itself without needing any
    extra VFS support.
  - No other filesystems have expressed a need for such a flag, so
    we're hardly covering a common need.

Those are 2 good reasons for doing the modification in the NFS code.

And no - NFSFS_RENAMED, and DONT_UNLINK are *not* the same concept.
Renaming sillyrenamed files would be quite acceptable as long as you
don't allow *cross-directory* renames. All other cases are fixable...

     > I hate to keep following up myself, but I forgot one point I
     > had in mind: other code in may_delete() is already fs-specific.
     >
     > IS_APPEND
     > IS_IMMUTABLE
     > check_sticky
     >
     > These things aren't generic, they require specific support from
     > the fs.

For starters, check_sticky has no business being in may_delete(), as
it breaks NFS (i.e. it should really be part of vfs_permission()).
All these comparisons of our local uid/gids or capabilities with
remote object uid/gids are broken.

That said, there is a huge leap between the 2 assertions that "some
special cases appear in the VFS." to "all special case must be done in
the VFS".

Cheers,
  Trond