* [PATCH 1/2] nilfs2: handle errors that nilfs_prepare_chunk() may return
2025-01-11 14:26 ` [PATCH 0/2] nilfs2: fix issues with rename operations Ryusuke Konishi
@ 2025-01-11 14:26 ` Ryusuke Konishi
2025-01-11 14:26 ` [PATCH 2/2] nilfs2: do not update mtime of renamed directory that is not moved Ryusuke Konishi
1 sibling, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2025-01-11 14:26 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-nilfs, syzbot+1097e95f134f37d9395c,
syzbot+32c3706ebf5d95046ea1, syzkaller-bugs, LKML
The directory manipulation routines nilfs_set_link() and
nilfs_delete_entry() rewrite the directory entry in the folio/page
previously read by nilfs_find_entry(), so error handling is omitted on
the assumption that nilfs_prepare_chunk(), which prepares the buffer
for rewriting, will always succeed for these. And if an error is
returned, it triggers the legacy BUG_ON() checks in each routine.
This assumption is wrong, as proven by syzbot: the buffer layer called
by nilfs_prepare_chunk() may call nilfs_get_block() if necessary,
which may fail due to metadata corruption or other reasons. This has
been there all along, but improved sanity checks and error handling
may have made it more reproducible in fuzzing tests.
Fix this issue by adding missing error paths in nilfs_set_link(),
nilfs_delete_entry(), and their caller nilfs_rename().
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Reported-by: syzbot+32c3706ebf5d95046ea1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=32c3706ebf5d95046ea1
Reported-by: syzbot+1097e95f134f37d9395c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1097e95f134f37d9395c
Fixes: 2ba466d74ed7 ("nilfs2: directory entry operations")
---
fs/nilfs2/dir.c | 13 ++++++++++---
fs/nilfs2/namei.c | 29 +++++++++++++++--------------
fs/nilfs2/nilfs.h | 4 ++--
3 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index 0a3aea6c416b..9b7f8e9655a2 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -400,7 +400,7 @@ int nilfs_inode_by_name(struct inode *dir, const struct qstr *qstr, ino_t *ino)
return 0;
}
-void nilfs_set_link(struct inode *dir, struct nilfs_dir_entry *de,
+int nilfs_set_link(struct inode *dir, struct nilfs_dir_entry *de,
struct folio *folio, struct inode *inode)
{
size_t from = offset_in_folio(folio, de);
@@ -410,11 +410,15 @@ void nilfs_set_link(struct inode *dir, struct nilfs_dir_entry *de,
folio_lock(folio);
err = nilfs_prepare_chunk(folio, from, to);
- BUG_ON(err);
+ if (unlikely(err)) {
+ folio_unlock(folio);
+ return err;
+ }
de->inode = cpu_to_le64(inode->i_ino);
de->file_type = fs_umode_to_ftype(inode->i_mode);
nilfs_commit_chunk(folio, mapping, from, to);
inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
+ return 0;
}
/*
@@ -543,7 +547,10 @@ int nilfs_delete_entry(struct nilfs_dir_entry *dir, struct folio *folio)
from = (char *)pde - kaddr;
folio_lock(folio);
err = nilfs_prepare_chunk(folio, from, to);
- BUG_ON(err);
+ if (unlikely(err)) {
+ folio_unlock(folio);
+ goto out;
+ }
if (pde)
pde->rec_len = nilfs_rec_len_to_disk(to - from);
dir->inode = 0;
diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
index 1d836a5540f3..e02fae6757f1 100644
--- a/fs/nilfs2/namei.c
+++ b/fs/nilfs2/namei.c
@@ -406,8 +406,10 @@ static int nilfs_rename(struct mnt_idmap *idmap,
err = PTR_ERR(new_de);
goto out_dir;
}
- nilfs_set_link(new_dir, new_de, new_folio, old_inode);
+ err = nilfs_set_link(new_dir, new_de, new_folio, old_inode);
folio_release_kmap(new_folio, new_de);
+ if (unlikely(err))
+ goto out_dir;
nilfs_mark_inode_dirty(new_dir);
inode_set_ctime_current(new_inode);
if (dir_de)
@@ -430,28 +432,27 @@ static int nilfs_rename(struct mnt_idmap *idmap,
*/
inode_set_ctime_current(old_inode);
- nilfs_delete_entry(old_de, old_folio);
-
- if (dir_de) {
- nilfs_set_link(old_inode, dir_de, dir_folio, new_dir);
- folio_release_kmap(dir_folio, dir_de);
- drop_nlink(old_dir);
+ err = nilfs_delete_entry(old_de, old_folio);
+ if (likely(!err)) {
+ if (dir_de) {
+ err = nilfs_set_link(old_inode, dir_de, dir_folio,
+ new_dir);
+ drop_nlink(old_dir);
+ }
+ nilfs_mark_inode_dirty(old_dir);
}
- folio_release_kmap(old_folio, old_de);
-
- nilfs_mark_inode_dirty(old_dir);
nilfs_mark_inode_dirty(old_inode);
- err = nilfs_transaction_commit(old_dir->i_sb);
- return err;
-
out_dir:
if (dir_de)
folio_release_kmap(dir_folio, dir_de);
out_old:
folio_release_kmap(old_folio, old_de);
out:
- nilfs_transaction_abort(old_dir->i_sb);
+ if (likely(!err))
+ err = nilfs_transaction_commit(old_dir->i_sb);
+ else
+ nilfs_transaction_abort(old_dir->i_sb);
return err;
}
diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
index dff241c53fc5..cb6ed54accd7 100644
--- a/fs/nilfs2/nilfs.h
+++ b/fs/nilfs2/nilfs.h
@@ -261,8 +261,8 @@ struct nilfs_dir_entry *nilfs_find_entry(struct inode *, const struct qstr *,
int nilfs_delete_entry(struct nilfs_dir_entry *, struct folio *);
int nilfs_empty_dir(struct inode *);
struct nilfs_dir_entry *nilfs_dotdot(struct inode *, struct folio **);
-void nilfs_set_link(struct inode *, struct nilfs_dir_entry *,
- struct folio *, struct inode *);
+int nilfs_set_link(struct inode *dir, struct nilfs_dir_entry *de,
+ struct folio *folio, struct inode *inode);
/* file.c */
extern int nilfs_sync_file(struct file *, loff_t, loff_t, int);
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] nilfs2: do not update mtime of renamed directory that is not moved
2025-01-11 14:26 ` [PATCH 0/2] nilfs2: fix issues with rename operations Ryusuke Konishi
2025-01-11 14:26 ` [PATCH 1/2] nilfs2: handle errors that nilfs_prepare_chunk() may return Ryusuke Konishi
@ 2025-01-11 14:26 ` Ryusuke Konishi
1 sibling, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2025-01-11 14:26 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-nilfs, syzbot+1097e95f134f37d9395c,
syzbot+32c3706ebf5d95046ea1, syzkaller-bugs, LKML
A minor issue with nilfs_rename, originating from an old ext2
implementation, is that the mtime is updated even if the rename target
is a directory and it is renamed within the same directory, rather
than moved to a different directory.
In this case, the child directory being renamed does not change in any
way, so changing its mtime is unnecessary according to the
specification, and can unnecessarily confuse backup tools.
In ext2, this issue was fixed by commit 39fe7557b4d6 ("ext2: Do not
update mtime of a moved directory") and a few subsequent fixes, but it
remained in nilfs2.
Fix this issue by not calling nilfs_set_link(), which rewrites the
inode number of the directory entry that refers to the parent
directory, when the move target is a directory and the source and
destination are the same directory.
Here, the directory to be moved only needs to be read if the inode
number of the parent directory is rewritten with nilfs_set_link, so
also adjust the execution conditions of the preparation work to avoid
unnecessary directory reads.
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
---
fs/nilfs2/namei.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
index e02fae6757f1..953fbd5f0851 100644
--- a/fs/nilfs2/namei.c
+++ b/fs/nilfs2/namei.c
@@ -370,6 +370,7 @@ static int nilfs_rename(struct mnt_idmap *idmap,
struct folio *old_folio;
struct nilfs_dir_entry *old_de;
struct nilfs_transaction_info ti;
+ bool old_is_dir = S_ISDIR(old_inode->i_mode);
int err;
if (flags & ~RENAME_NOREPLACE)
@@ -385,7 +386,7 @@ static int nilfs_rename(struct mnt_idmap *idmap,
goto out;
}
- if (S_ISDIR(old_inode->i_mode)) {
+ if (old_is_dir && old_dir != new_dir) {
err = -EIO;
dir_de = nilfs_dotdot(old_inode, &dir_folio);
if (!dir_de)
@@ -397,7 +398,7 @@ static int nilfs_rename(struct mnt_idmap *idmap,
struct nilfs_dir_entry *new_de;
err = -ENOTEMPTY;
- if (dir_de && !nilfs_empty_dir(new_inode))
+ if (old_is_dir && !nilfs_empty_dir(new_inode))
goto out_dir;
new_de = nilfs_find_entry(new_dir, &new_dentry->d_name,
@@ -412,7 +413,7 @@ static int nilfs_rename(struct mnt_idmap *idmap,
goto out_dir;
nilfs_mark_inode_dirty(new_dir);
inode_set_ctime_current(new_inode);
- if (dir_de)
+ if (old_is_dir)
drop_nlink(new_inode);
drop_nlink(new_inode);
nilfs_mark_inode_dirty(new_inode);
@@ -420,7 +421,7 @@ static int nilfs_rename(struct mnt_idmap *idmap,
err = nilfs_add_link(new_dentry, old_inode);
if (err)
goto out_dir;
- if (dir_de) {
+ if (old_is_dir) {
inc_nlink(new_dir);
nilfs_mark_inode_dirty(new_dir);
}
@@ -434,9 +435,10 @@ static int nilfs_rename(struct mnt_idmap *idmap,
err = nilfs_delete_entry(old_de, old_folio);
if (likely(!err)) {
- if (dir_de) {
- err = nilfs_set_link(old_inode, dir_de, dir_folio,
- new_dir);
+ if (old_is_dir) {
+ if (old_dir != new_dir)
+ err = nilfs_set_link(old_inode, dir_de,
+ dir_folio, new_dir);
drop_nlink(old_dir);
}
nilfs_mark_inode_dirty(old_dir);
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread