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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 14:15:02 +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

Hello OGAWA,

Thanks a lot for your feedback and comments.

On 5/31/22 11:51, OGAWA Hirofumi wrote:
> Javier Martinez Canillas <javierm@...hat.com> writes:
> 
>> The renameat2 RENAME_EXCHANGE flag allows to atomically exchange two paths
>> but is currently not supported by the Linux vfat filesystem driver.
>>
>> Add a vfat_rename_exchange() helper function that implements this support.
>>
>> The super block lock is acquired during the operation to ensure atomicity,
>> and in the error path actions made are reversed also with the mutex held.
>>
>> It makes the operation as transactional as possible, within the limitation
>> impossed by vfat due not having a journal with logs to replay.
> 
> I tweaked your patch (tested only slightly), can you review and merge to
> this patch if ok?
>

Your changes make a lot of sense to me and from a quick glance they look OK
to me. I'll look at them in more detail when merging the changes for a v4.

> 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.

>> +	if (old_dir != new_dir) {
>> +		err = vfat_get_dotdot_info(old_inode, &old_dotdot_bh, &old_dotdot_de);
>> +		if (err)
>> +			goto out;
>> +
>> +		err = vfat_get_dotdot_info(new_inode, &new_dotdot_bh, &new_dotdot_de);
>> +		if (err)
>> +			goto out;
> 
> This should not return -ENOENT here. I tweaked to return -EIO in my patch.
>

Got it. I'm OK with that.

>> +	/* 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().

>> +error_new_dotdot:
>> +	/* data cluster is shared, serious corruption */
> 
> Sharing data cluster would happen here only if one inode success to sync
> and another one failed. Then so I/O error, we would not be able to do
> much for it.
>

I see. Thanks for the clarification.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat