[syzbot] [ntfs3?] general protection fault in pick_link (2)

[syzbot] [ntfs3?] general protection fault in pick_link (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    9afe652958c3 Merge tag 'x86_urgent_for_6.16-rc3' of git://..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10cf95d4580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=d11f52d3049c3790
dashboard link: https://syzkaller.appspot.com/bug?extid=1aa90f0eb1fc3e77d969
compiler:       Debian clang version 20.1.6 (++20250514063057+1e4d39e07757-1~exp1~20250514183223.118), Debian LLD 20.1.6
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11969e82580000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14fd450c580000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/d900f083ada3/non_bootable_disk-9afe6529.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/46695f4e5fdb/vmlinux-9afe6529.xz
kernel image: https://storage.googleapis.com/syzbot-assets/4357674be01a/bzImage-9afe6529.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/dd817d4f3932/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+1aa90f0eb1fc3e77d969@syzkaller.appspotmail.com

ntfs3(loop0): ino=1b, "file0" ntfs_rename
Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
CPU: 0 UID: 0 PID: 5313 Comm: syz-executor352 Not tainted 6.16.0-rc2-syzkaller-00024-g9afe652958c3 #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
RIP: 0010:pick_link+0x4f1/0xe80 fs/namei.c:1949
Code: 4c 89 f7 e8 81 fa ea ff 4d 8b 36 4d 85 f6 0f 84 9b 00 00 00 e8 50 7c 87 ff 49 bf 00 00 00 00 00 fc ff df 4c 89 f0 48 c1 e8 03 <42> 0f b6 04 38 84 c0 0f 85 b9 06 00 00 41 0f b6 2e bf 2f 00 00 00
RSP: 0018:ffffc9000d2477e8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffc9000d247908 RCX: ffff88801a34c880
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff823d3d98
R10: 000000003b9aca00 R11: ffffffff823cb0a0 R12: 1ffff92001a48f8b
R13: ffffc9000d247c20 R14: 0000000000000002 R15: dffffc0000000000
FS:  00005555725f0380(0000) GS:ffff88808d251000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f4a1afaf000 CR3: 00000000438a8000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 step_into+0xc5d/0xf30 fs/namei.c:2008
 open_last_lookups fs/namei.c:3843 [inline]
 path_openat+0x1bc6/0x3830 fs/namei.c:4052
 do_filp_open+0x1fa/0x410 fs/namei.c:4082
 do_sys_openat2+0x121/0x1c0 fs/open.c:1437
 do_sys_open fs/open.c:1452 [inline]
 __do_sys_open fs/open.c:1460 [inline]
 __se_sys_open fs/open.c:1456 [inline]
 __x64_sys_open+0x11e/0x150 fs/open.c:1456
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f5aa2adeed9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffc2e7cf7e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 00007f5aa2b47ac0 RCX: 00007f5aa2adeed9
RDX: 0000000000000000 RSI: 0000000000048500 RDI: 0000200000000a80
RBP: 0000200000001240 R08: 00005555725f14c0 R09: 00005555725f14c0
R10: 00005555725f14c0 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffc2e7cfa38 R14: 431bde82d7b634db R15: 00007f5aa2b2803b
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:pick_link+0x4f1/0xe80 fs/namei.c:1949
Code: 4c 89 f7 e8 81 fa ea ff 4d 8b 36 4d 85 f6 0f 84 9b 00 00 00 e8 50 7c 87 ff 49 bf 00 00 00 00 00 fc ff df 4c 89 f0 48 c1 e8 03 <42> 0f b6 04 38 84 c0 0f 85 b9 06 00 00 41 0f b6 2e bf 2f 00 00 00
RSP: 0018:ffffc9000d2477e8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffc9000d247908 RCX: ffff88801a34c880
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff823d3d98
R10: 000000003b9aca00 R11: ffffffff823cb0a0 R12: 1ffff92001a48f8b
R13: ffffc9000d247c20 R14: 0000000000000002 R15: dffffc0000000000
FS:  00005555725f0380(0000) GS:ffff88808d251000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055ff6c3df070 CR3: 00000000438a8000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
   0:	4c 89 f7             	mov    %r14,%rdi
   3:	e8 81 fa ea ff       	call   0xffeafa89
   8:	4d 8b 36             	mov    (%r14),%r14
   b:	4d 85 f6             	test   %r14,%r14
   e:	0f 84 9b 00 00 00    	je     0xaf
  14:	e8 50 7c 87 ff       	call   0xff877c69
  19:	49 bf 00 00 00 00 00 	movabs $0xdffffc0000000000,%r15
  20:	fc ff df
  23:	4c 89 f0             	mov    %r14,%rax
  26:	48 c1 e8 03          	shr    $0x3,%rax
* 2a:	42 0f b6 04 38       	movzbl (%rax,%r15,1),%eax <-- trapping instruction
  2f:	84 c0                	test   %al,%al
  31:	0f 85 b9 06 00 00    	jne    0x6f0
  37:	41 0f b6 2e          	movzbl (%r14),%ebp
  3b:	bf 2f 00 00 00       	mov    $0x2f,%edi


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] fs: Prevent non-symlinks from entering pick link

[Edward Adam Davis]

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

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Al Viro]

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.

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Al Viro]

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.

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Al Viro]

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.

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Edward Adam Davis]

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?

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Al Viro]

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.

Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Edward Adam Davis]

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

[PATCH V2] fs/ntfs3: cancle set bad inode after removing name fails

[Edward Adam Davis]

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