| 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
| ||
|
Message-ID: <87ilpmows4.fsf@mail.parknet.co.jp>
Date: Tue, 31 May 2022 18:51:07 +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:
> 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?
Main purpose of me is to consolidate helpers with vfat_rename(), and
tweak coding style to use existent fat codes.
> + 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.
> + /* 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.
> +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.
Thanks.
--
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Signed-off-by: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
---
fs/fat/namei_vfat.c | 205 +++++++++++++++++++++------------------------------
1 file changed, 87 insertions(+), 118 deletions(-)
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 3e3cd4f..84f95eb 100644
--- a/fs/fat/namei_vfat.c 2022-05-31 16:53:56.987009983 +0900
+++ b/fs/fat/namei_vfat.c 2022-05-31 18:37:30.893473188 +0900
@@ -893,16 +893,55 @@ out:
return err;
}
+static int vfat_get_dotdot_de(struct inode *inode, struct buffer_head **bh,
+ struct msdos_dir_entry **de)
+{
+ if (S_ISDIR(inode->i_mode)) {
+ if (fat_get_dotdot_entry(inode, bh, de))
+ return -EIO;
+ }
+ return 0;
+}
+
+static int vfat_sync_ipos(struct inode *dir, struct inode *inode)
+{
+ if (IS_DIRSYNC(dir))
+ return fat_sync_inode(inode);
+ mark_inode_dirty(inode);
+ return 0;
+}
+
+static int vfat_update_dotdot_de(struct inode *dir, struct inode *inode,
+ struct buffer_head *dotdot_bh,
+ struct msdos_dir_entry *dotdot_de)
+{
+ fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart);
+ mark_buffer_dirty_inode(dotdot_bh, inode);
+ if (IS_DIRSYNC(dir))
+ return sync_dirty_buffer(dotdot_bh);
+ return 0;
+}
+
+static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
+{
+ inode_inc_iversion(dir);
+ fat_truncate_time(dir, ts, S_CTIME | S_MTIME);
+ if (IS_DIRSYNC(dir))
+ (void)fat_sync_inode(dir);
+ else
+ mark_inode_dirty(dir);
+}
+
static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry)
{
struct buffer_head *dotdot_bh;
- struct msdos_dir_entry *dotdot_de;
+ struct msdos_dir_entry *dotdot_de = NULL;
struct inode *old_inode, *new_inode;
struct fat_slot_info old_sinfo, sinfo;
struct timespec64 ts;
loff_t new_i_pos;
- int err, is_dir, update_dotdot, corrupt = 0;
+ int err, is_dir, corrupt = 0;
struct super_block *sb = old_dir->i_sb;
old_sinfo.bh = sinfo.bh = dotdot_bh = NULL;
@@ -913,15 +952,13 @@ static int vfat_rename(struct inode *old
if (err)
goto out;
- is_dir = S_ISDIR(old_inode->i_mode);
- update_dotdot = (is_dir && old_dir != new_dir);
- if (update_dotdot) {
- if (fat_get_dotdot_entry(old_inode, &dotdot_bh, &dotdot_de)) {
- err = -EIO;
+ if (old_dir != new_dir) {
+ err = vfat_get_dotdot_de(old_inode, &dotdot_bh, &dotdot_de);
+ if (err)
goto out;
- }
}
+ is_dir = S_ISDIR(old_inode->i_mode);
ts = current_time(old_dir);
if (new_inode) {
if (is_dir) {
@@ -942,21 +979,15 @@ static int vfat_rename(struct inode *old
fat_detach(old_inode);
fat_attach(old_inode, new_i_pos);
- if (IS_DIRSYNC(new_dir)) {
- err = fat_sync_inode(old_inode);
+ err = vfat_sync_ipos(new_dir, old_inode);
+ if (err)
+ goto error_inode;
+
+ if (dotdot_de) {
+ err = vfat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
+ dotdot_de);
if (err)
- goto error_inode;
- } else
- mark_inode_dirty(old_inode);
-
- if (update_dotdot) {
- fat_set_start(dotdot_de, MSDOS_I(new_dir)->i_logstart);
- mark_buffer_dirty_inode(dotdot_bh, old_inode);
- if (IS_DIRSYNC(new_dir)) {
- err = sync_dirty_buffer(dotdot_bh);
- if (err)
- goto error_dotdot;
- }
+ goto error_dotdot;
drop_nlink(old_dir);
if (!new_inode)
inc_nlink(new_dir);
@@ -966,12 +997,7 @@ static int vfat_rename(struct inode *old
old_sinfo.bh = NULL;
if (err)
goto error_dotdot;
- inode_inc_iversion(old_dir);
- fat_truncate_time(old_dir, &ts, S_CTIME|S_MTIME);
- if (IS_DIRSYNC(old_dir))
- (void)fat_sync_inode(old_dir);
- else
- mark_inode_dirty(old_dir);
+ vfat_update_dir_metadata(old_dir, &ts);
if (new_inode) {
drop_nlink(new_inode);
@@ -991,10 +1017,9 @@ error_dotdot:
/* data cluster is shared, serious corruption */
corrupt = 1;
- if (update_dotdot) {
- fat_set_start(dotdot_de, MSDOS_I(old_dir)->i_logstart);
- mark_buffer_dirty_inode(dotdot_bh, old_inode);
- corrupt |= sync_dirty_buffer(dotdot_bh);
+ if (dotdot_de) {
+ corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
+ dotdot_de);
}
error_inode:
fat_detach(old_inode);
@@ -1021,66 +1046,18 @@ error_inode:
goto out;
}
-/* Helpers for vfat_rename_exchange() */
-
-static int vfat_get_dotdot_info(struct inode *inode, struct buffer_head **dotdot_bh,
- struct msdos_dir_entry **dotdot_de)
-{
- if (!S_ISDIR(inode->i_mode))
- return 0;
-
- return fat_get_dotdot_entry(inode, dotdot_bh, dotdot_de);
-}
-
-static void vfat_exchange_dentries(struct inode *old_inode, struct inode *new_inode,
- loff_t old_i_pos, loff_t new_i_pos)
+static void vfat_exchange_ipos(struct inode *old_inode, struct inode *new_inode,
+ loff_t old_i_pos, loff_t new_i_pos)
{
fat_detach(old_inode);
fat_detach(new_inode);
-
fat_attach(old_inode, new_i_pos);
fat_attach(new_inode, old_i_pos);
}
-static int vfat_sync_after_exchange(struct inode *dir, struct inode *inode)
-{
- int err = 0;
-
- if (IS_DIRSYNC(dir))
- err = fat_sync_inode(inode);
- else
- mark_inode_dirty(inode);
-
- return err;
-}
-
-static int vfat_update_dotdot_info(struct buffer_head *dotdot_bh, struct msdos_dir_entry *dotdot_de,
- struct inode *dir, struct inode *inode)
-{
- int err = 0;
-
- fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart);
- mark_buffer_dirty_inode(dotdot_bh, inode);
-
- if (IS_DIRSYNC(dir))
- err = sync_dirty_buffer(dotdot_bh);
-
- return err;
-}
-
-static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts)
-{
- inode_inc_iversion(dir);
- fat_truncate_time(dir, ts, S_CTIME | S_MTIME);
-
- if (IS_DIRSYNC(dir))
- (void)fat_sync_inode(dir);
- else
- mark_inode_dirty(dir);
-}
-
-static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
- struct inode *new_dir, struct dentry *new_dentry)
+static int
+vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
+ struct inode *new_dir, struct dentry *new_dentry)
{
struct buffer_head *old_dotdot_bh = NULL, *new_dotdot_bh = NULL;
struct msdos_dir_entry *old_dotdot_de = NULL, *new_dotdot_de = NULL;
@@ -1098,11 +1075,13 @@ static int vfat_rename_exchange(struct i
/* if directories are not the same, get ".." info to update */
if (old_dir != new_dir) {
- err = vfat_get_dotdot_info(old_inode, &old_dotdot_bh, &old_dotdot_de);
+ err = vfat_get_dotdot_de(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);
+ err = vfat_get_dotdot_de(new_inode, &new_dotdot_bh,
+ &new_dotdot_de);
if (err)
goto out;
}
@@ -1110,45 +1089,41 @@ static int vfat_rename_exchange(struct i
old_i_pos = MSDOS_I(old_inode)->i_pos;
new_i_pos = MSDOS_I(new_inode)->i_pos;
- /* exchange the two dentries */
- vfat_exchange_dentries(old_inode, new_inode, old_i_pos, new_i_pos);
+ vfat_exchange_ipos(old_inode, new_inode, old_i_pos, new_i_pos);
- err = vfat_sync_after_exchange(old_dir, new_inode);
+ err = vfat_sync_ipos(old_dir, new_inode);
if (err)
goto error_exchange;
-
- err = vfat_sync_after_exchange(new_dir, old_inode);
+ err = vfat_sync_ipos(new_dir, old_inode);
if (err)
goto error_exchange;
/* update ".." directory entry info */
if (old_dotdot_de) {
- err = vfat_update_dotdot_info(old_dotdot_bh, old_dotdot_de, new_dir, old_inode);
+ err = vfat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh,
+ old_dotdot_de);
if (err)
goto error_old_dotdot;
-
drop_nlink(old_dir);
inc_nlink(new_dir);
}
if (new_dotdot_de) {
- err = vfat_update_dotdot_info(new_dotdot_bh, new_dotdot_de, old_dir, new_inode);
+ err = vfat_update_dotdot_de(old_dir, new_inode, new_dotdot_bh,
+ new_dotdot_de);
if (err)
goto error_new_dotdot;
-
drop_nlink(new_dir);
inc_nlink(old_dir);
}
- /* update inode version and timestamps */
inode_inc_iversion(old_inode);
inode_inc_iversion(new_inode);
-
vfat_update_dir_metadata(old_dir, &ts);
-
/* if directories are not the same, update new_dir as well */
if (old_dir != new_dir)
vfat_update_dir_metadata(new_dir, &ts);
+
out:
brelse(old_dotdot_bh);
brelse(new_dotdot_bh);
@@ -1157,30 +1132,21 @@ out:
return err;
error_new_dotdot:
- /* data cluster is shared, serious corruption */
- corrupt = 1;
-
if (new_dotdot_de) {
- corrupt |= vfat_update_dotdot_info(new_dotdot_bh, new_dotdot_de,
- new_dir, new_inode);
+ corrupt |= vfat_update_dotdot_de(new_dir, new_inode,
+ new_dotdot_bh, new_dotdot_de);
}
error_old_dotdot:
- /* data cluster is shared, serious corruption */
- corrupt = 1;
-
if (old_dotdot_de) {
- corrupt |= vfat_update_dotdot_info(old_dotdot_bh, old_dotdot_de,
- old_dir, old_inode);
+ corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
+ old_dotdot_bh, old_dotdot_de);
}
error_exchange:
- vfat_exchange_dentries(old_inode, new_inode, new_i_pos, old_i_pos);
-
- if (corrupt) {
- corrupt |= fat_sync_inode(old_inode);
- corrupt |= fat_sync_inode(new_inode);
- }
+ vfat_exchange_ipos(old_inode, new_inode, new_i_pos, old_i_pos);
+ corrupt |= vfat_sync_ipos(new_dir, new_inode);
+ corrupt |= vfat_sync_ipos(old_dir, old_inode);
if (corrupt < 0) {
fat_fs_error(new_dir->i_sb,
@@ -1190,15 +1156,18 @@ error_exchange:
goto out;
}
-static int vfat_rename2(struct user_namespace *mnt_userns, struct inode *old_dir,
+static int vfat_rename2(struct user_namespace *mnt_userns,
+ struct inode *old_dir,
struct dentry *old_dentry, struct inode *new_dir,
struct dentry *new_dentry, unsigned int flags)
{
if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
return -EINVAL;
- if (flags & RENAME_EXCHANGE)
- return vfat_rename_exchange(old_dir, old_dentry, new_dir, new_dentry);
+ if (flags & RENAME_EXCHANGE) {
+ return vfat_rename_exchange(old_dir, old_dentry,
+ new_dir, new_dentry);
+ }
/* VFS already handled RENAME_NOREPLACE, handle it as a normal rename */
return vfat_rename(old_dir, old_dentry, new_dir, new_dentry);
_
Powered by blists - more mailing lists