Re: [PATCH v2] namei: implemented RENAME_NEWER_MTIME flag for renameat2() conditional replace

[James Yonan <james@openvpn.net>]

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