linux-kernel - Re: [PATCH v3 2/3] fat: add renameat2 RENAME

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 21:41:45 +0900
From:   OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Christian Kellner <ckellner@...hat.com>,
        Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Alexander Larsson <alexl@...hat.com>,
        Alberto Ruiz <aruiz@...hat.com>,
        Peter Jones <pjones@...hat.com>,
        Lennart Poettering <lennart@...ttering.net>,
        Colin Walters <walters@...bum.org>,
        Chung-Chiang Cheng <cccheng@...ology.com>
Subject: Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support

Javier Martinez Canillas <javierm@...hat.com> writes:

>> Main purpose of me is to consolidate helpers with vfat_rename(), and
>> tweak coding style to use existent fat codes.
>>
>
> Indeed. What do you think of the following plan for v4 ?
>
> 1) Keep patch "fat: add a vfat_rename2() and make existing .rename callback a helper"
>    as the first patch of the series.
> 2) Add a patch #2 with your authorship that adds the helper and use them in the
>    vfat_rename() function.
> 3) Make this patch "fat: add renameat2 RENAME_EXCHANGE flag support" to be patch #3
>    and use the helpers introduced in patch #2.
> 4) Make patch #4 to not only add a test for RENAME_EXCHANGE but also for renameat()
>    and renameat2(..., RENAME_NOREPLACE). That way it will also cover your changes in
>    patch #2.

I don't care much about it because whole is not big (in short, I'm ok
with even one patch), so the point is the patches should be able to
bisect easily if separated.

>>> +	/* update inode version and timestamps */
>>> +	inode_inc_iversion(old_inode);
>>> +	inode_inc_iversion(new_inode);
>> 
>> Why do we need to update iversion of those inodes? I couldn't get intent
>> of this.
>>
>
> To be honest, I wasn't sure about this either but I saw that the implementation
> of RENAME_EXCHANGE in other filesystems did. For example btrfs_rename_exchange().

Ok. If I'm not overlooking, it looks like only btrfs. Please remove
those inode_inc_iversion() for {new,old}_inode.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>