It is confusing that tty_init() in called in the initialization of
memdev, i.e., static chr_dev_init().
Through blame, it is introduced by commit 31d1d48e199e ("Fix init
ordering of /dev/console vs callers of modprobe"), which fixes the
initialization order of /dev/console driver. However, there seems
to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
This fixes the typo, IIUC.
Note that the return value of tty_init() is always 0, and thus no error
handling is provided in chrdev_init().
Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
Signed-off-by: Xu Yu <[email protected]>
---
drivers/char/mem.c | 2 +-
fs/char_dev.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index cc296f0823bd..8c90881f8115 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
NULL, devlist[minor].name);
}
- return tty_init();
+ return 0;
}
fs_initcall(chr_dev_init);
diff --git a/fs/char_dev.c b/fs/char_dev.c
index ba0ded7842a7..fc042a0a098f 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
void __init chrdev_init(void)
{
cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
+ tty_init();
}
--
2.20.1.2432.ga663e714
On Mon, Feb 07, 2022 at 12:27:31AM +0800, Xu Yu wrote:
> It is confusing that tty_init() in called in the initialization of
> memdev, i.e., static chr_dev_init().
>
> Through blame, it is introduced by commit 31d1d48e199e ("Fix init
> ordering of /dev/console vs callers of modprobe"), which fixes the
> initialization order of /dev/console driver. However, there seems
> to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
>
> This fixes the typo, IIUC.
>
> Note that the return value of tty_init() is always 0, and thus no error
> handling is provided in chrdev_init().
>
> Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
> Signed-off-by: Xu Yu <[email protected]>
> ---
> drivers/char/mem.c | 2 +-
> fs/char_dev.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index cc296f0823bd..8c90881f8115 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
> NULL, devlist[minor].name);
> }
>
> - return tty_init();
> + return 0;
> }
>
> fs_initcall(chr_dev_init);
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index ba0ded7842a7..fc042a0a098f 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
> void __init chrdev_init(void)
> {
> cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
> + tty_init();
> }
>
You just changed the ordering sequence here, are you SURE this is
correct?
How was this tested? Did you verify that the problem that the original
commit here was fixing is now not happening again?
And what real problem is this solving? How did you hit the issue that
this solves?
And finally, yes, it is not good to throw away the return value of
tty_init(). If it really can not return anything but 0, then let us
make it a void function first.
thanks,
greg k-h
On 2/7/22 1:03 AM, Greg KH wrote:
> On Mon, Feb 07, 2022 at 12:27:31AM +0800, Xu Yu wrote:
>> It is confusing that tty_init() in called in the initialization of
>> memdev, i.e., static chr_dev_init().
>>
>> Through blame, it is introduced by commit 31d1d48e199e ("Fix init
>> ordering of /dev/console vs callers of modprobe"), which fixes the
>> initialization order of /dev/console driver. However, there seems
>> to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
>>
>> This fixes the typo, IIUC.
>>
>> Note that the return value of tty_init() is always 0, and thus no error
>> handling is provided in chrdev_init().
>>
>> Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
>> Signed-off-by: Xu Yu <[email protected]>
>> ---
>> drivers/char/mem.c | 2 +-
>> fs/char_dev.c | 1 +
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> index cc296f0823bd..8c90881f8115 100644
>> --- a/drivers/char/mem.c
>> +++ b/drivers/char/mem.c
>> @@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
>> NULL, devlist[minor].name);
>> }
>>
>> - return tty_init();
>> + return 0;
>> }
>>
>> fs_initcall(chr_dev_init);
>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>> index ba0ded7842a7..fc042a0a098f 100644
>> --- a/fs/char_dev.c
>> +++ b/fs/char_dev.c
>> @@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
>> void __init chrdev_init(void)
>> {
>> cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
>> + tty_init();
>> }
>>
>
> You just changed the ordering sequence here, are you SURE this is
> correct?
To be honest, not 100% sure.
>
> How was this tested? Did you verify that the problem that the original
> commit here was fixing is now not happening again?
I tried to reproduce the issue described in the original commit, and
failed. The issue does not appear, or my reproduction is wrong.
1. revert 31d1d48e199e manually;
2. request_module("xxx") anywhere before do_initcalls(), since
tty_init() now is initialized by module_init();
3. no warning on request_module is shown.
>
> And what real problem is this solving? How did you hit the issue that
> this solves?
No real problem actually. As described in the log, it is confusing that
tty_init() in called in the initialization of memdev. They don't have
strong dependencies. I found the issue when I read through codes of
drivers/char/mem.c.
>
> And finally, yes, it is not good to throw away the return value of
> tty_init(). If it really can not return anything but 0, then let us
> make it a void function first.
Got it. But I will first try to figure out whether this patch is a real
issue.
>
> thanks,
>
> greg k-h
--
Thanks,
Yu
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: c194b1c9f07df8228702e6117703de4b4eac0777 ("[PATCH] chardev: call tty_init() in real chrdev_init()")
url: https://github.com/0day-ci/linux/commits/Xu-Yu/chardev-call-tty_init-in-real-chrdev_init/20220207-002828
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
patch link: https://lore.kernel.org/lkml/4e753e51d0516413fbf557cf861d654ca73486cc.1644164597.git.xuyu@linux.alibaba.com
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 0.706940][ T0] BUG: kernel NULL pointer dereference, address: 000000000000008c
[ 0.708307][ T0] #PF: supervisor read access in kernel mode
[ 0.709271][ T0] #PF: error_code(0x0000) - not-present page
[ 0.709913][ T0] PGD 0 P4D 0
[ 0.709913][ T0] Oops: 0000 [#1] PTI
[ 0.709913][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.16.0-11414-gc194b1c9f07d #1 e2938b95216b0a390b46d405eced1ebac117cf4b
[ 0.709913][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 0.709913][ T0] RIP: 0010:kobject_get (lib/kobject.c:655)
[ 0.709913][ T0] Code: ff 48 89 ef e8 2b fd ff ff 5b 5d 41 5c e9 f2 62 54 ff 66 90 41 54 49 89 fc 55 53 e8 e4 62 54 ff 4d 85 e4 74 64 e8 da 62 54 ff <41> 0f b6 5c 24 3c 31 ff 83 e3 01 89 de e8 28 64 54 ff 84 db 74 56
All code
========
0: ff 48 89 decl -0x77(%rax)
3: ef out %eax,(%dx)
4: e8 2b fd ff ff callq 0xfffffffffffffd34
9: 5b pop %rbx
a: 5d pop %rbp
b: 41 5c pop %r12
d: e9 f2 62 54 ff jmpq 0xffffffffff546304
12: 66 90 xchg %ax,%ax
14: 41 54 push %r12
16: 49 89 fc mov %rdi,%r12
19: 55 push %rbp
1a: 53 push %rbx
1b: e8 e4 62 54 ff callq 0xffffffffff546304
20: 4d 85 e4 test %r12,%r12
23: 74 64 je 0x89
25: e8 da 62 54 ff callq 0xffffffffff546304
2a:* 41 0f b6 5c 24 3c movzbl 0x3c(%r12),%ebx <-- trapping instruction
30: 31 ff xor %edi,%edi
32: 83 e3 01 and $0x1,%ebx
35: 89 de mov %ebx,%esi
37: e8 28 64 54 ff callq 0xffffffffff546464
3c: 84 db test %bl,%bl
3e: 74 56 je 0x96
Code starting with the faulting instruction
===========================================
0: 41 0f b6 5c 24 3c movzbl 0x3c(%r12),%ebx
6: 31 ff xor %edi,%edi
8: 83 e3 01 and $0x1,%ebx
b: 89 de mov %ebx,%esi
d: e8 28 64 54 ff callq 0xffffffffff54643a
12: 84 db test %bl,%bl
14: 74 56 je 0x6c
[ 0.709913][ T0] RSP: 0000:ffffffffa1403c60 EFLAGS: 00010246
[ 0.709913][ T0] RAX: 0000000000000000 RBX: 0000000000000076 RCX: ffffffff9e8860b2
[ 0.709913][ T0] RDX: 0000000000000000 RSI: ffffffffa147e100 RDI: 0000000000000002
[ 0.709913][ T0] RBP: ffff88810029d440 R08: 0000000000000040 R09: 0000000000000000
[ 0.709913][ T0] R10: ffff88810029d440 R11: 0000000000000000 R12: 0000000000000050
[ 0.709913][ T0] R13: ffff88810029d440 R14: ffffffffa114e65a R15: 0000000000700000
[ 0.709913][ T0] FS: 0000000000000000(0000) GS:ffffffffa14b3000(0000) knlGS:0000000000000000
[ 0.709913][ T0] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 0.709913][ T0] CR2: 000000000000008c CR3: 0000000268476000 CR4: 00000000000406b0
[ 0.709913][ T0] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 0.709913][ T0] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 0.709913][ T0] Call Trace:
[ 0.709913][ T0] <TASK>
[ 0.709913][ T0] kobject_add_internal (lib/kobject.c:243)
[ 0.709913][ T0] ? __sanitizer_cov_trace_pc (kernel/kcov.c:200)
[ 0.709913][ T0] kobject_add (lib/kobject.c:390 lib/kobject.c:442)
[ 0.709913][ T0] ? trace_hardirqs_on (kernel/trace/trace_preemptirq.c:50 (discriminator 22))
[ 0.709913][ T0] ? write_comp_data (kernel/kcov.c:221)
[ 0.709913][ T0] kobject_create_and_add (lib/kobject.c:815)
[ 0.709913][ T0] get_device_parent (drivers/base/core.c:2895 drivers/base/core.c:2975)
[ 0.709913][ T0] device_add (drivers/base/core.c:3327)
[ 0.709913][ T0] ? rcu_read_lock_sched_held (kernel/rcu/update.c:104 kernel/rcu/update.c:123)
[ 0.709913][ T0] ? kfree (include/trace/events/kmem.h:118 mm/slab.c:3780)
[ 0.709913][ T0] ? __sanitizer_cov_trace_pc (kernel/kcov.c:200)
[ 0.709913][ T0] device_create_groups_vargs (drivers/base/core.c:4063)
[ 0.709913][ T0] device_create (drivers/base/core.c:4107)
[ 0.709913][ T0] ? kobject_put (include/linux/refcount.h:282 include/linux/refcount.h:315 include/linux/refcount.h:333 include/linux/kref.h:64 lib/kobject.c:753)
[ 0.709913][ T0] ? __sanitizer_cov_trace_pc (kernel/kcov.c:200)
[ 0.709913][ T0] ? write_comp_data (kernel/kcov.c:221)
[ 0.709913][ T0] vcs_init (drivers/tty/vt/vc_screen.c:810)
[ 0.709913][ T0] vty_init (drivers/tty/vt/vt.c:3602)
[ 0.709913][ T0] tty_init (drivers/tty/tty_io.c:3611)
[ 0.709913][ T0] ? trace_init_perf_perm_irq_work_exit (arch/x86/kernel/time.c:86)
[ 0.709913][ T0] start_kernel (init/main.c:1117)
[ 0.709913][ T0] ? copy_bootdata (arch/x86/kernel/head64.c:440)
[ 0.709913][ T0] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:300)
[ 0.709913][ T0] </TASK>
[ 0.709913][ T0] Modules linked in:
[ 0.709913][ T0] CR2: 000000000000008c
[ 0.709913][ T0] ---[ end trace 0000000000000000 ]---
[ 0.709913][ T0] RIP: 0010:kobject_get (lib/kobject.c:655)
[ 0.709913][ T0] Code: ff 48 89 ef e8 2b fd ff ff 5b 5d 41 5c e9 f2 62 54 ff 66 90 41 54 49 89 fc 55 53 e8 e4 62 54 ff 4d 85 e4 74 64 e8 da 62 54 ff <41> 0f b6 5c 24 3c 31 ff 83 e3 01 89 de e8 28 64 54 ff 84 db 74 56
All code
========
0: ff 48 89 decl -0x77(%rax)
3: ef out %eax,(%dx)
4: e8 2b fd ff ff callq 0xfffffffffffffd34
9: 5b pop %rbx
a: 5d pop %rbp
b: 41 5c pop %r12
d: e9 f2 62 54 ff jmpq 0xffffffffff546304
12: 66 90 xchg %ax,%ax
14: 41 54 push %r12
16: 49 89 fc mov %rdi,%r12
19: 55 push %rbp
1a: 53 push %rbx
1b: e8 e4 62 54 ff callq 0xffffffffff546304
20: 4d 85 e4 test %r12,%r12
23: 74 64 je 0x89
25: e8 da 62 54 ff callq 0xffffffffff546304
2a:* 41 0f b6 5c 24 3c movzbl 0x3c(%r12),%ebx <-- trapping instruction
30: 31 ff xor %edi,%edi
32: 83 e3 01 and $0x1,%ebx
35: 89 de mov %ebx,%esi
37: e8 28 64 54 ff callq 0xffffffffff546464
3c: 84 db test %bl,%bl
3e: 74 56 je 0x96
Code starting with the faulting instruction
===========================================
0: 41 0f b6 5c 24 3c movzbl 0x3c(%r12),%ebx
6: 31 ff xor %edi,%edi
8: 83 e3 01 and $0x1,%ebx
b: 89 de mov %ebx,%esi
d: e8 28 64 54 ff callq 0xffffffffff54643a
12: 84 db test %bl,%bl
14: 74 56 je 0x6c
To reproduce:
# build kernel
cd linux
cp config-5.16.0-11414-gc194b1c9f07d .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang
On Mon, Feb 07, 2022 at 02:37:46PM +0800, Yu Xu wrote:
> On 2/7/22 1:03 AM, Greg KH wrote:
> > On Mon, Feb 07, 2022 at 12:27:31AM +0800, Xu Yu wrote:
> > > It is confusing that tty_init() in called in the initialization of
> > > memdev, i.e., static chr_dev_init().
> > >
> > > Through blame, it is introduced by commit 31d1d48e199e ("Fix init
> > > ordering of /dev/console vs callers of modprobe"), which fixes the
> > > initialization order of /dev/console driver. However, there seems
> > > to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
> > >
> > > This fixes the typo, IIUC.
> > >
> > > Note that the return value of tty_init() is always 0, and thus no error
> > > handling is provided in chrdev_init().
> > >
> > > Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
> > > Signed-off-by: Xu Yu <[email protected]>
> > > ---
> > > drivers/char/mem.c | 2 +-
> > > fs/char_dev.c | 1 +
> > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > > index cc296f0823bd..8c90881f8115 100644
> > > --- a/drivers/char/mem.c
> > > +++ b/drivers/char/mem.c
> > > @@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
> > > NULL, devlist[minor].name);
> > > }
> > > - return tty_init();
> > > + return 0;
> > > }
> > > fs_initcall(chr_dev_init);
> > > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > > index ba0ded7842a7..fc042a0a098f 100644
> > > --- a/fs/char_dev.c
> > > +++ b/fs/char_dev.c
> > > @@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
> > > void __init chrdev_init(void)
> > > {
> > > cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
> > > + tty_init();
> > > }
> >
> > You just changed the ordering sequence here, are you SURE this is
> > correct?
>
> To be honest, not 100% sure.
>
> >
> > How was this tested? Did you verify that the problem that the original
> > commit here was fixing is now not happening again?
>
> I tried to reproduce the issue described in the original commit, and
> failed. The issue does not appear, or my reproduction is wrong.
> 1. revert 31d1d48e199e manually;
> 2. request_module("xxx") anywhere before do_initcalls(), since
> tty_init() now is initialized by module_init();
> 3. no warning on request_module is shown.
>
> >
> > And what real problem is this solving? How did you hit the issue that
> > this solves?
>
> No real problem actually. As described in the log, it is confusing that
> tty_init() in called in the initialization of memdev. They don't have
> strong dependencies. I found the issue when I read through codes of
> drivers/char/mem.c.
It was added here as the linker order is what describes the init calls,
and this call is probably needed before other code in the init call
order, when things are built into the kernel.
That might help in your debugging.
good luck!
greg k-h