BUG: unable to handle kernel paging request in dput (2)

BUG: unable to handle kernel paging request in dput (2)

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    02495e76ded5 Add linux-next specific files for 20190130
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=17a5bb64c00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=a2b2e9c0bc43c14d
dashboard link: https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

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

BUG: unable to handle kernel paging request at ffffffffffffffea
#PF error: [normal kernel read fault]
PGD 9874067 P4D 9874067 PUD 9876067 PMD 0
Oops: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 18660 Comm: syz-executor2 Not tainted 5.0.0-rc4-next-20190130  
#22
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
RIP: 0010:fast_dput fs/dcache.c:700 [inline]
RIP: 0010:dput+0x11e/0x790 fs/dcache.c:819
Code: e8 97 2e a8 ff 45 84 ed 0f 84 e2 03 00 00 e8 49 2d a8 ff 4c 89 e0 48  
c1 e8 03 0f b6 04 18 84 c0 74 08 3c 03 0f 8e f7 04 00 00 <45> 8b 2c 24 4d  
8d bc 24 80 00 00 00 31 ff 41 83 e5 08 44 89 ee e8
RSP: 0018:ffff888050bef740 EFLAGS: 00010246
RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffc9000c426000
RDX: 0000000000000777 RSI: ffffffff81d9ec07 RDI: 0000000000000001
RBP: ffff888050bef7d8 R08: ffff88805fc105c0 R09: ffffed1015ce5b80
R10: ffffed1015ce5b7f R11: ffff8880ae72dbfb R12: ffffffffffffffea
R13: 0000000000000001 R14: 1ffff1100a17deea R15: ffff888050bef8a0
FS:  00007f5052230700(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffea CR3: 00000000a5276000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  do_blk_trace_setup+0x8e9/0xdb0 kernel/trace/blktrace.c:561
  __blk_trace_setup+0xe3/0x190 kernel/trace/blktrace.c:577
  blk_trace_ioctl+0x170/0x300 kernel/trace/blktrace.c:716
  blkdev_ioctl+0x141/0x2120 block/ioctl.c:591
  block_ioctl+0xee/0x130 fs/block_dev.c:1914
  vfs_ioctl fs/ioctl.c:46 [inline]
  file_ioctl fs/ioctl.c:509 [inline]
  do_vfs_ioctl+0x107b/0x17d0 fs/ioctl.c:696
  ksys_ioctl+0xab/0xd0 fs/ioctl.c:713
  __do_sys_ioctl fs/ioctl.c:720 [inline]
  __se_sys_ioctl fs/ioctl.c:718 [inline]
  __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:718
  do_syscall_64+0x1a3/0x800 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458089
Code: 6d b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 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 0f 83 3b b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f505222fc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000458089
RDX: 00000000200002c0 RSI: 00000000c0481273 RDI: 0000000000000003
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f50522306d4
R13: 00000000004bf54d R14: 00000000004d0e38 R15: 00000000ffffffff
Modules linked in:
CR2: ffffffffffffffea
---[ end trace 454866349b9af1ed ]---
RIP: 0010:fast_dput fs/dcache.c:700 [inline]
RIP: 0010:dput+0x11e/0x790 fs/dcache.c:819
Code: e8 97 2e a8 ff 45 84 ed 0f 84 e2 03 00 00 e8 49 2d a8 ff 4c 89 e0 48  
c1 e8 03 0f b6 04 18 84 c0 74 08 3c 03 0f 8e f7 04 00 00 <45> 8b 2c 24 4d  
8d bc 24 80 00 00 00 31 ff 41 83 e5 08 44 89 ee e8
RSP: 0018:ffff888050bef740 EFLAGS: 00010246
RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffc9000c426000
RDX: 0000000000000777 RSI: ffffffff81d9ec07 RDI: 0000000000000001
RBP: ffff888050bef7d8 R08: ffff88805fc105c0 R09: ffffed1015ce5b80
R10: ffffed1015ce5b7f R11: ffff8880ae72dbfb R12: ffffffffffffffea
R13: 0000000000000001 R14: 1ffff1100a17deea R15: ffff888050bef8a0
FS:  00007f5052230700(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffea CR3: 00000000a5276000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This bug 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 bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.

Re: BUG: unable to handle kernel paging request in dput (2)

[Tetsuo Handa]

Hello, Omar.

syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is

        dir = debugfs_lookup(buts->name, blk_debugfs_root);
        if (!dir)
                bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);

added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").

Currently, Greg Kroah-Hartman is posting patches:

  When calling debugfs functions, there is no need to ever check the
  return value.  The function can work or not, but the code logic should
  never do something different based on this.

Omar, what do you want to do for this case?

[1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790

Re: BUG: unable to handle kernel paging request in dput (2)

[syzbot]

syzbot has found a reproducer for the following crash on:

HEAD commit:    02495e76ded5 Add linux-next specific files for 20190130
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=116209ef400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=a2b2e9c0bc43c14d
dashboard link: https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10bdea98c00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16390850c00000

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

BUG: unable to handle kernel paging request at ffffffffffffffea
#PF error: [normal kernel read fault]
PGD 9874067 P4D 9874067 PUD 9876067 PMD 0
Oops: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 7985 Comm: syz-executor249 Not tainted 5.0.0-rc4-next-20190130  
#22
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
RIP: 0010:fast_dput fs/dcache.c:700 [inline]
RIP: 0010:dput+0x11e/0x790 fs/dcache.c:819
Code: e8 97 2e a8 ff 45 84 ed 0f 84 e2 03 00 00 e8 49 2d a8 ff 4c 89 e0 48  
c1 e8 03 0f b6 04 18 84 c0 74 08 3c 03 0f 8e f7 04 00 00 <45> 8b 2c 24 4d  
8d bc 24 80 00 00 00 31 ff 41 83 e5 08 44 89 ee e8
RSP: 0018:ffff88808bc57740 EFLAGS: 00010246
RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffffff81d9effa
RDX: 0000000000000000 RSI: ffffffff81d9ec07 RDI: 0000000000000001
RBP: ffff88808bc577d8 R08: ffff888090abc500 R09: ffffed1015cc5b80
R10: ffffed1015cc5b7f R11: ffff8880ae62dbfb R12: ffffffffffffffea
R13: 0000000000000001 R14: 1ffff1101178aeea R15: ffff88808bc578a0
FS:  0000000001144880(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffea CR3: 00000000946ec000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  do_blk_trace_setup+0x8e9/0xdb0 kernel/trace/blktrace.c:561
  __blk_trace_setup+0xe3/0x190 kernel/trace/blktrace.c:577
  blk_trace_ioctl+0x170/0x300 kernel/trace/blktrace.c:716
  blkdev_ioctl+0x141/0x2120 block/ioctl.c:591
  block_ioctl+0xee/0x130 fs/block_dev.c:1914
  vfs_ioctl fs/ioctl.c:46 [inline]
  file_ioctl fs/ioctl.c:509 [inline]
  do_vfs_ioctl+0x107b/0x17d0 fs/ioctl.c:696
  ksys_ioctl+0xab/0xd0 fs/ioctl.c:713
  __do_sys_ioctl fs/ioctl.c:720 [inline]
  __se_sys_ioctl fs/ioctl.c:718 [inline]
  __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:718
  do_syscall_64+0x1a3/0x800 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440139
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 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 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fffa5cab9f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440139
RDX: 00000000200002c0 RSI: 00000000c0481273 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004019c0
R13: 0000000000401a50 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
CR2: ffffffffffffffea
---[ end trace 9fd433d20871e341 ]---
RIP: 0010:fast_dput fs/dcache.c:700 [inline]
RIP: 0010:dput+0x11e/0x790 fs/dcache.c:819
Code: e8 97 2e a8 ff 45 84 ed 0f 84 e2 03 00 00 e8 49 2d a8 ff 4c 89 e0 48  
c1 e8 03 0f b6 04 18 84 c0 74 08 3c 03 0f 8e f7 04 00 00 <45> 8b 2c 24 4d  
8d bc 24 80 00 00 00 31 ff 41 83 e5 08 44 89 ee e8
RSP: 0018:ffff88808bc57740 EFLAGS: 00010246
RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffffff81d9effa
RDX: 0000000000000000 RSI: ffffffff81d9ec07 RDI: 0000000000000001
RBP: ffff88808bc577d8 R08: ffff888090abc500 R09: ffffed1015cc5b80
R10: ffffed1015cc5b7f R11: ffff8880ae62dbfb R12: ffffffffffffffea
R13: 0000000000000001 R14: 1ffff1101178aeea R15: ffff88808bc578a0
FS:  0000000001144880(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffea CR3: 00000000946ec000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

Re: BUG: unable to handle kernel paging request in dput (2)

[Greg Kroah-Hartman]

On Wed, Jan 30, 2019 at 08:11:17PM +0900, Tetsuo Handa wrote:
> Hello, Omar.
> 
> syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> 
>         dir = debugfs_lookup(buts->name, blk_debugfs_root);
>         if (!dir)
>                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> 
> added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> 
> Currently, Greg Kroah-Hartman is posting patches:
> 
>   When calling debugfs functions, there is no need to ever check the
>   return value.  The function can work or not, but the code logic should
>   never do something different based on this.
> 
> Omar, what do you want to do for this case?
> 
> [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> 

Obviously debugfs_lookup() should have its return value checked, so
that's fine :)

But how are we getting an error with dput()?  Are you sure this is a
debugfs issue and not a relayfs problem?

I'll send patches to clean up the tracing code's use of debugfs later,
as it is, I don't see anything obviously wrong with it.

thanks,

greg k-h

Re: BUG: unable to handle kernel paging request in dput (2)

[Tetsuo Handa]

On 2019/01/30 20:11, Tetsuo Handa wrote:
> Hello, Omar.
> 
> syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> 
>         dir = debugfs_lookup(buts->name, blk_debugfs_root);
>         if (!dir)
>                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> 
> added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> 
> Currently, Greg Kroah-Hartman is posting patches:
> 
>   When calling debugfs functions, there is no need to ever check the
>   return value.  The function can work or not, but the code logic should
>   never do something different based on this.
> 
> Omar, what do you want to do for this case?
> 
> [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> 

The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").

Re: BUG: unable to handle kernel paging request in dput (2)

[Greg Kroah-Hartman]

On Wed, Jan 30, 2019 at 08:26:24PM +0900, Tetsuo Handa wrote:
> On 2019/01/30 20:11, Tetsuo Handa wrote:
> > Hello, Omar.
> > 
> > syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> > 
> >         dir = debugfs_lookup(buts->name, blk_debugfs_root);
> >         if (!dir)
> >                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > 
> > added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> > 
> > Currently, Greg Kroah-Hartman is posting patches:
> > 
> >   When calling debugfs functions, there is no need to ever check the
> >   return value.  The function can work or not, but the code logic should
> >   never do something different based on this.
> > 
> > Omar, what do you want to do for this case?
> > 
> > [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> > 
> 
> The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
> modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").

Ah, my fault then, yes, debugfs_lookup() should return NULL if nothing
is found.  My patch was wrong, let me go fix this up, thanks for the
report.

greg k-h

Re: BUG: unable to handle kernel paging request in dput (2)

[Greg Kroah-Hartman]

On Wed, Jan 30, 2019 at 08:26:24PM +0900, Tetsuo Handa wrote:
> On 2019/01/30 20:11, Tetsuo Handa wrote:
> > Hello, Omar.
> > 
> > syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> > 
> >         dir = debugfs_lookup(buts->name, blk_debugfs_root);
> >         if (!dir)
> >                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > 
> > added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> > 
> > Currently, Greg Kroah-Hartman is posting patches:
> > 
> >   When calling debugfs functions, there is no need to ever check the
> >   return value.  The function can work or not, but the code logic should
> >   never do something different based on this.
> > 
> > Omar, what do you want to do for this case?
> > 
> > [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> > 
> 
> The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
> modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").

Ok, the patch below should fix this up.

thanks,

greg k-h

-------------------------

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: [PATCH] debugfs: debugfs_lookup() should return NULL if not found

Lots of callers of debugfs_lookup() were just checking NULL to see if
the file/directory was found or not.  By changing this in ff9fb72bc077
("debugfs: return error values, not NULL") we caused some subsystems to
easily crash.

Fixes: ff9fb72bc077 ("debugfs: return error values, not NULL")
Reported-by: syzbot+b382ba6a802a3d242790@syzkaller.appspotmail.com
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Jens Axboe <axboe@fb.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/debugfs/inode.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index b16f8035b1af..29c68c5d44d5 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
  * @parent: a pointer to the parent dentry of the file.
  *
  * This function will return a pointer to a dentry if it succeeds.  If the file
- * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
- * returned dentry must be passed to dput() when it is no longer needed.
+ * doesn't exist or an error occurs, %NULL will be returned.  The returned
+ * dentry must be passed to dput() when it is no longer needed.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
 	struct dentry *dentry;
 
 	if (IS_ERR(parent))
-		return parent;
+		return NULL;
 
 	if (!parent)
 		parent = debugfs_mount->mnt_root;
 
 	dentry = lookup_one_len_unlocked(name, parent, strlen(name));
 	if (IS_ERR(dentry))
-		return dentry;
+		return NULL;
 	if (!d_really_is_positive(dentry)) {
 		dput(dentry);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 	return dentry;
 }
-- 
2.20.1

Re: BUG: unable to handle kernel paging request in dput (2)

[Kees Cook]

On Thu, Jan 31, 2019 at 12:41 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 30, 2019 at 08:26:24PM +0900, Tetsuo Handa wrote:
> > On 2019/01/30 20:11, Tetsuo Handa wrote:
> > > Hello, Omar.
> > >
> > > syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> > >
> > >         dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > >         if (!dir)
> > >                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > >
> > > added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> > >
> > > Currently, Greg Kroah-Hartman is posting patches:
> > >
> > >   When calling debugfs functions, there is no need to ever check the
> > >   return value.  The function can work or not, but the code logic should
> > >   never do something different based on this.
> > >
> > > Omar, what do you want to do for this case?
> > >
> > > [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> > >
> >
> > The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
> > modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").
>
> Ok, the patch below should fix this up.
>
> thanks,
>
> greg k-h
>
> -------------------------
>
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Subject: [PATCH] debugfs: debugfs_lookup() should return NULL if not found
>
> Lots of callers of debugfs_lookup() were just checking NULL to see if
> the file/directory was found or not.  By changing this in ff9fb72bc077
> ("debugfs: return error values, not NULL") we caused some subsystems to
> easily crash.
>
> Fixes: ff9fb72bc077 ("debugfs: return error values, not NULL")
> Reported-by: syzbot+b382ba6a802a3d242790@syzkaller.appspotmail.com
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Jens Axboe <axboe@fb.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/debugfs/inode.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index b16f8035b1af..29c68c5d44d5 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
>   * @parent: a pointer to the parent dentry of the file.
>   *
>   * This function will return a pointer to a dentry if it succeeds.  If the file
> - * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> - * returned dentry must be passed to dput() when it is no longer needed.
> + * doesn't exist or an error occurs, %NULL will be returned.  The returned
> + * dentry must be passed to dput() when it is no longer needed.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
>         struct dentry *dentry;
>
>         if (IS_ERR(parent))
> -               return parent;
> +               return NULL;
>
>         if (!parent)
>                 parent = debugfs_mount->mnt_root;
>
>         dentry = lookup_one_len_unlocked(name, parent, strlen(name));
>         if (IS_ERR(dentry))
> -               return dentry;
> +               return NULL;
>         if (!d_really_is_positive(dentry)) {
>                 dput(dentry);
> -               return ERR_PTR(-EINVAL);
> +               return NULL;
>         }
>         return dentry;
>  }
> --
> 2.20.1
>

FYI, this patch does not fix the relay.c crash I bisected... I think
more clean-up is needed?

-Kees

Re: BUG: unable to handle kernel paging request in dput (2)

[Tetsuo Handa]

On 2019/01/31 19:09, Kees Cook wrote:
> FYI, this patch does not fix the relay.c crash I bisected... I think
> more clean-up is needed?

You mean "general protection fault in relay_open_buf" at
https://syzkaller.appspot.com/bug?id=080fbc3a5d10bd8ada100799168ebe1b70e2eefa ?

Re: BUG: unable to handle kernel paging request in dput (2)

[Greg Kroah-Hartman]

On Thu, Jan 31, 2019 at 11:09:11PM +1300, Kees Cook wrote:
> On Thu, Jan 31, 2019 at 12:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jan 30, 2019 at 08:26:24PM +0900, Tetsuo Handa wrote:
> > > On 2019/01/30 20:11, Tetsuo Handa wrote:
> > > > Hello, Omar.
> > > >
> > > > syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> > > >
> > > >         dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > > >         if (!dir)
> > > >                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > > >
> > > > added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> > > >
> > > > Currently, Greg Kroah-Hartman is posting patches:
> > > >
> > > >   When calling debugfs functions, there is no need to ever check the
> > > >   return value.  The function can work or not, but the code logic should
> > > >   never do something different based on this.
> > > >
> > > > Omar, what do you want to do for this case?
> > > >
> > > > [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> > > >
> > >
> > > The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
> > > modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").
> >
> > Ok, the patch below should fix this up.
> >
> > thanks,
> >
> > greg k-h
> >
> > -------------------------
> >
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Subject: [PATCH] debugfs: debugfs_lookup() should return NULL if not found
> >
> > Lots of callers of debugfs_lookup() were just checking NULL to see if
> > the file/directory was found or not.  By changing this in ff9fb72bc077
> > ("debugfs: return error values, not NULL") we caused some subsystems to
> > easily crash.
> >
> > Fixes: ff9fb72bc077 ("debugfs: return error values, not NULL")
> > Reported-by: syzbot+b382ba6a802a3d242790@syzkaller.appspotmail.com
> > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Jens Axboe <axboe@fb.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  fs/debugfs/inode.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index b16f8035b1af..29c68c5d44d5 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
> >   * @parent: a pointer to the parent dentry of the file.
> >   *
> >   * This function will return a pointer to a dentry if it succeeds.  If the file
> > - * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> > - * returned dentry must be passed to dput() when it is no longer needed.
> > + * doesn't exist or an error occurs, %NULL will be returned.  The returned
> > + * dentry must be passed to dput() when it is no longer needed.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
> >         struct dentry *dentry;
> >
> >         if (IS_ERR(parent))
> > -               return parent;
> > +               return NULL;
> >
> >         if (!parent)
> >                 parent = debugfs_mount->mnt_root;
> >
> >         dentry = lookup_one_len_unlocked(name, parent, strlen(name));
> >         if (IS_ERR(dentry))
> > -               return dentry;
> > +               return NULL;
> >         if (!d_really_is_positive(dentry)) {
> >                 dput(dentry);
> > -               return ERR_PTR(-EINVAL);
> > +               return NULL;
> >         }
> >         return dentry;
> >  }
> > --
> > 2.20.1
> >
> 
> FYI, this patch does not fix the relay.c crash I bisected... I think
> more clean-up is needed?

Yes, you are right, I sent you a patch for that one, here it is as well.

Note, this would be blowing up anyway if debugfs was not enabled, is
relay.c not built if that is not the case?

Ah, no, it isn't, that makes more sense now.

thanks,

greg k-h

diff --git a/kernel/relay.c b/kernel/relay.c
index 04f248644e06..9e0f52375487 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
 	dentry = chan->cb->create_buf_file(tmpname, chan->parent,
 					   S_IRUSR, buf,
 					   &chan->is_global);
+	if (IS_ERR(dentry))
+		dentry = NULL;
 
 	kfree(tmpname);
 
@@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 		dentry = chan->cb->create_buf_file(NULL, NULL,
 						   S_IRUSR, buf,
 						   &chan->is_global);
-		if (WARN_ON(dentry))
+		if (IS_ERR_OR_NULL(dentry))
 			goto free_buf;
 	}