[PATCH] kobject: Access kobject name with caution if state is not initialized

[PATCH] kobject: Access kobject name with caution if state is not initialized

[Srikar Dronamraju]

If kobject state is not initialized, then its not even certain that
kobject'name is initialized. Hence when accessing the kobject's name
tread carefully.

A stupid module test like
https://github.com/srikard/tests/blob/master/modules/kobject_test.c
can panic the system.

With patch: We will see the correct warning.

[ 2058.129913] ------------[ cut here ]------------
[ 2058.129919] kobject: ' ' (00000000ad405b63): is not initialized, yet kobject_get() is being called.
[ 2058.129938] WARNING: CPU: 58 PID: 18529 at /home/srikar/work/linux.git/lib/kobject.c:620 kobject_get+0x90/0xb0
[ 2058.129946] Modules linked in: kobject_test(OE+) uio_pdrv_genirq(E) uio(E) leds_powernv(E) powernv_op_panel(E) ipmi_powernv(E) ipmi_devintf(E) powernv_rng(E) ibmpowernv(E) ipmi_msghandler(E) vmx_crypto(E) crct10dif_vpmsum(E) sch_fq_codel(E) ip_tables(E) x_tables(E) autofs4(E) ses enclosure scsi_transport_sas mlx4_ib mlx4_en ib_core lpfc crc32c_vpmsum mlx4_core nvmet_fc nvmet nvme_fc ipr nvme_fabrics devlink scsi_transport_fc tg3 [last unloaded: module_test]
[ 2058.130014] CPU: 58 PID: 18529 Comm: insmod Tainted: G        W  OEL    4.18.0-master+ #3
[ 2058.130022] NIP:  c000000000d5f530 LR: c000000000d5f52c CTR: 0000000000000000
[ 2058.130029] REGS: c000002fd32f3640 TRAP: 0700   Tainted: G        W  OEL     (4.18.0-master+)
[ 2058.130036] MSR:  9000000002029033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE>  CR: 48002282  XER: 20000000
[ 2058.130054] CFAR: c000000000114e10 IRQMASK: 0
               GPR00: c000000000d5f52c c000002fd32f38c0 c0000000017bc200 0000000000000057
               GPR04: 0000000000000001 000000000000107e 9000000002009033 00000000000000f2
               GPR08: 0000000000000007 0000000000000007 0000000000000001 9000000002001003
               GPR12: 0000000000002200 c000002ffffe0d00 0000000000000000 0000000000000000
               GPR16: 0000000000000000 c0000000001e92e0 0000000000000000 0000000000000100
               GPR20: 0000000000000001 c000000001662e60 c000000001662ed8 c000002feb4114a0
               GPR24: 0000000000000001 c000000000dbd438 c000000001662ea0 c000000001662ec0
               GPR28: c000002fe5f0cea0 d00000001ded03d0 c000002fd32f39a0 c000002fd32f39a0
[ 2058.130133] NIP [c000000000d5f530] kobject_get+0x90/0xb0
[ 2058.130140] LR [c000000000d5f52c] kobject_get+0x8c/0xb0
[ 2058.130146] Call Trace:
[ 2058.130150] [c000002fd32f38c0] [c000000000d5f52c] kobject_get+0x8c/0xb0 (unreliable)
[ 2058.130159] [c000002fd32f3940] [d00000001ded0088] kobject_test_init+0x80/0xb0 [kobject_test]
[ 2058.130168] [c000002fd32f39f0] [c0000000000101f8] do_one_initcall+0x58/0x240
[ 2058.130178] [c000002fd32f3ab0] [c0000000001ef2b0] do_init_module+0x90/0x260
[ 2058.130186] [c000002fd32f3b40] [c0000000001edec8] load_module+0x2d88/0x3320
[ 2058.130193] [c000002fd32f3d20] [c0000000001ee764] sys_finit_module+0xc4/0x130
[ 2058.130204] [c000002fd32f3e30] [c00000000000b288] system_call+0x5c/0x70
[ 2058.130210] Instruction dump:
[ 2058.130215] e89f0000 4b5b4c15 60000000 2f830000 419e0030 3c82ff8c 388491e0 3c62ff90
[ 2058.130228] 7fe5fb78 3863e9b0 4b3b5881 60000000 <0fe00000> e8010090 7c0803a6 4bffff88
[ 2058.130240] ---[ end trace 0f471c192555a013 ]---
[ 2070.084234] kobject_test module unloaded

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 lib/kobject.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 389829d3a1d1..2d65be37fd7b 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -16,6 +16,7 @@
 #include <linux/stat.h>
 #include <linux/slab.h>
 #include <linux/random.h>
+#include <linux/uaccess.h>
 
 /**
  * kobject_namespace - return @kobj's namespace tag
@@ -417,8 +418,11 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
 		return -EINVAL;
 
 	if (!kobj->state_initialized) {
+		char tmp;
+		int ret = probe_kernel_address(kobject_name(kobj), tmp);
+
 		pr_err("kobject '%s' (%p): tried to add an uninitialized object, something is seriously wrong.\n",
-		       kobject_name(kobj), kobj);
+			ret ? " " : kobject_name(kobj), kobj);
 		dump_stack();
 		return -EINVAL;
 	}
@@ -606,10 +610,14 @@ EXPORT_SYMBOL(kobject_del);
 struct kobject *kobject_get(struct kobject *kobj)
 {
 	if (kobj) {
-		if (!kobj->state_initialized)
+		if (!kobj->state_initialized) {
+			char tmp;
+			int ret = probe_kernel_address(kobject_name(kobj), tmp);
+
 			WARN(1, KERN_WARNING
 				"kobject: '%s' (%p): is not initialized, yet kobject_get() is being called.\n",
-			     kobject_name(kobj), kobj);
+			     ret ? " " : kobject_name(kobj), kobj);
+		}
 		kref_get(&kobj->kref);
 	}
 	return kobj;
@@ -701,10 +709,14 @@ static void kobject_release(struct kref *kref)
 void kobject_put(struct kobject *kobj)
 {
 	if (kobj) {
-		if (!kobj->state_initialized)
+		if (!kobj->state_initialized) {
+			char tmp;
+			int ret = probe_kernel_address(kobject_name(kobj), tmp);
+
 			WARN(1, KERN_WARNING
 				"kobject: '%s' (%p): is not initialized, yet kobject_put() is being called.\n",
-			     kobject_name(kobj), kobj);
+			     ret ? " " : kobject_name(kobj), kobj);
+		}
 		kref_put(&kobj->kref, kobject_release);
 	}
 }
-- 
2.17.1

Re: [PATCH] kobject: Access kobject name with caution if state is not initialized

[Greg Kroah-Hartman]

On Mon, Aug 20, 2018 at 10:39:47PM +0530, Srikar Dronamraju wrote:
> If kobject state is not initialized, then its not even certain that
> kobject'name is initialized. Hence when accessing the kobject's name
> tread carefully.
> 
> A stupid module test like
> https://github.com/srikard/tests/blob/master/modules/kobject_test.c
> can panic the system.

Lots of stupid modules can do dumb things.  Just don't do that.  The
kernel is not built to keep you from doing stupid things in kernel code
:)

So I fail to see why this patch is needed.  What in-kernel code path is
trying to print a kobject's name before it is initialized?  Why not fix
that obvious bug instead of forcing the kernel core to protect from
stupid code?

thanks,

greg k-h

Re: [PATCH] kobject: Access kobject name with caution if state is not initialized

[Dmitry Torokhov]

On Mon, Aug 20, 2018 at 12:22 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 20, 2018 at 10:39:47PM +0530, Srikar Dronamraju wrote:
> > If kobject state is not initialized, then its not even certain that
> > kobject'name is initialized. Hence when accessing the kobject's name
> > tread carefully.
> >
> > A stupid module test like
> > https://github.com/srikard/tests/blob/master/modules/kobject_test.c
> > can panic the system.
>
> Lots of stupid modules can do dumb things.  Just don't do that.  The
> kernel is not built to keep you from doing stupid things in kernel code
> :)
>
> So I fail to see why this patch is needed.  What in-kernel code path is
> trying to print a kobject's name before it is initialized?  Why not fix
> that obvious bug instead of forcing the kernel core to protect from
> stupid code?

Kay decided to add some guards in:

commit 0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806
Author: Kay Sievers <kay.sievers@vrfy.org>
Date:   Wed Dec 19 01:40:42 2007 +0100

   Kobject: auto-cleanup on final unref
...

+       if (!kobj->state_initialized) {
+               printk(KERN_ERR "kobject '%s' (%p): tried to add an "
+                      "uninitialized object, something is seriously wrong.\n",
+                      kobject_name(kobj), kobj);
+               dump_stack();
+               return -EINVAL;

Given that we have dump_stack() we can probably simply drop
kobject_name(kobj) instead of building even more elaborate checks. Or
just drop the whole check. Adding kobjects is somewhat uncommon
operation, plus "gabage in, garbage out".

Thanks.

-- 
Dmitry

Re: [PATCH] kobject: Access kobject name with caution if state is not initialized

[Srikar Dronamraju]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [2018-08-20 21:22:47]:

> On Mon, Aug 20, 2018 at 10:39:47PM +0530, Srikar Dronamraju wrote:
> > A stupid module test like
> > https://github.com/srikard/tests/blob/master/modules/kobject_test.c
> > can panic the system.
> 
> Lots of stupid modules can do dumb things.  Just don't do that.  The
> kernel is not built to keep you from doing stupid things in kernel code
> :)
> 

Completely agree. kernel/module code is not for doing stupid things.
However we seem to be hitting this once in a while in a weird case with
a slightly older kernel with no out of tree modules.

crash> bt
PID: 54813  TASK: c000000c4c76c160  CPU: 40  COMMAND: "lvm"
 #0 [c00000004ac0eb50] crash_kexec at c0000000001a1be4
 #1 [c00000004ac0eb80] die at c000000000025668
 #2 [c00000004ac0ec20] bad_page_fault at c00000000005d7a0
 #3 [c00000004ac0ec90] handle_page_fault at c000000000009608
 Data Access [300] exception frame:
 R0:  c000000000521cb4    R1:  c00000004ac0ef80    R2:  c000000001274800   
 R3:  0000000200000000    R4:  ffffffffffffffff    R5:  0000000200000000   
 R6:  0000000000000000    R7:  ffffffffffffffff    R8:  ffffffffffffffff   
 R9:  0000000000000004    R10: c000000000521c90    R11: c000000001434800   
 R12: c0000000000e3750    R13: c000000007b16800    R14: 00003fff85970520   
 R15: 00003fffd442c6c0    R16: 0000000000000000    R17: 000000000000005c   
 R18: 00003fff859797c8    R19: 00003fff859a28f0    R20: 00000100333c2400   
 R21: 00000100333c23d0    R22: 0000000000000025    R23: c00000000143a648   
 R24: 00000000000003e0    R25: 0000000000000020    R26: c00000004ac0f140   
 R27: 0000000000000000    R28: 0000000200000000    R29: ffffffffffffffff   
 R30: c00000000143aa28    R31: c00000000143a654   
 NIP: c00000000051bda8    MSR: 8000000100009033    OR3: c0000000000093ec
 CTR: c000000000521c90    LR:  c00000000051e7b8    XER: 0000000000000000
 CCR: 0000000088048444    MQ:  0000000000000000    DAR: 0000000200000000
 DSISR: 0000000040000000     Syscall Result: 0000000000000000
 #4 [c00000004ac0ef80] strnlen at c00000000051bda8
 [Link Register] [c00000004ac0ef80] string at c00000000051e7b8
 #5 [c00000004ac0efd0] vsnprintf at c000000000521cb4
 #6 [c00000004ac0f050] vscnprintf at c0000000005226e0
 #7 [c00000004ac0f080] vprintk_default at c0000000000e381c
 #8 [c00000004ac0f0f0] printk at c000000000a2047c
 #9 [c00000004ac0f110] kobject_put at c000000000510c34
#10 [c00000004ac0f1a0] of_node_put at c000000000813034
#11 [c00000004ac0f1c0] pci_release_of_node at c0000000005ae724
#12 [c00000004ac0f1f0] pci_release_dev at c00000000056ff60
#13 [c00000004ac0f220] device_release at c00000000065a468
#14 [c00000004ac0f2a0] kobject_put at c000000000510abc
#15 [c00000004ac0f330] put_device at c00000000065aaf4
#16 [c00000004ac0f350] scsi_host_dev_release at c0000000006a2f10
#17 [c00000004ac0f390] device_release at c00000000065a468
#18 [c00000004ac0f410] kobject_put at c000000000510abc
#19 [c00000004ac0f4a0] put_device at c00000000065aaf4
#20 [c00000004ac0f4c0] scsi_target_dev_release at c0000000006b2bb4
#21 [c00000004ac0f4f0] device_release at c00000000065a468
#22 [c00000004ac0f570] kobject_put at c000000000510abc
#23 [c00000004ac0f600] put_device at c00000000065aaf4
#24 [c00000004ac0f620] scsi_device_dev_release_usercontext at c0000000006b8790
#25 [c00000004ac0f670] execute_in_process_context at c0000000001185b4
#26 [c00000004ac0f6a0] scsi_device_dev_release at c0000000006b8644
#27 [c00000004ac0f6c0] device_release at c00000000065a468
#28 [c00000004ac0f740] kobject_put at c000000000510abc
#29 [c00000004ac0f7d0] put_device at c00000000065aaf4
#30 [c00000004ac0f7f0] scsi_device_put at c00000000069ff98
#31 [c00000004ac0f820] sd_release at d000000008013f4c [sd_mod]
#32 [c00000004ac0f8a0] __blkdev_put at c0000000003a7318
#33 [c00000004ac0f900] dm_put_table_device at d000000006ed09fc [dm_mod]
#34 [c00000004ac0f940] dm_put_device at d000000006ed663c [dm_mod]
#35 [c00000004ac0f9b0] linear_dtr at d000000006eda2d4 [dm_mod]
#36 [c00000004ac0f9e0] dm_table_destroy at d000000006ed7380 [dm_mod]
#37 [c00000004ac0fa70] dev_suspend at d000000006edf114 [dm_mod]
#38 [c00000004ac0faf0] ctl_ioctl at d000000006edc2e0 [dm_mod]
#39 [c00000004ac0fcf0] dm_ctl_ioctl at d000000006edc548 [dm_mod]
#40 [c00000004ac0fd10] do_vfs_ioctl at c00000000035a9a8
#41 [c00000004ac0fdd0] sys_ioctl at c00000000035aea4
#42 [c00000004ac0fe30] system_call at c00000000000a284
 System Call [c00] exception frame:
 R0:  0000000000000036    R1:  00003fffd442c4b0    R2:  00003fff858a7400   
 R3:  0000000000000006    R4:  00000000c138fd06    R5:  00000100333c23d0   
 R6:  0000000000000004    R7:  00003fff8597a2f0    R8:  0000000000000006   
 R9:  0000000000000000    R10: 0000000000000000    R11: 0000000000000000   
 R12: 0000000000000000    R13: 00003fff85b42ff0    R14: 00003fff85970520   
 R15: 00003fffd442c6c0    R16: 0000000000000000    R17: 000000000000005c   
 R18: 00003fff859797c8    R19: 00003fff859a28f0    R20: 00000100333c2400   
 R21: 00000100333c23d0    R22: 00003fff859a40e8    R23: 00003fff85970520   
 R24: 00003fff85970520    R25: 00003fff85970520    R26: 0000000000000007   
 R27: 00000100333c2480    R28: 00003fff85970520    R29: 0000000106da8290   
 R30: 00003fff85970520    R31: 00000100333bdb00   
 NIP: 00003fff857d9ecc    MSR: 800000010000d033    OR3: 0000000000000006
 CTR: 0000000000000000    LR:  00003fff8596dc08    XER: 0000000000000000
 CCR: 0000000044022444    MQ:  0000000000000001    DAR: 0000010033dae5d0
 DSISR: 0000000042000000     Syscall Result: 0000000000000004


Since this happens once in a bluemoon, there is no point in trying with
a newer/later kernel.

> So I fail to see why this patch is needed.  What in-kernel code path is
> trying to print a kobject's name before it is initialized?  Why not fix
> that obvious bug instead of forcing the kernel core to protect from
> stupid code?

By the time it has crashed, we don't have a clue on who created the
kobject, which thread tried to delete the object.

To see if two threads were racing, I looked at stack traces from other
threads but no other thread is in kobject_* calls. So there is a genuine
bug in a genuine module/kernel. The point that kobject state in
uninitialized itself means that the call was unexpected. So the question
now is should we detect and abort gracefully or should we allow it to
panic. By aborting, we are not masking the problem.

-- 
Thanks and Regards
Srikar

Re: [PATCH] kobject: Access kobject name with caution if state is not initialized

[Srikar Dronamraju]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [2018-08-20 12:33:50]:

> On Mon, Aug 20, 2018 at 12:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Aug 20, 2018 at 10:39:47PM +0530, Srikar Dronamraju wrote:
> > Lots of stupid modules can do dumb things.  Just don't do that.  The
> > kernel is not built to keep you from doing stupid things in kernel code
> > :)
> >
> > So I fail to see why this patch is needed.  What in-kernel code path is
> > trying to print a kobject's name before it is initialized?  Why not fix
> > that obvious bug instead of forcing the kernel core to protect from
> > stupid code?
> 
> Kay decided to add some guards in:
> 
> commit 0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806
> Author: Kay Sievers <kay.sievers@vrfy.org>
> Date:   Wed Dec 19 01:40:42 2007 +0100
> 
>    Kobject: auto-cleanup on final unref
> ...
> 
> +       if (!kobj->state_initialized) {
> +               printk(KERN_ERR "kobject '%s' (%p): tried to add an "
> +                      "uninitialized object, something is seriously wrong.\n",
> +                      kobject_name(kobj), kobj);
> +               dump_stack();
> +               return -EINVAL;
> 
> Given that we have dump_stack() we can probably simply drop
> kobject_name(kobj) instead of building even more elaborate checks. Or
> just drop the whole check. Adding kobjects is somewhat uncommon
> operation, plus "gabage in, garbage out".
> 

Dropping the kobject_name(kobj) should also be okay.