The reproducer uses a file0 on a ntfs3 file system with a corrupted i_link.
When renaming, the file0's inode is marked as a bad inode because the file
name cannot be deleted. However, before renaming, file0 is a directory.
After the renaming fails, it is marked as a bad inode, which makes it a
regular file. In any case, when opening it after creating a hard link,
pick_link() should not be entered because it is not a symbolic link from
beginning to end.

Add a check on the symbolic link before entering pick_link() to avoid
triggering unknown exceptions when performing the i_link acquisition
operation on other types of files.

Reported-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1aa90f0eb1fc3e77d969
Tested-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/namei.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 4bb889fc980b..1524a5359d46 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2005,6 +2005,10 @@ static const char *step_into(struct nameidata *nd, int flags,
 		if (path.mnt == nd->path.mnt)
 			mntget(path.mnt);
 	}
+
+	if (inode && !S_ISLNK(inode->i_mode))
+		return NULL;
+
 	return pick_link(nd, &path, inode, flags);
 }
 
-- 
2.43.0

On Wed, Jun 18, 2025 at 11:30:48AM +0800, Edward Adam Davis wrote:
> The reproducer uses a file0 on a ntfs3 file system with a corrupted i_link.
> When renaming, the file0's inode is marked as a bad inode because the file
> name cannot be deleted. However, before renaming, file0 is a directory.
> After the renaming fails, it is marked as a bad inode, which makes it a
> regular file. In any case, when opening it after creating a hard link,
> pick_link() should not be entered because it is not a symbolic link from
> beginning to end.
> 
> Add a check on the symbolic link before entering pick_link() to avoid
> triggering unknown exceptions when performing the i_link acquisition
> operation on other types of files.
> 
> Reported-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1aa90f0eb1fc3e77d969
> Tested-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

NAK.  This is not the first time that garbage is suggested and no,
we are not going to paper over that shite in fs/namei.c.

Not going to happen.

You ARE NOT ALLOWED to call make_bad_inode() on a live inode, period.
Never, ever to be done.

There's a lot of assertions it violates and there's no chance in
hell to plaster each with that kind of checks.

Fix NTFS.  End of story.

On Wed, Jun 18, 2025 at 05:50:16AM +0100, Al Viro wrote:

> NAK.  This is not the first time that garbage is suggested and no,
> we are not going to paper over that shite in fs/namei.c.
> 
> Not going to happen.
> 
> You ARE NOT ALLOWED to call make_bad_inode() on a live inode, period.
> Never, ever to be done.
> 
> There's a lot of assertions it violates and there's no chance in
> hell to plaster each with that kind of checks.
> 
> Fix NTFS.  End of story.

To elaborate a bit: if you look at the end of e.g. their attr_set_size(),
you'll see
out:
        if (is_bad) {
bad_inode:
		_ntfs_bad_inode(&ni->vfs_inode);
	}
	return err;
}

This is a bug.  So are similar places all over the place there.
You are not supposed to use make_bad_inode() as a general-purpose
"something went wrong, don't wanna see it anymore" tool.

And as long as it stays there, any fuzzing reports of ntfs are pretty
much worthless - any of those places (easily located by grepping for
_ntfs_bad_inode) can fuck the kernel up.  Once ntfs folks get around
to saner error recovery, it would make sense to start looking into
fuzzing that thing again.  Until then - nope.  Again, this is *NOT*
going to be papered over in a random set of places (pretty certain
to remain incomplete) in VFS.

On Wed, Jun 18, 2025 at 06:02:00AM +0100, Al Viro wrote:
> On Wed, Jun 18, 2025 at 05:50:16AM +0100, Al Viro wrote:
> 
> > NAK.  This is not the first time that garbage is suggested and no,
> > we are not going to paper over that shite in fs/namei.c.
> > 
> > Not going to happen.
> > 
> > You ARE NOT ALLOWED to call make_bad_inode() on a live inode, period.
> > Never, ever to be done.
> > 
> > There's a lot of assertions it violates and there's no chance in
> > hell to plaster each with that kind of checks.
> > 
> > Fix NTFS.  End of story.
> 
> To elaborate a bit: if you look at the end of e.g. their attr_set_size(),
> you'll see
> out:
>         if (is_bad) {
> bad_inode:
> 		_ntfs_bad_inode(&ni->vfs_inode);
> 	}
> 	return err;
> }
> 
> This is a bug.  So are similar places all over the place there.
> You are not supposed to use make_bad_inode() as a general-purpose
> "something went wrong, don't wanna see it anymore" tool.
> 
> And as long as it stays there, any fuzzing reports of ntfs are pretty
> much worthless - any of those places (easily located by grepping for
> _ntfs_bad_inode) can fuck the kernel up.  Once ntfs folks get around
> to saner error recovery, it would make sense to start looking into
> fuzzing that thing again.  Until then - nope.  Again, this is *NOT*
> going to be papered over in a random set of places (pretty certain
> to remain incomplete) in VFS.

Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
(or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
is also FUBAR.

So's anything that calls make_bad_inode() on a struct inode that might be
in process of being passed to one of those functions by another thread.

This is fundamentally wrong; bad inodes are not supposed to end up attached
to dentries.

On Wed, 18 Jun 2025 06:27:47 +0100, Al Viro wrote:
> Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
> (or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
> is also FUBAR.
> 
> So's anything that calls make_bad_inode() on a struct inode that might be
> in process of being passed to one of those functions by another thread.
> 
> This is fundamentally wrong; bad inodes are not supposed to end up attached
> to dentries.
As far as I know, pick_link() is used to resolve the target path of a
symbolic link (symlink). Can you explain why pick_link() is executed on
a directory or a regular file?

On Wed, Jun 18, 2025 at 01:34:18PM +0800, Edward Adam Davis wrote:
> On Wed, 18 Jun 2025 06:27:47 +0100, Al Viro wrote:
> > Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
> > (or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
> > is also FUBAR.
> > 
> > So's anything that calls make_bad_inode() on a struct inode that might be
> > in process of being passed to one of those functions by another thread.
> > 
> > This is fundamentally wrong; bad inodes are not supposed to end up attached
> > to dentries.
> As far as I know, pick_link() is used to resolve the target path of a
> symbolic link (symlink). Can you explain why pick_link() is executed on
> a directory or a regular file?

Because the inode_operations of that thing contains ->get_link().  Which means
"symlink" to dcache.  Again, there is code all over the place written in
assumption that no dentry will ever have ->d_inode pointing to any of those.

No, we are not going to paper over that in __d_add() or __d_instantiate() either;
it's fundamentally a losing game.  _Maybe_ a couple of WARN_ON() when built with
CONFIG_DEBUG_VFS or something similar, but that would only make for slightly
more specific diagnostics; not all that useful, since you can literally grep for
_ntfs_bad_inode to pick the location of actual underlying bugs.

Again, the underlying bug is that make_bad_inode() is called on a live inode.
In some cases it's "icache lookup finds a normal inode, d_splice_alias() is called
to attach it to dentry, while another thread decides to call make_bad_inode() on
it - that would evict it from icache, but we'd already found it there earlier".
In some it's outright "we have an inode attached to dentry - that's how we got
it in the first place; let's call make_bad_inode() on it just for shits and giggles".
Either is a bug.

_ntfs_bad_inode() uses are completely broken.  Matter of fact, we probably ought to
retire make_bad_inode() - there are few callers and most of them don't actually
need anything other than remove_inode_hash() (e.g. iget_failed()).  In any case,
whether there is a case for several new helpers or not, the kind of use
_ntfs_bad_inode() gets is right out.

On Wed, 18 Jun 2025 07:18:15 +0100, Al Viro wrote:
> > On Wed, 18 Jun 2025 06:27:47 +0100, Al Viro wrote:
> > > Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
> > > (or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
> > > is also FUBAR.
> > >
> > > So's anything that calls make_bad_inode() on a struct inode that might be
> > > in process of being passed to one of those functions by another thread.
> > >
> > > This is fundamentally wrong; bad inodes are not supposed to end up attached
> > > to dentries.
> > As far as I know, pick_link() is used to resolve the target path of a
> > symbolic link (symlink). Can you explain why pick_link() is executed on
> > a directory or a regular file?
> 
> Because the inode_operations of that thing contains ->get_link().
I removed _ntfs_bad_inode() and it fixed the problem.
I should have thought more carefully about what you said about the bad inode.
> Again, the underlying bug is that make_bad_inode() is called on a live inode.
> In some cases it's "icache lookup finds a normal inode, d_splice_alias() is called
> to attach it to dentry, while another thread decides to call make_bad_inode() on
> it - that would evict it from icache, but we'd already found it there earlier".
> In some it's outright "we have an inode attached to dentry - that's how we got
> it in the first place; let's call make_bad_inode() on it just for shits and giggles".

BR,
Edward

The reproducer uses a file0 on a ntfs3 file system with a corrupted i_link.
When renaming, the file0's inode is marked as a bad inode because the file
name cannot be deleted.

The underlying bug is that make_bad_inode() is called on a live inode.
In some cases it's "icache lookup finds a normal inode, d_splice_alias()
is called to attach it to dentry, while another thread decides to call
make_bad_inode() on it - that would evict it from icache, but we'd already
found it there earlier".
In some it's outright "we have an inode attached to dentry - that's how we
got it in the first place; let's call make_bad_inode() on it just for shits
and giggles".

Fixes: 78ab59fee07f ("fs/ntfs3: Rework file operations")
Reported-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1aa90f0eb1fc3e77d969
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: fix it by removing set bad inode

 fs/ntfs3/frecord.c |  7 +++----
 fs/ntfs3/namei.c   | 10 +++-------
 fs/ntfs3/ntfs_fs.h |  3 +--
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 756e1306fe6c..7afbb4418eb2 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -3003,8 +3003,7 @@ int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni,
  * ni_rename - Remove one name and insert new name.
  */
 int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni,
-	      struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de,
-	      bool *is_bad)
+	      struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de)
 {
 	int err;
 	struct NTFS_DE *de2 = NULL;
@@ -3027,8 +3026,8 @@ int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni,
 	err = ni_add_name(new_dir_ni, ni, new_de);
 	if (!err) {
 		err = ni_remove_name(dir_ni, ni, de, &de2, &undo);
-		if (err && ni_remove_name(new_dir_ni, ni, new_de, &de2, &undo))
-			*is_bad = true;
+		WARN_ON(err && ni_remove_name(new_dir_ni, ni, new_de, &de2,
+			&undo));
 	}
 
 	/*
diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
index b807744fc6a9..0db7ca3b64ea 100644
--- a/fs/ntfs3/namei.c
+++ b/fs/ntfs3/namei.c
@@ -244,7 +244,7 @@ static int ntfs_rename(struct mnt_idmap *idmap, struct inode *dir,
 	struct ntfs_inode *ni = ntfs_i(inode);
 	struct inode *new_inode = d_inode(new_dentry);
 	struct NTFS_DE *de, *new_de;
-	bool is_same, is_bad;
+	bool is_same;
 	/*
 	 * de		- memory of PATH_MAX bytes:
 	 * [0-1024)	- original name (dentry->d_name)
@@ -313,12 +313,8 @@ static int ntfs_rename(struct mnt_idmap *idmap, struct inode *dir,
 	if (dir_ni != new_dir_ni)
 		ni_lock_dir2(new_dir_ni);
 
-	is_bad = false;
-	err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de, &is_bad);
-	if (is_bad) {
-		/* Restore after failed rename failed too. */
-		_ntfs_bad_inode(inode);
-	} else if (!err) {
+	err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de);
+	if (!err) {
 		simple_rename_timestamp(dir, dentry, new_dir, new_dentry);
 		mark_inode_dirty(inode);
 		mark_inode_dirty(dir);
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 36b8052660d5..f54635df18fa 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -577,8 +577,7 @@ int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni,
 		struct NTFS_DE *de);
 
 int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni,
-	      struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de,
-	      bool *is_bad);
+	      struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de);
 
 bool ni_is_dirty(struct inode *inode);
 
-- 
2.43.0