nfs_unlink() again, and trivial nfs_fhget

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
@@ -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
@@ -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;

> 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

Simple place to fix it.

No, there isn't.

I did make an argument, and did send you a patch. Please see my email with message id <[email protected]>.

/fc

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

> 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

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

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

> 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