On 7/1/22 04:34, Amir Goldstein wrote:
@@ -4685,11 +4686,22 @@ int vfs_rename(struct renamedata *rd) take_dentry_name_snapshot(&old_name, old_dentry); dget(new_dentry); - if (!is_dir || (flags & RENAME_EXCHANGE)) + if (!is_dir || (flags & (RENAME_EXCHANGE|RENAME_NEWER_MTIME))) lock_two_nondirectories(source, target); else if (target) inode_lock(target); + if ((flags & RENAME_NEWER_MTIME) && target) { + /* deny write access to stabilize mtime comparison below */ + error = inode_deny_write_access2(source, target);This is not needed for non regular file, but I guess it doesn't hurt... You could do a helper lock_two_inodes_deny_write() that takes care of both inode_lock() and inode_deny_write_access() and call it instead of lock_two_nondirectories() above. Then the lock and unlock routines would be more straightforward and less error prone, e.g.: - if (!is_dir || (flags & RENAME_EXCHANGE)) + if (flags & RENAME_NEWER_MTIME) + lock_two_inodes_deny_write(source, target); + else if (!is_dir || (flags & (RENAME_EXCHANGE))) lock_two_nondirectories(source, target); ... out: - if (!is_dir || (flags & RENAME_EXCHANGE)) + if (flags & RENAME_NEWER_MTIME) + unlock_two_inodes_allow_write(source, target); + else if (!is_dir || (flags & (RENAME_EXCHANGE))) unlock_two_nondirectories(source, target);
So keep in mind that RENAME_NEWER_MTIME can be used together with RENAME_EXCHANGE, and I'm a bit concerned about having RENAME_NEWER_MTIME usurp the locking logic of RENAME_EXCHANGE when both are used. My thinking in the v2 patch was to keep the locking for each mostly separate and nested, to make it clear that they are independent options that can also be used together. Also because lock_two_inodes_deny_write() can fail, it adds a new possible bailout point that would need to unwind take_dentry_name_snapshot() and dget(). So it's hard to avoid having to add a new "out1" label.
OTOH, for directory, inode_lock is needed to stabilize mtime and lock_two_nondirectories() doesn't do that for you and it is also non trivial to get the locking order and lockdep annotations correct. Since you don't have a use case for RENAME_NEWER_MTIME and directories (?), maybe the easier way around this would be to deny that earlier in do_renameat2() with -EISDIR.
Yes, that makes sense. I will add that. James
