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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <cbd99278-89ff-0742-a4cb-99723c397e6c@redhat.com>
Date:   Tue, 31 May 2022 14:46:05 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
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

On 5/31/22 14:41, OGAWA Hirofumi wrote:
> 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.
>

Yes, git bisect-ability is why I mentioned that we could do it in separate patches
but I'll integrate your changes now and see what approach I take depending on how
the code looks then.
 
>>>> +	/* 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.
>

Sure.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat