KASAN report null-ptr-deref as follows:
BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd]
Write of size 8 at addr 000000000000005d by task a.out/852
CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x34/0x44
kasan_report+0xab/0x120
? nfsd_mkdir+0x71/0x1c0 [nfsd]
? nfsd_fill_super+0xc6/0xe0 [nfsd]
nfsd_fill_super+0xc6/0xe0 [nfsd]
? nfsd_mkdir+0x1c0/0x1c0 [nfsd]
get_tree_keyed+0x8e/0x100
vfs_get_tree+0x41/0xf0
__do_sys_fsconfig+0x590/0x670
? fscontext_read+0x180/0x180
? anon_inode_getfd+0x4f/0x70
do_syscall_64+0x35/0x80
entry_SYSCALL_64_after_hwframe+0x44/0xae
This can be reproduce by concurrent operations:
1. fsopen(nfsd)/fsconfig
2. insmod/rmmod nfsd
Since the nfsd file system is registered before than nfsd_net allocated,
the caller may get the file_system_type and use the nfsd_net before it
allocated, then null-ptr-deref occured.
So should allocate the nfsd_net firstly, other than register file system.
Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first")
Cc: [email protected]
Signed-off-by: Zhang Xiaoxu <[email protected]>
---
fs/nfsd/nfsctl.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 16920e4512bd..e17100e90e19 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1535,20 +1535,20 @@ static int __init init_nfsd(void)
retval = create_proc_exports_entry();
if (retval)
goto out_free_lockd;
- retval = register_filesystem(&nfsd_fs_type);
- if (retval)
- goto out_free_exports;
retval = register_pernet_subsys(&nfsd_net_ops);
if (retval < 0)
- goto out_free_filesystem;
+ goto out_free_exports;
retval = register_cld_notifier();
+ if (retval)
+ goto out_free_subsys;
+ retval = register_filesystem(&nfsd_fs_type);
if (retval)
goto out_free_all;
return 0;
out_free_all:
+ unregister_cld_notifier();
+out_free_subsys:
unregister_pernet_subsys(&nfsd_net_ops);
-out_free_filesystem:
- unregister_filesystem(&nfsd_fs_type);
out_free_exports:
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
@@ -1566,6 +1566,7 @@ static int __init init_nfsd(void)
static void __exit exit_nfsd(void)
{
+ unregister_filesystem(&nfsd_fs_type);
unregister_cld_notifier();
unregister_pernet_subsys(&nfsd_net_ops);
nfsd_drc_slab_free();
@@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
nfsd_lockd_shutdown();
nfsd4_free_slabs();
nfsd4_exit_pnfs();
- unregister_filesystem(&nfsd_fs_type);
}
MODULE_AUTHOR("Olaf Kirch <[email protected]>");
--
2.31.1
[ Note well: Updated Bruce's email address. ]
> On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <[email protected]> wrote:
>
> KASAN report null-ptr-deref as follows:
>
> BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd]
> Write of size 8 at addr 000000000000005d by task a.out/852
>
> CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl+0x34/0x44
> kasan_report+0xab/0x120
> ? nfsd_mkdir+0x71/0x1c0 [nfsd]
> ? nfsd_fill_super+0xc6/0xe0 [nfsd]
> nfsd_fill_super+0xc6/0xe0 [nfsd]
> ? nfsd_mkdir+0x1c0/0x1c0 [nfsd]
> get_tree_keyed+0x8e/0x100
> vfs_get_tree+0x41/0xf0
> __do_sys_fsconfig+0x590/0x670
> ? fscontext_read+0x180/0x180
> ? anon_inode_getfd+0x4f/0x70
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> This can be reproduce by concurrent operations:
> 1. fsopen(nfsd)/fsconfig
> 2. insmod/rmmod nfsd
>
> Since the nfsd file system is registered before than nfsd_net allocated,
> the caller may get the file_system_type and use the nfsd_net before it
> allocated, then null-ptr-deref occured.
>
> So should allocate the nfsd_net firstly, other than register file system.
IIUC, I suggest: "So init_nfsd() should call register_filesystem() last."
> Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first")
> Cc: [email protected]
> Signed-off-by: Zhang Xiaoxu <[email protected]>
I think this looks right. Bruce, as author of bd5ae9288d64, any
thoughts?
I need a v2 of this, though. The current version conflicts with the
courteous server patches already in my for-next branch. See:
https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next
> ---
> fs/nfsd/nfsctl.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 16920e4512bd..e17100e90e19 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void)
> retval = create_proc_exports_entry();
> if (retval)
> goto out_free_lockd;
> - retval = register_filesystem(&nfsd_fs_type);
> - if (retval)
> - goto out_free_exports;
> retval = register_pernet_subsys(&nfsd_net_ops);
> if (retval < 0)
> - goto out_free_filesystem;
> + goto out_free_exports;
> retval = register_cld_notifier();
> + if (retval)
> + goto out_free_subsys;
> + retval = register_filesystem(&nfsd_fs_type);
> if (retval)
> goto out_free_all;
> return 0;
> out_free_all:
> + unregister_cld_notifier();
> +out_free_subsys:
> unregister_pernet_subsys(&nfsd_net_ops);
> -out_free_filesystem:
> - unregister_filesystem(&nfsd_fs_type);
> out_free_exports:
> remove_proc_entry("fs/nfs/exports", NULL);
> remove_proc_entry("fs/nfs", NULL);
> @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void)
>
> static void __exit exit_nfsd(void)
> {
> + unregister_filesystem(&nfsd_fs_type);
> unregister_cld_notifier();
> unregister_pernet_subsys(&nfsd_net_ops);
> nfsd_drc_slab_free();
> @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
> nfsd_lockd_shutdown();
> nfsd4_free_slabs();
> nfsd4_exit_pnfs();
> - unregister_filesystem(&nfsd_fs_type);
> }
>
> MODULE_AUTHOR("Olaf Kirch <[email protected]>");
> --
> 2.31.1
>
--
Chuck Lever
Thanks for your review.
I will send v2 patch to update the commit description and resolve the
conflicts in your for-next branch
?? 2022/5/21 0:05, Bruce Fields ะด??:
> On Fri, May 20, 2022 at 03:22:51PM +0000, Chuck Lever III wrote:
>> [ Note well: Updated Bruce's email address. ]
>>
>>
>>> On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <[email protected]> wrote:
>>>
>>> KASAN report null-ptr-deref as follows:
>>>
>>> BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd]
>>> Write of size 8 at addr 000000000000005d by task a.out/852
>>>
>>> CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014
>>> Call Trace:
>>> <TASK>
>>> dump_stack_lvl+0x34/0x44
>>> kasan_report+0xab/0x120
>>> ? nfsd_mkdir+0x71/0x1c0 [nfsd]
>>> ? nfsd_fill_super+0xc6/0xe0 [nfsd]
>>> nfsd_fill_super+0xc6/0xe0 [nfsd]
>>> ? nfsd_mkdir+0x1c0/0x1c0 [nfsd]
>>> get_tree_keyed+0x8e/0x100
>>> vfs_get_tree+0x41/0xf0
>>> __do_sys_fsconfig+0x590/0x670
>>> ? fscontext_read+0x180/0x180
>>> ? anon_inode_getfd+0x4f/0x70
>>> do_syscall_64+0x35/0x80
>>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>>>
>>> This can be reproduce by concurrent operations:
>>> 1. fsopen(nfsd)/fsconfig
>>> 2. insmod/rmmod nfsd
>>>
>>> Since the nfsd file system is registered before than nfsd_net allocated,
>>> the caller may get the file_system_type and use the nfsd_net before it
>>> allocated, then null-ptr-deref occured.
>>>
>>> So should allocate the nfsd_net firstly, other than register file system.
>>
>> IIUC, I suggest: "So init_nfsd() should call register_filesystem() last."
>>
>>
>>> Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first")
>>> Cc: [email protected]
>>> Signed-off-by: Zhang Xiaoxu <[email protected]>
>>
>> I think this looks right. Bruce, as author of bd5ae9288d64, any
>> thoughts?
>
> I'm not seeing any problem with the patch.
>
> Reviewed-by: J. Bruce Fields <[email protected]>
>
> --b.
>
>>
>> I need a v2 of this, though. The current version conflicts with the
>> courteous server patches already in my for-next branch. See:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next
>>
>>
>>> ---
>>> fs/nfsd/nfsctl.c | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>> index 16920e4512bd..e17100e90e19 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void)
>>> retval = create_proc_exports_entry();
>>> if (retval)
>>> goto out_free_lockd;
>>> - retval = register_filesystem(&nfsd_fs_type);
>>> - if (retval)
>>> - goto out_free_exports;
>>> retval = register_pernet_subsys(&nfsd_net_ops);
>>> if (retval < 0)
>>> - goto out_free_filesystem;
>>> + goto out_free_exports;
>>> retval = register_cld_notifier();
>>> + if (retval)
>>> + goto out_free_subsys;
>>> + retval = register_filesystem(&nfsd_fs_type);
>>> if (retval)
>>> goto out_free_all;
>>> return 0;
>>> out_free_all:
>>> + unregister_cld_notifier();
>>> +out_free_subsys:
>>> unregister_pernet_subsys(&nfsd_net_ops);
>>> -out_free_filesystem:
>>> - unregister_filesystem(&nfsd_fs_type);
>>> out_free_exports:
>>> remove_proc_entry("fs/nfs/exports", NULL);
>>> remove_proc_entry("fs/nfs", NULL);
>>> @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void)
>>>
>>> static void __exit exit_nfsd(void)
>>> {
>>> + unregister_filesystem(&nfsd_fs_type);
>>> unregister_cld_notifier();
>>> unregister_pernet_subsys(&nfsd_net_ops);
>>> nfsd_drc_slab_free();
>>> @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
>>> nfsd_lockd_shutdown();
>>> nfsd4_free_slabs();
>>> nfsd4_exit_pnfs();
>>> - unregister_filesystem(&nfsd_fs_type);
>>> }
>>>
>>> MODULE_AUTHOR("Olaf Kirch <[email protected]>");
>>> --
>>> 2.31.1
>>>
>>
>> --
>> Chuck Lever
>>
>>
> .
>
On Fri, May 20, 2022 at 03:22:51PM +0000, Chuck Lever III wrote:
> [ Note well: Updated Bruce's email address. ]
>
>
> > On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <[email protected]> wrote:
> >
> > KASAN report null-ptr-deref as follows:
> >
> > BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd]
> > Write of size 8 at addr 000000000000005d by task a.out/852
> >
> > CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014
> > Call Trace:
> > <TASK>
> > dump_stack_lvl+0x34/0x44
> > kasan_report+0xab/0x120
> > ? nfsd_mkdir+0x71/0x1c0 [nfsd]
> > ? nfsd_fill_super+0xc6/0xe0 [nfsd]
> > nfsd_fill_super+0xc6/0xe0 [nfsd]
> > ? nfsd_mkdir+0x1c0/0x1c0 [nfsd]
> > get_tree_keyed+0x8e/0x100
> > vfs_get_tree+0x41/0xf0
> > __do_sys_fsconfig+0x590/0x670
> > ? fscontext_read+0x180/0x180
> > ? anon_inode_getfd+0x4f/0x70
> > do_syscall_64+0x35/0x80
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > This can be reproduce by concurrent operations:
> > 1. fsopen(nfsd)/fsconfig
> > 2. insmod/rmmod nfsd
> >
> > Since the nfsd file system is registered before than nfsd_net allocated,
> > the caller may get the file_system_type and use the nfsd_net before it
> > allocated, then null-ptr-deref occured.
> >
> > So should allocate the nfsd_net firstly, other than register file system.
>
> IIUC, I suggest: "So init_nfsd() should call register_filesystem() last."
>
>
> > Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first")
> > Cc: [email protected]
> > Signed-off-by: Zhang Xiaoxu <[email protected]>
>
> I think this looks right. Bruce, as author of bd5ae9288d64, any
> thoughts?
I'm not seeing any problem with the patch.
Reviewed-by: J. Bruce Fields <[email protected]>
--b.
>
> I need a v2 of this, though. The current version conflicts with the
> courteous server patches already in my for-next branch. See:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next
>
>
> > ---
> > fs/nfsd/nfsctl.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 16920e4512bd..e17100e90e19 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void)
> > retval = create_proc_exports_entry();
> > if (retval)
> > goto out_free_lockd;
> > - retval = register_filesystem(&nfsd_fs_type);
> > - if (retval)
> > - goto out_free_exports;
> > retval = register_pernet_subsys(&nfsd_net_ops);
> > if (retval < 0)
> > - goto out_free_filesystem;
> > + goto out_free_exports;
> > retval = register_cld_notifier();
> > + if (retval)
> > + goto out_free_subsys;
> > + retval = register_filesystem(&nfsd_fs_type);
> > if (retval)
> > goto out_free_all;
> > return 0;
> > out_free_all:
> > + unregister_cld_notifier();
> > +out_free_subsys:
> > unregister_pernet_subsys(&nfsd_net_ops);
> > -out_free_filesystem:
> > - unregister_filesystem(&nfsd_fs_type);
> > out_free_exports:
> > remove_proc_entry("fs/nfs/exports", NULL);
> > remove_proc_entry("fs/nfs", NULL);
> > @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void)
> >
> > static void __exit exit_nfsd(void)
> > {
> > + unregister_filesystem(&nfsd_fs_type);
> > unregister_cld_notifier();
> > unregister_pernet_subsys(&nfsd_net_ops);
> > nfsd_drc_slab_free();
> > @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
> > nfsd_lockd_shutdown();
> > nfsd4_free_slabs();
> > nfsd4_exit_pnfs();
> > - unregister_filesystem(&nfsd_fs_type);
> > }
> >
> > MODULE_AUTHOR("Olaf Kirch <[email protected]>");
> > --
> > 2.31.1
> >
>
> --
> Chuck Lever
>
>