| Message ID | 20221209062919.1096779-1-mcgrof@kernel.org |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/acpi: fix null dereference on probe for missing ACPI_COMPANION() | expand |
On Thu, 08 Dec 2022, Luis Chamberlain wrote: >Simply loading cxl_test ends up triggering a null pointer dereference >on next-20221207, and it happens because the fetched ACPI_COMPANION() can >end up not correct / missing. As with other code which uses ACPI_COMPANION() >(drivers/acpi/device_pm.c comes to mind) be defensive over the assumption >the companion is always present and bail right away. > >This can be easily reproduced with kdevops [0] with linux next-20221207 [1] >and cxl enabled workflows: > >make menuconfig # enable cxl and linux-next >make # sets up variables, builds qemu from source >make linux # builds and install next-20221207 >make cxl # installs cxl tool >make cxl-test-probe # loads cxl_test > >The oops: > > # modprobe cxl_test >No TPM handle discovered. >failed to open file /etc/ndctl/keys/nvdimm-master.blob: No such file or directory > >[0] https://github.com/linux-kdevops/kdevops >[1] https://github.com/linux-kdevops/kdevops/blob/master/playbooks/roles/bootlinux/templates/config-next-20221207 > >cxl_mock: loading out-of-tree module taints kernel. >cxl_mock: loading test module taints kernel. >cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform >cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform >cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform >cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform >cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform >cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform >cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform >cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform >cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform >cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform >cxl_mem mem10: CXL port topology not found >BUG: kernel NULL pointer dereference, address: 00000000000002c0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page >PGD 0 P4D 0 >Oops: 0000 [#1] PREEMPT SMP PTI >CPU: 4 PID: 1644 Comm: systemd-udevd Kdump: loaded Tainted: G O N 6.1.0-rc8-next-20221207 #5 >Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 >RIP: 0010:cxl_acpi_probe+0xeb/0x2f0 [cxl_acpi] >Code: ff ff ff 48 c7 40 08 ff ff ff ff 48 c7 40 18 00 02 00 00 e8 57 29 fd ff 49 89 c7 41 89 c4 48 3d 00 f0 ff ff 0f 87 73 ff ff ff <49> 8b bd c0 02 00 00 48 c7 c1 c0 64 e4 c0 48 89 c2 31 f6 e8 bd f1 >RSP: 0018:ffffbe6d008b7c30 EFLAGS: 00010287 >RAX: ffff97a7c6e01000 RBX: ffff97a7c51fd810 RCX: 0000000000000000 >RDX: 0000000000000001 RSI: 0000000000000282 RDI: 00000000ffffffff >RBP: 0000000000000000 R08: ffff97a7c51fdaa8 R09: 0000000000000010 >R10: 0000000000000002 R11: 00000000000013c7 R12: 00000000c6e01000 >R13: 0000000000000000 R14: ffff97a7d9c653a8 R15: ffff97a7c6e01000 >FS: 00007f34b038ed00(0000) GS:ffff97a83bd00000(0000) knlGS:0000000000000000 >CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >CR2: 00000000000002c0 CR3: 0000000102f7e005 CR4: 0000000000770ee0 >DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >PKRU: 55555554 >Call Trace: > <TASK> > ? kernfs_create_link+0x5d/0xa0 > platform_probe+0x41/0x90 > really_probe+0xdb/0x380 > ? pm_runtime_barrier+0x50/0x90 > __driver_probe_device+0x78/0x170 > driver_probe_device+0x1f/0x90 > __driver_attach+0xce/0x1c0 > ? __pfx___driver_attach+0x10/0x10 > bus_for_each_dev+0x73/0xc0 > bus_add_driver+0x1ae/0x200 > driver_register+0x89/0xe0 > ? __pfx_init_module+0x10/0x10 [cxl_acpi] > do_one_initcall+0x43/0x220 > ? kmalloc_trace+0x26/0x90 > do_init_module+0x4a/0x1f0 > __do_sys_init_module+0x17f/0x1b0 > do_syscall_64+0x37/0x90 > entry_SYSCALL_64_after_hwframe+0x72/0xdc >RIP: 0033:0x7f34b061baaa >Code: 48 8b 0d 59 83 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 26 83 0c 00 f7 d8 64 89 01 48 >RSP: 002b:00007fff6a198408 EFLAGS: 00000246 ORIG_RAX: 00000000000000af >RAX: ffffffffffffffda RBX: 00005635afc7e5e0 RCX: 00007f34b061baaa >RDX: 00007f34b07a5efd RSI: 0000000000060a29 RDI: 00005635afdd6510 >RBP: 00007f34b07a5efd R08: 000000000001f5b3 R09: 0000000000000000 >R10: 000000000000eb81 R11: 0000000000000246 R12: 00005635afdd6510 >R13: 0000000000000000 R14: 00005635afca6f40 R15: 00005635af874e50 > </TASK> >Modules linked in: cxl_acpi(+) cxl_pmem cxl_mem cxl_port cxl_mock_mem(ON) cxl_test(ON) cxl_mock(ON) cxl_core libnvdimm cbc encrypted_keys kvm_intel kvm 9p netfs irqbypass crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 sha512_generic aesni_intel crypto_simd cryptd cirrus drm_shmem_helper 9pnet_virtio virtio_balloon i6300esb drm_kms_helper joydev evdev button serio_raw drm configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 md_mod virtio_net net_failover virtio_blk failover psmouse virtio_pci virtio_pci_legacy_dev nvme virtio_pci_modern_dev crc32_pclmul nvme_core virtio crc32c_intel t10_pi virtio_ring crc64_rocksoft crc64 > >And gdb: > >(gdb) l *(cxl_acpi_probe+0xeb) >0xa8b is in cxl_acpi_probe (tools/testing/cxl/../../../drivers/cxl/acpi.c:648). >643 >644 root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); >645 if (IS_ERR(root_port)) >646 return PTR_ERR(root_port); >647 >648 rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, >649 add_host_bridge_dport); >650 if (rc < 0) >651 return rc; >652 > >Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
On 12/8/2022 11:29 PM, Luis Chamberlain wrote: > Simply loading cxl_test ends up triggering a null pointer dereference > on next-20221207, and it happens because the fetched ACPI_COMPANION() can > end up not correct / missing. As with other code which uses ACPI_COMPANION() > (drivers/acpi/device_pm.c comes to mind) be defensive over the assumption > the companion is always present and bail right away. > > This can be easily reproduced with kdevops [0] with linux next-20221207 [1] > and cxl enabled workflows: > > make menuconfig # enable cxl and linux-next > make # sets up variables, builds qemu from source > make linux # builds and install next-20221207 > make cxl # installs cxl tool > make cxl-test-probe # loads cxl_test > > The oops: > > # modprobe cxl_test > No TPM handle discovered. > failed to open file /etc/ndctl/keys/nvdimm-master.blob: No such file or directory > > [0] https://github.com/linux-kdevops/kdevops > [1] https://github.com/linux-kdevops/kdevops/blob/master/playbooks/roles/bootlinux/templates/config-next-20221207 > > cxl_mock: loading out-of-tree module taints kernel. > cxl_mock: loading test module taints kernel. > cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform > cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform > cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform > cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform > cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform > cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform > cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform > cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform > cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform > cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform > cxl_mem mem10: CXL port topology not found > BUG: kernel NULL pointer dereference, address: 00000000000002c0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP PTI > CPU: 4 PID: 1644 Comm: systemd-udevd Kdump: loaded Tainted: G O N 6.1.0-rc8-next-20221207 #5 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 > RIP: 0010:cxl_acpi_probe+0xeb/0x2f0 [cxl_acpi] > Code: ff ff ff 48 c7 40 08 ff ff ff ff 48 c7 40 18 00 02 00 00 e8 57 29 fd ff 49 89 c7 41 89 c4 48 3d 00 f0 ff ff 0f 87 73 ff ff ff <49> 8b bd c0 02 00 00 48 c7 c1 c0 64 e4 c0 48 89 c2 31 f6 e8 bd f1 > RSP: 0018:ffffbe6d008b7c30 EFLAGS: 00010287 > RAX: ffff97a7c6e01000 RBX: ffff97a7c51fd810 RCX: 0000000000000000 > RDX: 0000000000000001 RSI: 0000000000000282 RDI: 00000000ffffffff > RBP: 0000000000000000 R08: ffff97a7c51fdaa8 R09: 0000000000000010 > R10: 0000000000000002 R11: 00000000000013c7 R12: 00000000c6e01000 > R13: 0000000000000000 R14: ffff97a7d9c653a8 R15: ffff97a7c6e01000 > FS: 00007f34b038ed00(0000) GS:ffff97a83bd00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000000002c0 CR3: 0000000102f7e005 CR4: 0000000000770ee0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > PKRU: 55555554 > Call Trace: > <TASK> > ? kernfs_create_link+0x5d/0xa0 > platform_probe+0x41/0x90 > really_probe+0xdb/0x380 > ? pm_runtime_barrier+0x50/0x90 > __driver_probe_device+0x78/0x170 > driver_probe_device+0x1f/0x90 > __driver_attach+0xce/0x1c0 > ? __pfx___driver_attach+0x10/0x10 > bus_for_each_dev+0x73/0xc0 > bus_add_driver+0x1ae/0x200 > driver_register+0x89/0xe0 > ? __pfx_init_module+0x10/0x10 [cxl_acpi] > do_one_initcall+0x43/0x220 > ? kmalloc_trace+0x26/0x90 > do_init_module+0x4a/0x1f0 > __do_sys_init_module+0x17f/0x1b0 > do_syscall_64+0x37/0x90 > entry_SYSCALL_64_after_hwframe+0x72/0xdc > RIP: 0033:0x7f34b061baaa > Code: 48 8b 0d 59 83 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 26 83 0c 00 f7 d8 64 89 01 48 > RSP: 002b:00007fff6a198408 EFLAGS: 00000246 ORIG_RAX: 00000000000000af > RAX: ffffffffffffffda RBX: 00005635afc7e5e0 RCX: 00007f34b061baaa > RDX: 00007f34b07a5efd RSI: 0000000000060a29 RDI: 00005635afdd6510 > RBP: 00007f34b07a5efd R08: 000000000001f5b3 R09: 0000000000000000 > R10: 000000000000eb81 R11: 0000000000000246 R12: 00005635afdd6510 > R13: 0000000000000000 R14: 00005635afca6f40 R15: 00005635af874e50 > </TASK> > Modules linked in: cxl_acpi(+) cxl_pmem cxl_mem cxl_port cxl_mock_mem(ON) cxl_test(ON) cxl_mock(ON) cxl_core libnvdimm cbc encrypted_keys kvm_intel kvm 9p netfs irqbypass crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 sha512_generic aesni_intel crypto_simd cryptd cirrus drm_shmem_helper 9pnet_virtio virtio_balloon i6300esb drm_kms_helper joydev evdev button serio_raw drm configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 md_mod virtio_net net_failover virtio_blk failover psmouse virtio_pci virtio_pci_legacy_dev nvme virtio_pci_modern_dev crc32_pclmul nvme_core virtio crc32c_intel t10_pi virtio_ring crc64_rocksoft crc64 > > And gdb: > > (gdb) l *(cxl_acpi_probe+0xeb) > 0xa8b is in cxl_acpi_probe (tools/testing/cxl/../../../drivers/cxl/acpi.c:648). > 643 > 644 root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > 645 if (IS_ERR(root_port)) > 646 return PTR_ERR(root_port); > 647 > 648 rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, > 649 add_host_bridge_dport); > 650 if (rc < 0) > 651 return rc; > 652 > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > > Note: kdevops also suports now the target: > > make cxl-test-meson > > But that does not *at least* crash the kernel although the tests fail too... > This is likely a misconfiguration of some sort, but the same kernel > works fine when I enable a Type 3 memory device (also supported on > kdevops via CONFIG_QEMU_ENABLE_CXL_DEMO_TOPOLOGY_1). This test was run > without that enabled, so a naked cxl system. > > Even if it *was* a mis-configuration, such things should not crash the kernel. > > drivers/cxl/acpi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index ad0849af42d7..cf5d1a455efc 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -633,6 +633,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) > struct acpi_device *adev = ACPI_COMPANION(host); > struct cxl_cfmws_context ctx; > > + if (!adev) > + return -ENODEV; > + > device_lock_set_class(&pdev->dev, &cxl_root_key); > rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, > &pdev->dev);
Luis Chamberlain wrote: > Simply loading cxl_test ends up triggering a null pointer dereference > on next-20221207, and it happens because the fetched ACPI_COMPANION() can > end up not correct / missing. As with other code which uses ACPI_COMPANION() > (drivers/acpi/device_pm.c comes to mind) be defensive over the assumption > the companion is always present and bail right away. > > This can be easily reproduced with kdevops [0] with linux next-20221207 [1] > and cxl enabled workflows: > > make menuconfig # enable cxl and linux-next > make # sets up variables, builds qemu from source > make linux # builds and install next-20221207 > make cxl # installs cxl tool > make cxl-test-probe # loads cxl_test > > The oops: > > # modprobe cxl_test > No TPM handle discovered. > failed to open file /etc/ndctl/keys/nvdimm-master.blob: No such file or directory > > [0] https://github.com/linux-kdevops/kdevops > [1] https://github.com/linux-kdevops/kdevops/blob/master/playbooks/roles/bootlinux/templates/config-next-20221207 > > cxl_mock: loading out-of-tree module taints kernel. > cxl_mock: loading test module taints kernel. > cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform > cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform > cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform > cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform > cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform > cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform > cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform > cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform > cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform > cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform > cxl_mem mem10: CXL port topology not found > BUG: kernel NULL pointer dereference, address: 00000000000002c0 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP PTI > CPU: 4 PID: 1644 Comm: systemd-udevd Kdump: loaded Tainted: G O N 6.1.0-rc8-next-20221207 #5 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 > RIP: 0010:cxl_acpi_probe+0xeb/0x2f0 [cxl_acpi] > Code: ff ff ff 48 c7 40 08 ff ff ff ff 48 c7 40 18 00 02 00 00 e8 57 29 fd ff 49 89 c7 41 89 c4 48 3d 00 f0 ff ff 0f 87 73 ff ff ff <49> 8b bd c0 02 00 00 48 c7 c1 c0 64 e4 c0 48 89 c2 31 f6 e8 bd f1 > RSP: 0018:ffffbe6d008b7c30 EFLAGS: 00010287 > RAX: ffff97a7c6e01000 RBX: ffff97a7c51fd810 RCX: 0000000000000000 > RDX: 0000000000000001 RSI: 0000000000000282 RDI: 00000000ffffffff > RBP: 0000000000000000 R08: ffff97a7c51fdaa8 R09: 0000000000000010 > R10: 0000000000000002 R11: 00000000000013c7 R12: 00000000c6e01000 > R13: 0000000000000000 R14: ffff97a7d9c653a8 R15: ffff97a7c6e01000 > FS: 00007f34b038ed00(0000) GS:ffff97a83bd00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000000002c0 CR3: 0000000102f7e005 CR4: 0000000000770ee0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > PKRU: 55555554 > Call Trace: > <TASK> > ? kernfs_create_link+0x5d/0xa0 > platform_probe+0x41/0x90 > really_probe+0xdb/0x380 > ? pm_runtime_barrier+0x50/0x90 > __driver_probe_device+0x78/0x170 > driver_probe_device+0x1f/0x90 > __driver_attach+0xce/0x1c0 > ? __pfx___driver_attach+0x10/0x10 > bus_for_each_dev+0x73/0xc0 > bus_add_driver+0x1ae/0x200 > driver_register+0x89/0xe0 > ? __pfx_init_module+0x10/0x10 [cxl_acpi] > do_one_initcall+0x43/0x220 > ? kmalloc_trace+0x26/0x90 > do_init_module+0x4a/0x1f0 > __do_sys_init_module+0x17f/0x1b0 > do_syscall_64+0x37/0x90 > entry_SYSCALL_64_after_hwframe+0x72/0xdc > RIP: 0033:0x7f34b061baaa > Code: 48 8b 0d 59 83 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 26 83 0c 00 f7 d8 64 89 01 48 > RSP: 002b:00007fff6a198408 EFLAGS: 00000246 ORIG_RAX: 00000000000000af > RAX: ffffffffffffffda RBX: 00005635afc7e5e0 RCX: 00007f34b061baaa > RDX: 00007f34b07a5efd RSI: 0000000000060a29 RDI: 00005635afdd6510 > RBP: 00007f34b07a5efd R08: 000000000001f5b3 R09: 0000000000000000 > R10: 000000000000eb81 R11: 0000000000000246 R12: 00005635afdd6510 > R13: 0000000000000000 R14: 00005635afca6f40 R15: 00005635af874e50 > </TASK> > Modules linked in: cxl_acpi(+) cxl_pmem cxl_mem cxl_port cxl_mock_mem(ON) cxl_test(ON) cxl_mock(ON) cxl_core libnvdimm cbc encrypted_keys kvm_intel kvm 9p netfs irqbypass crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 sha512_generic aesni_intel crypto_simd cryptd cirrus drm_shmem_helper 9pnet_virtio virtio_balloon i6300esb drm_kms_helper joydev evdev button serio_raw drm configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 md_mod virtio_net net_failover virtio_blk failover psmouse virtio_pci virtio_pci_legacy_dev nvme virtio_pci_modern_dev crc32_pclmul nvme_core virtio crc32c_intel t10_pi virtio_ring crc64_rocksoft crc64 > > And gdb: > > (gdb) l *(cxl_acpi_probe+0xeb) > 0xa8b is in cxl_acpi_probe (tools/testing/cxl/../../../drivers/cxl/acpi.c:648). > 643 > 644 root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > 645 if (IS_ERR(root_port)) > 646 return PTR_ERR(root_port); > 647 > 648 rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, > 649 add_host_bridge_dport); > 650 if (rc < 0) > 651 return rc; > 652 > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > > Note: kdevops also suports now the target: > > make cxl-test-meson > > But that does not *at least* crash the kernel although the tests fail too... > This is likely a misconfiguration of some sort, but the same kernel > works fine when I enable a Type 3 memory device (also supported on > kdevops via CONFIG_QEMU_ENABLE_CXL_DEMO_TOPOLOGY_1). This test was run > without that enabled, so a naked cxl system. > > Even if it *was* a mis-configuration, such things should not crash the kernel. > > drivers/cxl/acpi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index ad0849af42d7..cf5d1a455efc 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -633,6 +633,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) > struct acpi_device *adev = ACPI_COMPANION(host); > struct cxl_cfmws_context ctx; > > + if (!adev) > + return -ENODEV; > + I can hear the static analysis bots sharpening their knives at the thought that ACPI platform device drivers need to check the result of ACPI_COMPANION(). This is clearly a cxl_test bug, not something the cxl_acpi driver should ever worry about. If ACPI_COMPANION() is failing for the mock platform device then there are bigger problems afoot and this is just a band-aid until the next failure. I'll try booting linux-next, because cxl_test is working for me just testing the tip of cxl.git/next.
Dan Williams wrote: > Luis Chamberlain wrote: > > Simply loading cxl_test ends up triggering a null pointer dereference > > on next-20221207, and it happens because the fetched ACPI_COMPANION() can > > end up not correct / missing. As with other code which uses ACPI_COMPANION() > > (drivers/acpi/device_pm.c comes to mind) be defensive over the assumption > > the companion is always present and bail right away. > > > > This can be easily reproduced with kdevops [0] with linux next-20221207 [1] > > and cxl enabled workflows: > > > > make menuconfig # enable cxl and linux-next > > make # sets up variables, builds qemu from source > > make linux # builds and install next-20221207 > > make cxl # installs cxl tool > > make cxl-test-probe # loads cxl_test > > > > The oops: > > > > # modprobe cxl_test > > No TPM handle discovered. > > failed to open file /etc/ndctl/keys/nvdimm-master.blob: No such file or directory > > > > [0] https://github.com/linux-kdevops/kdevops > > [1] https://github.com/linux-kdevops/kdevops/blob/master/playbooks/roles/bootlinux/templates/config-next-20221207 > > > > cxl_mock: loading out-of-tree module taints kernel. > > cxl_mock: loading test module taints kernel. > > cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform > > cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform > > cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform > > cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform > > cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform > > cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform > > cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform > > cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform > > cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform > > cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform > > cxl_mem mem10: CXL port topology not found > > BUG: kernel NULL pointer dereference, address: 00000000000002c0 > > #PF: supervisor read access in kernel mode > > #PF: error_code(0x0000) - not-present page > > PGD 0 P4D 0 > > Oops: 0000 [#1] PREEMPT SMP PTI > > CPU: 4 PID: 1644 Comm: systemd-udevd Kdump: loaded Tainted: G O N 6.1.0-rc8-next-20221207 #5 > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 > > RIP: 0010:cxl_acpi_probe+0xeb/0x2f0 [cxl_acpi] > > Code: ff ff ff 48 c7 40 08 ff ff ff ff 48 c7 40 18 00 02 00 00 e8 57 29 fd ff 49 89 c7 41 89 c4 48 3d 00 f0 ff ff 0f 87 73 ff ff ff <49> 8b bd c0 02 00 00 48 c7 c1 c0 64 e4 c0 48 89 c2 31 f6 e8 bd f1 > > RSP: 0018:ffffbe6d008b7c30 EFLAGS: 00010287 > > RAX: ffff97a7c6e01000 RBX: ffff97a7c51fd810 RCX: 0000000000000000 > > RDX: 0000000000000001 RSI: 0000000000000282 RDI: 00000000ffffffff > > RBP: 0000000000000000 R08: ffff97a7c51fdaa8 R09: 0000000000000010 > > R10: 0000000000000002 R11: 00000000000013c7 R12: 00000000c6e01000 > > R13: 0000000000000000 R14: ffff97a7d9c653a8 R15: ffff97a7c6e01000 > > FS: 00007f34b038ed00(0000) GS:ffff97a83bd00000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00000000000002c0 CR3: 0000000102f7e005 CR4: 0000000000770ee0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > PKRU: 55555554 > > Call Trace: > > <TASK> > > ? kernfs_create_link+0x5d/0xa0 > > platform_probe+0x41/0x90 > > really_probe+0xdb/0x380 > > ? pm_runtime_barrier+0x50/0x90 > > __driver_probe_device+0x78/0x170 > > driver_probe_device+0x1f/0x90 > > __driver_attach+0xce/0x1c0 > > ? __pfx___driver_attach+0x10/0x10 > > bus_for_each_dev+0x73/0xc0 > > bus_add_driver+0x1ae/0x200 > > driver_register+0x89/0xe0 > > ? __pfx_init_module+0x10/0x10 [cxl_acpi] > > do_one_initcall+0x43/0x220 > > ? kmalloc_trace+0x26/0x90 > > do_init_module+0x4a/0x1f0 > > __do_sys_init_module+0x17f/0x1b0 > > do_syscall_64+0x37/0x90 > > entry_SYSCALL_64_after_hwframe+0x72/0xdc > > RIP: 0033:0x7f34b061baaa > > Code: 48 8b 0d 59 83 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 26 83 0c 00 f7 d8 64 89 01 48 > > RSP: 002b:00007fff6a198408 EFLAGS: 00000246 ORIG_RAX: 00000000000000af > > RAX: ffffffffffffffda RBX: 00005635afc7e5e0 RCX: 00007f34b061baaa > > RDX: 00007f34b07a5efd RSI: 0000000000060a29 RDI: 00005635afdd6510 > > RBP: 00007f34b07a5efd R08: 000000000001f5b3 R09: 0000000000000000 > > R10: 000000000000eb81 R11: 0000000000000246 R12: 00005635afdd6510 > > R13: 0000000000000000 R14: 00005635afca6f40 R15: 00005635af874e50 > > </TASK> > > Modules linked in: cxl_acpi(+) cxl_pmem cxl_mem cxl_port cxl_mock_mem(ON) cxl_test(ON) cxl_mock(ON) cxl_core libnvdimm cbc encrypted_keys kvm_intel kvm 9p netfs irqbypass crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 sha512_generic aesni_intel crypto_simd cryptd cirrus drm_shmem_helper 9pnet_virtio virtio_balloon i6300esb drm_kms_helper joydev evdev button serio_raw drm configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 md_mod virtio_net net_failover virtio_blk failover psmouse virtio_pci virtio_pci_legacy_dev nvme virtio_pci_modern_dev crc32_pclmul nvme_core virtio crc32c_intel t10_pi virtio_ring crc64_rocksoft crc64 > > > > And gdb: > > > > (gdb) l *(cxl_acpi_probe+0xeb) > > 0xa8b is in cxl_acpi_probe (tools/testing/cxl/../../../drivers/cxl/acpi.c:648). > > 643 > > 644 root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > > 645 if (IS_ERR(root_port)) > > 646 return PTR_ERR(root_port); > > 647 > > 648 rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, > > 649 add_host_bridge_dport); > > 650 if (rc < 0) > > 651 return rc; > > 652 > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > > > Note: kdevops also suports now the target: > > > > make cxl-test-meson > > > > But that does not *at least* crash the kernel although the tests fail too... > > This is likely a misconfiguration of some sort, but the same kernel > > works fine when I enable a Type 3 memory device (also supported on > > kdevops via CONFIG_QEMU_ENABLE_CXL_DEMO_TOPOLOGY_1). This test was run > > without that enabled, so a naked cxl system. > > > > Even if it *was* a mis-configuration, such things should not crash the kernel. > > > > drivers/cxl/acpi.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index ad0849af42d7..cf5d1a455efc 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -633,6 +633,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) > > struct acpi_device *adev = ACPI_COMPANION(host); > > struct cxl_cfmws_context ctx; > > > > + if (!adev) > > + return -ENODEV; > > + > > I can hear the static analysis bots sharpening their knives at the > thought that ACPI platform device drivers need to check the result of > ACPI_COMPANION(). This is clearly a cxl_test bug, not something the > cxl_acpi driver should ever worry about. If ACPI_COMPANION() is failing > for the mock platform device then there are bigger problems afoot and > this is just a band-aid until the next failure. > > I'll try booting linux-next, because cxl_test is working for me just > testing the tip of cxl.git/next. Ok, my 6.1.0-rc8-next-20221208 build passed. # meson test -C build --suite cxl ninja: Entering directory `/root/git/ndctl/build' [109/109] Linking target ndctl/ndctl 1/5 ndctl:cxl / cxl-topology.sh OK 11.84s 2/5 ndctl:cxl / cxl-region-sysfs.sh OK 6.82s 3/5 ndctl:cxl / cxl-labels.sh OK 10.14s 4/5 ndctl:cxl / cxl-create-region.sh OK 18.32s 5/5 ndctl:cxl / security-cxl.sh OK 3.35s So, what I suspect is happening is that the kdevops environment is permitting the "production" version of cxl_acpi.ko to load rather than the test module instrumented to make ACPI_COMPANION() work with mock devices. I.e. note that tools/testing/cxl/Kbuild has: ldflags-y += --wrap=is_acpi_device_node ...which means if cxl_acpi is the production version it will call the real is_acpi_device_node() and result in the crash signature you see. Let's just preclude that from happening. With the patch below if I force install the production modules: # insmod /lib/modules/6.1.0-rc8-next-20221208+/kernel/drivers/cxl/core/cxl_core.ko # insmod /lib/modules/6.1.0-rc8-next-20221208+/kernel/drivers/cxl/cxl_acpi.ko ...and then try to load cxl_test I get: # modprobe cxl_test modprobe: ERROR: could not insert 'cxl_test': Unknown symbol in module, or unknown parameter (see dmesg) ...where dmesg has: cxl_test: Unknown symbol cxl_acpi_test (err -2) cxl_test: Unknown symbol cxl_core_test (err -2) -- >8 -- From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams@intel.com> Date: Fri, 9 Dec 2022 13:04:26 -0800 Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production modules The cxl_test machinery builds modified versions of the modules in drivers/cxl/ and intercepts some of their calls to allow cxl_test to inject mock CXL topologies for test. However, if cxl_test attempts the same with production modules, fireworks ensue as Luis discovered [1]. Prevent that scenario by arranging for cxl_test to check for a "watermark" symbol in each of the modules it expects to be modified before the test can run. This turns undefined runtime behavior or crashes into a safer failure to load the cxl_test module. Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1] Reported-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- tools/testing/cxl/Kbuild | 6 ++++++ tools/testing/cxl/cxl_acpi_test.c | 6 ++++++ tools/testing/cxl/cxl_core_test.c | 6 ++++++ tools/testing/cxl/cxl_mem_test.c | 6 ++++++ tools/testing/cxl/cxl_pmem_test.c | 6 ++++++ tools/testing/cxl/cxl_port_test.c | 6 ++++++ tools/testing/cxl/test/cxl.c | 8 ++++++++ tools/testing/cxl/watermark.h | 25 +++++++++++++++++++++++++ 8 files changed, 69 insertions(+) create mode 100644 tools/testing/cxl/cxl_acpi_test.c create mode 100644 tools/testing/cxl/cxl_core_test.c create mode 100644 tools/testing/cxl/cxl_mem_test.c create mode 100644 tools/testing/cxl/cxl_pmem_test.c create mode 100644 tools/testing/cxl/cxl_port_test.c create mode 100644 tools/testing/cxl/watermark.h diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild index 0805f08af8b3..427174feeb7d 100644 --- a/tools/testing/cxl/Kbuild +++ b/tools/testing/cxl/Kbuild @@ -23,22 +23,27 @@ obj-m += cxl_acpi.o cxl_acpi-y := $(CXL_SRC)/acpi.o cxl_acpi-y += mock_acpi.o cxl_acpi-y += config_check.o +cxl_acpi-y += cxl_acpi_test.o obj-m += cxl_pmem.o cxl_pmem-y := $(CXL_SRC)/pmem.o cxl_pmem-y += $(CXL_SRC)/security.o cxl_pmem-y += config_check.o +cxl_pmem-y += cxl_pmem_test.o obj-m += cxl_port.o cxl_port-y := $(CXL_SRC)/port.o cxl_port-y += config_check.o +cxl_port-y += cxl_port_test.o + obj-m += cxl_mem.o cxl_mem-y := $(CXL_SRC)/mem.o cxl_mem-y += config_check.o +cxl_mem-y += cxl_mem_test.o obj-m += cxl_core.o @@ -51,5 +56,6 @@ cxl_core-y += $(CXL_CORE_SRC)/pci.o cxl_core-y += $(CXL_CORE_SRC)/hdm.o cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o cxl_core-y += config_check.o +cxl_core-y += cxl_core_test.o obj-m += test/ diff --git a/tools/testing/cxl/cxl_acpi_test.c b/tools/testing/cxl/cxl_acpi_test.c new file mode 100644 index 000000000000..8602dc27c81c --- /dev/null +++ b/tools/testing/cxl/cxl_acpi_test.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ + +#include "watermark.h" + +cxl_test_watermark(cxl_acpi); diff --git a/tools/testing/cxl/cxl_core_test.c b/tools/testing/cxl/cxl_core_test.c new file mode 100644 index 000000000000..464a9255e4d6 --- /dev/null +++ b/tools/testing/cxl/cxl_core_test.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ + +#include "watermark.h" + +cxl_test_watermark(cxl_core); diff --git a/tools/testing/cxl/cxl_mem_test.c b/tools/testing/cxl/cxl_mem_test.c new file mode 100644 index 000000000000..ba7fb8a44288 --- /dev/null +++ b/tools/testing/cxl/cxl_mem_test.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ + +#include "watermark.h" + +cxl_test_watermark(cxl_mem); diff --git a/tools/testing/cxl/cxl_pmem_test.c b/tools/testing/cxl/cxl_pmem_test.c new file mode 100644 index 000000000000..3fd884fae537 --- /dev/null +++ b/tools/testing/cxl/cxl_pmem_test.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ + +#include "watermark.h" + +cxl_test_watermark(cxl_pmem); diff --git a/tools/testing/cxl/cxl_port_test.c b/tools/testing/cxl/cxl_port_test.c new file mode 100644 index 000000000000..be183917a9f6 --- /dev/null +++ b/tools/testing/cxl/cxl_port_test.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ + +#include "watermark.h" + +cxl_test_watermark(cxl_port); diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c index 30ee680d38ff..920bd969c554 100644 --- a/tools/testing/cxl/test/cxl.c +++ b/tools/testing/cxl/test/cxl.c @@ -9,6 +9,8 @@ #include <linux/pci.h> #include <linux/mm.h> #include <cxlmem.h> + +#include "../watermark.h" #include "mock.h" static int interleave_arithmetic; @@ -1119,6 +1121,12 @@ static __init int cxl_test_init(void) { int rc, i; + cxl_acpi_test(); + cxl_core_test(); + cxl_mem_test(); + cxl_pmem_test(); + cxl_port_test(); + register_cxl_mock_ops(&cxl_mock_ops); cxl_mock_pool = gen_pool_create(ilog2(SZ_2M), NUMA_NO_NODE); diff --git a/tools/testing/cxl/watermark.h b/tools/testing/cxl/watermark.h new file mode 100644 index 000000000000..9d81d4a5f6be --- /dev/null +++ b/tools/testing/cxl/watermark.h @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */ +#ifndef _TEST_CXL_WATERMARK_H_ +#define _TEST_CXL_WATERMARK_H_ +#include <linux/module.h> +#include <linux/printk.h> + +int cxl_acpi_test(void); +int cxl_core_test(void); +int cxl_mem_test(void); +int cxl_pmem_test(void); +int cxl_port_test(void); + +/* + * dummy routine for cxl_test to validate it is linking to the properly + * mocked module and not the standard one from the base tree. + */ +#define cxl_test_watermark(x) \ +int x##_test(void) \ +{ \ + pr_debug("%s for cxl_test\n", KBUILD_MODNAME); \ + return 0; \ +} \ +EXPORT_SYMBOL(x##_test) +#endif /* _TEST_CXL_WATERMARK_H_ */
On Fri, Dec 09, 2022 at 01:07:53PM -0800, Dan Williams wrote: > Dan Williams wrote: > > Luis Chamberlain wrote: > > > Simply loading cxl_test ends up triggering a null pointer dereference > > > on next-20221207, > > Ok, my 6.1.0-rc8-next-20221208 build passed. > > # meson test -C build --suite cxl > ninja: Entering directory `/root/git/ndctl/build' > [109/109] Linking target ndctl/ndctl > 1/5 ndctl:cxl / cxl-topology.sh OK 11.84s > 2/5 ndctl:cxl / cxl-region-sysfs.sh OK 6.82s > 3/5 ndctl:cxl / cxl-labels.sh OK 10.14s > 4/5 ndctl:cxl / cxl-create-region.sh OK 18.32s > 5/5 ndctl:cxl / security-cxl.sh OK 3.35s What branch of ndctl do you use? > So, what I suspect is happening is <-- snip --> Yes you're right. > From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001 > From: Dan Williams <dan.j.williams@intel.com> > Date: Fri, 9 Dec 2022 13:04:26 -0800 > Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production > modules > > The cxl_test machinery builds modified versions of the modules in > drivers/cxl/ and intercepts some of their calls to allow cxl_test to > inject mock CXL topologies for test. > > However, if cxl_test attempts the same with production modules, > fireworks ensue as Luis discovered [1]. Prevent that scenario by > arranging for cxl_test to check for a "watermark" symbol in each of the > modules it expects to be modified before the test can run. This turns > undefined runtime behavior or crashes into a safer failure to load the > cxl_test module. > > Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1] > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Indeed that fixes the same crash. However that highlights a few other issues. 1) ndcl unit tests still fail once you do get the right driver loaded: a) pending branch fails on the first test and stops there b) main branch fails at the first test and continues and passes on the rest of the tests. What is with the discrepancy? 2) The instructions on ndctl to use an external module are easily misguiding folks on how to use external replacement modules, I'd like to suggest a fix below. Details below. 1a) The tests still fail, worse on ndctl pending branch, it fails on the first test and just doesn't continue: vagrant@cxl /data/ndctl (git::pending)$ sudo meson test -C build --suite cxl ninja: no work to do. ninja: Entering directory `/data/ndctl/build' [1/50] Generating version.h with a custom command 1/1 ndctl:cxl / cxl-topology.sh FAIL 0.87s exit status 1 >>> DATA_PATH=/data/ndctl/test MALLOC_PERTURB_=132 >>> DAXCTL=/data/ndctl/build/daxctl/daxctl >>> TEST_PATH=/data/ndctl/build/test NDCTL=/data/ndctl/build/ndctl/ndctl >>> /bin/bash /data/ndctl/test/cxl-topology.sh Ok: 0 Expected Fail: 0 Fail: 1 Unexpected Pass: 0 Skipped: 0 Timeout: 0 Full log written to /data/ndctl/build/meson-logs/testlog.txt The full log: https://gist.github.com/mcgrof/1237a31821331d503615e136e18ff5f9 1b) Using the main branch, it fails also on topology but continues and passes the other tests: vagrant@cxl /data/ndctl (git::main)$ sudo meson test -C build --suite cxl ninja: no work to do. ninja: Entering directory `/data/ndctl/build' [1/51] Generating version.h with a custom command 1/4 ndctl:cxl / cxl-topology.sh FAIL 0.96s exit status 1 >>> TEST_PATH=/data/ndctl/build/test >>> DAXCTL=/data/ndctl/build/daxctl/daxctl DATA_PATH=/data/ndctl/test >>> NDCTL=/data/ndctl/build/ndctl/ndctl MALLOC_PERTURB_=123 /bin/bash >>> /data/ndctl/test/cxl-topology.sh 2/4 ndctl:cxl / cxl-region-sysfs.sh OK 5.33s 3/4 ndctl:cxl / cxl-labels.sh OK 3.40s 4/4 ndctl:cxl / cxl-create-region.sh OK 8.14s Ok: 3 Expected Fail: 0 Fail: 1 Unexpected Pass: 0 Skipped: 0 Timeout: 0 Full log written to /data/ndctl/build/meson-logs/testlog.txt The full log: https://gist.github.com/mcgrof/b1f7dd67fe44870adbd0c773ac752dcb The kernel log for the relevant topology failure for both cases on line 35: cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform cxl_mem mem10: CXL port topology not found platform cxl_host_bridge.0: host supports CXL platform cxl_host_bridge.1: host supports CXL platform cxl_host_bridge.2: host supports CXL platform cxl_host_bridge.3: host supports CXL (restricted) The test in this case is: # collect cxl_test root device id json=$($CXL list -b cxl_test) count=$(jq "length" <<< $json) ((count == 1)) || err "$LINENO" root=$(jq -r ".[] | .bus" <<< $json) # validate 2 host bridges under a root port port_sort="sort_by(.port | .[4:] | tonumber)" json=$($CXL list -b cxl_test -BP) count=$(jq ".[] | .[\"ports:$root\"] | length" <<< $json) ((count == 2)) || err "$LINENO" In my case root is "root0" and this count is 3. The output of $(cxl list -b cxl_test -BP) is: [ { "bus":"root0", "provider":"cxl_test", "ports:root0":[ { "port":"port1", "host":"cxl_host_bridge.0", "depth":1, "ports:port1":[ { "port":"port8", "host":"cxl_switch_uport.2", "depth":2 }, { "port":"port4", "host":"cxl_switch_uport.0", "depth":2 } ] }, { "port":"port3", "host":"cxl_host_bridge.2", "depth":1, "ports:port3":[ { "port":"port16", "host":"cxl_switch_uport.4", "depth":2 } ] }, { "port":"port2", "host":"cxl_host_bridge.1", "depth":1, "ports:port2":[ { "port":"port6", "host":"cxl_switch_uport.1", "depth":2 }, { "port":"port10", "host":"cxl_switch_uport.3", "depth":2 } ] } ] } ] Anyway I think we should simplify the ndctl README.md to just use INSTALL_MOD_DIR follows, thoughts? But this also raises the question of *if* using ndctl and linux-next shoudl one use the main branch or the pending branch? Can there be issues with synchronizing ? Or should the main branch always work, and the pending should just have the latest and greatest and *can* fail? If we wanted to automate nightly tests what should we use? From c97b0de23fa8b9cc8945e20cea134a61868ada40 Mon Sep 17 00:00:00 2001 From: Luis Chamberlain <mcgrof@kernel.org> Date: Tue, 13 Dec 2022 22:34:35 +0000 Subject: [PATCH] README.md: recommend to use INSTALL_MOD_DIR By default depmod will only look at the updates directory for modules, and so if one wants to replace production modules with alternatives one can just use INSTALL_MOD_DIR=updates at modules_install time. This will ensure that the correctly mocked modules are used, whether that is for cxl or libnvdimm. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- README.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index e5c4940..fd285af 100644 --- a/README.md +++ b/README.md @@ -74,16 +74,17 @@ loaded. To build and install nfit_test.ko: CONFIG_ENCRYPTED_KEYS=y ``` -1. Build and install the unit test enabled libnvdimm modules in the - following order. The unit test modules need to be in place prior to - the `depmod` that runs during the final `modules_install` +1. Build and install the unit test enabled libnvdimm modules ``` - make M=tools/testing/nvdimm - sudo make M=tools/testing/nvdimm modules_install sudo make modules_install + make M=tools/testing/nvdimm + sudo make M=tools/testing/nvdimm modules_install INSTALL_MOD_DIR=updates ``` +To uninstall and use the production nvdimm modules just rm -rf the updates +directory from the respective kernel /lib/modules/ directory and run depmod -a. + 1. Now run `meson test -C build` in the ndctl source directory, or `ndctl test`, if ndctl was built with `-Dtest=enabled` as a configuration option to meson.
Luis Chamberlain wrote: > On Fri, Dec 09, 2022 at 01:07:53PM -0800, Dan Williams wrote: > > Dan Williams wrote: > > > Luis Chamberlain wrote: > > > > Simply loading cxl_test ends up triggering a null pointer dereference > > > > on next-20221207, > > > > Ok, my 6.1.0-rc8-next-20221208 build passed. > > > > # meson test -C build --suite cxl > > ninja: Entering directory `/root/git/ndctl/build' > > [109/109] Linking target ndctl/ndctl > > 1/5 ndctl:cxl / cxl-topology.sh OK 11.84s > > 2/5 ndctl:cxl / cxl-region-sysfs.sh OK 6.82s > > 3/5 ndctl:cxl / cxl-labels.sh OK 10.14s > > 4/5 ndctl:cxl / cxl-create-region.sh OK 18.32s > > 5/5 ndctl:cxl / security-cxl.sh OK 3.35s > > What branch of ndctl do you use? Yeah, somewhat unfair of me to say "works for me" with patches that had not yet been posted to the list. That's fixed now with this posting: http://lore.kernel.org/r/167097752151.1189953.3189708700022130101.stgit@dwillia2-xfh.jf.intel.com ...and this one that is now on the pending branch: http://lore.kernel.org/r/167053487710.582963.17616889985000817682.stgit@dwillia2-xfh.jf.intel.com > > > So, what I suspect is happening is > > <-- snip --> > > Yes you're right. > > > From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001 > > From: Dan Williams <dan.j.williams@intel.com> > > Date: Fri, 9 Dec 2022 13:04:26 -0800 > > Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production > > modules > > > > The cxl_test machinery builds modified versions of the modules in > > drivers/cxl/ and intercepts some of their calls to allow cxl_test to > > inject mock CXL topologies for test. > > > > However, if cxl_test attempts the same with production modules, > > fireworks ensue as Luis discovered [1]. Prevent that scenario by > > arranging for cxl_test to check for a "watermark" symbol in each of the > > modules it expects to be modified before the test can run. This turns > > undefined runtime behavior or crashes into a safer failure to load the > > cxl_test module. > > > > Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1] > > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Indeed that fixes the same crash. However that highlights a few other > issues. > > 1) ndcl unit tests still fail once you do get the right driver loaded: > a) pending branch fails on the first test and stops there > b) main branch fails at the first test and continues and passes on > the rest of the tests. What is with the discrepancy? > > 2) The instructions on ndctl to use an external module are easily > misguiding folks on how to use external replacement modules, I'd like > to suggest a fix below. > > Details below. [..] > > Anyway I think we should simplify the ndctl README.md to just use > INSTALL_MOD_DIR follows, thoughts? Hmm, but scripts/Makefile.modinst already has this: INSTALL_MOD_DIR ?= extra ...are you not getting these modules deployed in the "$modules/extra" directory, or is your distro not priortizing modules in that directory over the others? Fedora seems to have this policy by default, but it seems at least Ubuntu does not. That's what led to the discussion of a modprobe "override" policy in the Troubleshooting section of the README. My concern with changing the INSTALL_MOD_PATH recommendation is whether having modules in "$modules/updates" is guaranteed to work in all distros. Otherwise we're just shuffling which distros need a custom override workaround. > But this also raises the question of *if* using ndctl and linux-next > shoudl one use the main branch or the pending branch? Can there be > issues with synchronizing ? Or should the main branch always work, > and the pending should just have the latest and greatest and *can* > fail? This falls into the category of a "good problem to have" in the sense that I never had to worry before about others wanting to reproduce unit test results this early in the dev cycle, so I appreciate the nudge here. As to what to do about it, I am open to suggestions. The typical flow has some lag between upstream tools/testing/cxl/ changes and when the corresponding ndctl/cxl changes land. This is because the tool enabling does not start until after it is clear that the kernel changes are going to land. After that it's another round of review to settle on the tool changes. The ndctl/pending branch should usually be up to date by the time -rc1 arrives. Is that sufficient?
On Tue, Dec 13, 2022 at 04:54:32PM -0800, Dan Williams wrote: > Luis Chamberlain wrote: > > On Fri, Dec 09, 2022 at 01:07:53PM -0800, Dan Williams wrote: > > > Dan Williams wrote: > > > > Luis Chamberlain wrote: > > > > > Simply loading cxl_test ends up triggering a null pointer dereference > > > > > on next-20221207, > > > > > > Ok, my 6.1.0-rc8-next-20221208 build passed. > > > > > > # meson test -C build --suite cxl > > > ninja: Entering directory `/root/git/ndctl/build' > > > [109/109] Linking target ndctl/ndctl > > > 1/5 ndctl:cxl / cxl-topology.sh OK 11.84s > > > 2/5 ndctl:cxl / cxl-region-sysfs.sh OK 6.82s > > > 3/5 ndctl:cxl / cxl-labels.sh OK 10.14s > > > 4/5 ndctl:cxl / cxl-create-region.sh OK 18.32s > > > 5/5 ndctl:cxl / security-cxl.sh OK 3.35s > > > > What branch of ndctl do you use? > > Yeah, somewhat unfair of me to say "works for me" with patches that had > not yet been posted to the list. That's fixed now with this posting: > > http://lore.kernel.org/r/167097752151.1189953.3189708700022130101.stgit@dwillia2-xfh.jf.intel.com I b4 am'd this after my branch reset to the latest pending today. > ...and this one that is now on the pending branch: > > http://lore.kernel.org/r/167053487710.582963.17616889985000817682.stgit@dwillia2-xfh.jf.intel.com Groovy thanks. Progress, but still fails now on the cxl-xor-region.sh line 29 https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493 > > > So, what I suspect is happening is > > > > <-- snip --> > > > > Yes you're right. > > > > > From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001 > > > From: Dan Williams <dan.j.williams@intel.com> > > > Date: Fri, 9 Dec 2022 13:04:26 -0800 > > > Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production > > > modules > > > > > > The cxl_test machinery builds modified versions of the modules in > > > drivers/cxl/ and intercepts some of their calls to allow cxl_test to > > > inject mock CXL topologies for test. > > > > > > However, if cxl_test attempts the same with production modules, > > > fireworks ensue as Luis discovered [1]. Prevent that scenario by > > > arranging for cxl_test to check for a "watermark" symbol in each of the > > > modules it expects to be modified before the test can run. This turns > > > undefined runtime behavior or crashes into a safer failure to load the > > > cxl_test module. > > > > > > Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1] > > > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > Indeed that fixes the same crash. However that highlights a few other > > issues. > > > > 1) ndcl unit tests still fail once you do get the right driver loaded: > > a) pending branch fails on the first test and stops there > > b) main branch fails at the first test and continues and passes on > > the rest of the tests. What is with the discrepancy? > > > > 2) The instructions on ndctl to use an external module are easily > > misguiding folks on how to use external replacement modules, I'd like > > to suggest a fix below. > > > > Details below. > [..] > > > > Anyway I think we should simplify the ndctl README.md to just use > > INSTALL_MOD_DIR follows, thoughts? > > Hmm, but scripts/Makefile.modinst already has this: > > INSTALL_MOD_DIR ?= extra > > ...are you not getting these modules deployed in the "$modules/extra" > directory, or is your distro not priortizing modules in that directory > over the others? Fedora seems to have this policy by default, but it > seems at least Ubuntu does not. That's what led to the discussion of a > modprobe "override" policy in the Troubleshooting section of the README. I figured this was the assumption behind made. I'll very well *aware* of this situation, and no, developers *should not* assume everyone pegs "extra" with for example a /etc/depmod.d/dist.conf with: search updates extra built-in weak-updates That is a distribution specific policy. The *updates* however *is* picked up by upstream kmod *by default*, and so stuffing things into extra just ensures ensures folks don't break things now by using extra... unless they are on these distros that do use that... But it was *only* redhat derivatives that used it. Fedora's cloud vagrant image *does* not use it. And no developer should assume this generally for all Linux distributions. The *real* sensible thing to do would be to be very explicit about this and have say: /etc/depmod.d/cxl-mock.conf search mock built-in *But* then that would mean ndctl would have to carry installing this *if* CXL tests are enabled. That also leave folks *not* using ndctl without any clue about this issue and so if we wanted to just be super lazy, we just use updates as that is built-in to the C code for depmod.c on kmod.git *ifff* the distribution lacked *any* depmod.d file: https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/tools/depmod.c#n867 This has been there since the start on kmods since 2012. It's good enough for me, as old tools likely would not be used for CXL capable systems. Except maybe Android. And so using "INSTALL_MOD_DIR=updates" seems better than having us get ndctl into the business of installing a /etc/depmod.d/cxl-mock.conf. This would also allow experimentations to see if the cxl_test mock driver strategy could be replaced by a proper upstream kernel sefltests driver. > My concern with changing the INSTALL_MOD_PATH recommendation is whether > having modules in "$modules/updates" is guaranteed to work in all > distros. Otherwise we're just shuffling which distros need a custom > override workaround. All Linux distributions without *any* depmod.d file will work that use kmod. Most Linux distributions has phased old module-utils for kmod a long time ago, 11 ago, the code for the old crap module-init-toolls hasn't been updated for 11 years and specifically mentioned being replaced with libkmod usage which is how kmod got started. https://git.kernel.org/pub/scm/utils/kernel/module-init-tools/module-init-tools.git/ This is enough justification for me to simply replace the upstream scripts/Makefile.modinst to default to updates instead of extra as well so I'll go ahead and just do that now and CC the list on the patch. > > But this also raises the question of *if* using ndctl and linux-next > > shoudl one use the main branch or the pending branch? Can there be > > issues with synchronizing ? Or should the main branch always work, > > and the pending should just have the latest and greatest and *can* > > fail? > > This falls into the category of a "good problem to have" in the sense > that I never had to worry before about others wanting to reproduce unit > test results this early in the dev cycle, so I appreciate the nudge > here. Oh groovy, glad this is a step forward then :) I know folks have been using run-qemu for a while and have mentioned how it is still a bit difficult to ramp up with CXL, and so the goal here was reduce barrier to entry even further by automating the hell out of the entire process with modern automation tooling. Since kdevops did that well for complex testing such as filesystem testing and block layer testing I figured it'd be a good match to bring parity with CXL. So this also should now enable folks who want to say, run tests with qemu specific topologies too. It'd just be another guest on when spawing, and so tests can be run in parallel for each different topology. > As to what to do about it, I am open to suggestions. The typical flow > has some lag between upstream tools/testing/cxl/ changes and when the > corresponding ndctl/cxl changes land. This is because the tool enabling > does not start until after it is clear that the kernel changes are going > to land. After that it's another round of review to settle on the tool > changes. The ndctl/pending branch should usually be up to date by the > time -rc1 arrives. Is that sufficient? I think it would make sense to always assume that using the latest tooling will not break anything and we should then be able to always rely on it for all automationf or bleeding edge linux-next based testing. Tests however should then always look to see if a feature is available and gracefully skip if not available / supported by the target kernel being tested. This would then allow the *latest* ndctl and pending branch to be relied on to test also even older kernels. That way, new tests, but which can be useful for testing old kernels, can *also* be enabled to test things on those old kernels. And so the latest testing userspace scripts should *always* work with *any* kernel. It should not assume you have the latest and greatest respective cxl_test driver. Is that something we can count on today? Then as a developer, one can assume / expect the latest and greatest both tooling / testing will work regardless of the kernel. That would mimic the experience with using tools/testing/selftests , even linux-next selftests are supposed to work on older kernels. The respective old selftest modules would just be used if they were enabled for the older kernel. Then, a few questions, but while we're at it: Do we have enough experience at this point to say what things should be tested with the pair cxl_test / ndctl test suite and say what things instead are probably better tested using qemu? Are there tests, part of ndctl test suite which likely would be useful for non cxl_test.ko usages, but rather run on spawned qemu / bare metal CXL enabled systems, so one can just hammer on them? Luis
Luis Chamberlain wrote: > On Tue, Dec 13, 2022 at 04:54:32PM -0800, Dan Williams wrote: > > Luis Chamberlain wrote: > > > On Fri, Dec 09, 2022 at 01:07:53PM -0800, Dan Williams wrote: > > > > Dan Williams wrote: > > > > > Luis Chamberlain wrote: > > > > > > Simply loading cxl_test ends up triggering a null pointer dereference > > > > > > on next-20221207, > > > > > > > > Ok, my 6.1.0-rc8-next-20221208 build passed. > > > > > > > > # meson test -C build --suite cxl > > > > ninja: Entering directory `/root/git/ndctl/build' > > > > [109/109] Linking target ndctl/ndctl > > > > 1/5 ndctl:cxl / cxl-topology.sh OK 11.84s > > > > 2/5 ndctl:cxl / cxl-region-sysfs.sh OK 6.82s > > > > 3/5 ndctl:cxl / cxl-labels.sh OK 10.14s > > > > 4/5 ndctl:cxl / cxl-create-region.sh OK 18.32s > > > > 5/5 ndctl:cxl / security-cxl.sh OK 3.35s > > > > > > What branch of ndctl do you use? > > > > Yeah, somewhat unfair of me to say "works for me" with patches that had > > not yet been posted to the list. That's fixed now with this posting: > > > > http://lore.kernel.org/r/167097752151.1189953.3189708700022130101.stgit@dwillia2-xfh.jf.intel.com > > I b4 am'd this after my branch reset to the latest pending today. > > > ...and this one that is now on the pending branch: > > > > http://lore.kernel.org/r/167053487710.582963.17616889985000817682.stgit@dwillia2-xfh.jf.intel.com > > Groovy thanks. Progress, but still fails now on the cxl-xor-region.sh line 29 > > https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493 This is also good news in that we are finding integration issues early as I did not have this in my local tree previously. Will take a look, although I think Alison and Vishal might beat me to it. > > > > > So, what I suspect is happening is > > > > > > <-- snip --> > > > > > > Yes you're right. > > > > > > > From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001 > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > Date: Fri, 9 Dec 2022 13:04:26 -0800 > > > > Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production > > > > modules > > > > > > > > The cxl_test machinery builds modified versions of the modules in > > > > drivers/cxl/ and intercepts some of their calls to allow cxl_test to > > > > inject mock CXL topologies for test. > > > > > > > > However, if cxl_test attempts the same with production modules, > > > > fireworks ensue as Luis discovered [1]. Prevent that scenario by > > > > arranging for cxl_test to check for a "watermark" symbol in each of the > > > > modules it expects to be modified before the test can run. This turns > > > > undefined runtime behavior or crashes into a safer failure to load the > > > > cxl_test module. > > > > > > > > Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1] > > > > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > > Indeed that fixes the same crash. However that highlights a few other > > > issues. > > > > > > 1) ndcl unit tests still fail once you do get the right driver loaded: > > > a) pending branch fails on the first test and stops there > > > b) main branch fails at the first test and continues and passes on > > > the rest of the tests. What is with the discrepancy? > > > > > > 2) The instructions on ndctl to use an external module are easily > > > misguiding folks on how to use external replacement modules, I'd like > > > to suggest a fix below. > > > > > > Details below. > > [..] > > > > > > Anyway I think we should simplify the ndctl README.md to just use > > > INSTALL_MOD_DIR follows, thoughts? > > > > Hmm, but scripts/Makefile.modinst already has this: > > > > INSTALL_MOD_DIR ?= extra > > > > ...are you not getting these modules deployed in the "$modules/extra" > > directory, or is your distro not priortizing modules in that directory > > over the others? Fedora seems to have this policy by default, but it > > seems at least Ubuntu does not. That's what led to the discussion of a > > modprobe "override" policy in the Troubleshooting section of the README. > > I figured this was the assumption behind made. I'll very well *aware* of > this situation, and no, developers *should not* assume everyone pegs > "extra" with for example a /etc/depmod.d/dist.conf with: > > search updates extra built-in weak-updates > > That is a distribution specific policy. The *updates* however *is* > picked up by upstream kmod *by default*, and so stuffing things into > extra just ensures ensures folks don't break things now by using extra... > unless they are on these distros that do use that... > > But it was *only* redhat derivatives that used it. Fedora's cloud > vagrant image *does* not use it. And no developer should assume this > generally for all Linux distributions. > > The *real* sensible thing to do would be to be very explicit about this > and have say: > > /etc/depmod.d/cxl-mock.conf > search mock built-in > > *But* then that would mean ndctl would have to carry installing this > *if* CXL tests are enabled. That also leave folks *not* using ndctl > without any clue about this issue and so if we wanted to just be super > lazy, we just use updates as that is built-in to the C code for depmod.c > on kmod.git *ifff* the distribution lacked *any* depmod.d file: > > https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/tools/depmod.c#n867 > > This has been there since the start on kmods since 2012. It's good > enough for me, as old tools likely would not be used for CXL capable > systems. Except maybe Android. > > And so using "INSTALL_MOD_DIR=updates" seems better than having us get > ndctl into the business of installing a /etc/depmod.d/cxl-mock.conf. Makes sense, and acked your change. > > This would also allow experimentations to see if the cxl_test mock > driver strategy could be replaced by a proper upstream kernel sefltests > driver. Yes, I learned about this approach from this article: https://lwn.net/Articles/558106/ ...but that pre-dated KUnit. However, KUnit does more traditional per function unit testing whereas nfit_test and cxl_test are quick and dirty emulation. They are more for testing kernel-user-ABI than kernel internals, but kernel-internal testing comes along for that ride as a side effect. > > > My concern with changing the INSTALL_MOD_PATH recommendation is whether > > having modules in "$modules/updates" is guaranteed to work in all > > distros. Otherwise we're just shuffling which distros need a custom > > override workaround. > > All Linux distributions without *any* depmod.d file will work that use kmod. > Most Linux distributions has phased old module-utils for kmod a long time ago, > 11 ago, the code for the old crap module-init-toolls hasn't been updated > for 11 years and specifically mentioned being replaced with libkmod > usage which is how kmod got started. > > https://git.kernel.org/pub/scm/utils/kernel/module-init-tools/module-init-tools.git/ > > This is enough justification for me to simply replace the upstream > scripts/Makefile.modinst to default to updates instead of extra as well > so I'll go ahead and just do that now and CC the list on the patch. Thanks for that. > > > > But this also raises the question of *if* using ndctl and linux-next > > > shoudl one use the main branch or the pending branch? Can there be > > > issues with synchronizing ? Or should the main branch always work, > > > and the pending should just have the latest and greatest and *can* > > > fail? > > > > This falls into the category of a "good problem to have" in the sense > > that I never had to worry before about others wanting to reproduce unit > > test results this early in the dev cycle, so I appreciate the nudge > > here. > > Oh groovy, glad this is a step forward then :) > > I know folks have been using run-qemu for a while and have mentioned how > it is still a bit difficult to ramp up with CXL, and so the goal here was > reduce barrier to entry even further by automating the hell out of the > entire process with modern automation tooling. Since kdevops did that > well for complex testing such as filesystem testing and block layer > testing I figured it'd be a good match to bring parity with CXL. > > So this also should now enable folks who want to say, run tests with > qemu specific topologies too. It'd just be another guest on when > spawing, and so tests can be run in parallel for each different > topology. cxl_test is good for testing ABI, but not hardware interfaces. That's where QEMU shines and so they will continue to compliment each other. Any automation that helps people get from zero to testing their CXL patches is appreciated. > > > As to what to do about it, I am open to suggestions. The typical flow > > has some lag between upstream tools/testing/cxl/ changes and when the > > corresponding ndctl/cxl changes land. This is because the tool enabling > > does not start until after it is clear that the kernel changes are going > > to land. After that it's another round of review to settle on the tool > > changes. The ndctl/pending branch should usually be up to date by the > > time -rc1 arrives. Is that sufficient? > > I think it would make sense to always assume that using the latest > tooling will not break anything and we should then be able to always > rely on it for all automationf or bleeding edge linux-next based > testing. Latest tools should never break, but that's different that saying that tests should not break since tests are hard coded to the topology. Maybe the kernel could publish a version number that increments whenever it makes a change that breaks existing tests. Unless then you can notice that the failure is due to out-of-date tests. At the same time I like the flexibility of new tests brewing alongside kernel patches and then taking the time to review the new test code while the kernel patches move ahead. > Tests however should then always look to see if a feature is available > and gracefully skip if not available / supported by the target kernel > being tested. This would then allow the *latest* ndctl and pending > branch to be relied on to test also even older kernels. That way, new > tests, but which can be useful for testing old kernels, can *also* > be enabled to test things on those old kernels. The tests are backwards compatible for older kernels. It's the older tests getting confused on newer kernels that is the problem. > And so the latest testing userspace scripts should *always* work with > *any* kernel. It should not assume you have the latest and greatest > respective cxl_test driver. Is that something we can count on today? There will always be the tension that the user tooling not move ahead until it is clear that the kernel is moving ahead. So cxl_test changes that cause kernel regressions will always hit -next before the next tool release. > Then as a developer, one can assume / expect the latest and greatest > both tooling / testing will work regardless of the kernel. > > That would mimic the experience with using tools/testing/selftests , > even linux-next selftests are supposed to work on older kernels. The > respective old selftest modules would just be used if they were enabled > for the older kernel. The difference there is that you can check the test in with the kernel change in the same commit. > Then, a few questions, but while we're at it: > > Do we have enough experience at this point to say what things should be > tested with the pair cxl_test / ndctl test suite and say what things > instead are probably better tested using qemu? There are some things that QEMU may just never support like RCH topologies. In general cxl_test is amenable to small hacks to test a driver corner case that are a much heavier lift on QEMU. On the other side, there are things that cxl_test will never support like register emulation. So, I think both are needed and complimentary going forward. > Are there tests, part of ndctl test suite which likely would be useful > for non cxl_test.ko usages, but rather run on spawned qemu / bare metal > CXL enabled systems, so one can just hammer on them? Yes, and nfit_test has an example of such things in its "destructive" suite where those try to mutate resources outside of the mock environment. To date there are no "destructive" CXL tests, but I can imagine, for example, cxl-create-region.sh being modified to run against the ACPI.CXL topology instead of cxl_test.
On Wed, 14 Dec 2022, Luis Chamberlain wrote: >Groovy thanks. Progress, but still fails now on the cxl-xor-region.sh line 29 > >https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493 Unrelated, but looking at this log I noticed that we should limit the amount of noise "Bypassing cpu_cache_invalidate_memregion() for testing" emits. Thanks, Davidlohr -----8<------------------------------------- [PATCH] cxl/region: Only warn about cpu_cache_invalidate_memregion() once No need for more than once per region. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/core/region.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index b1281f528d7f..f367d1c10357 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2006,8 +2006,7 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr) if (!cpu_cache_has_invalidate_memregion()) { if (IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) { - dev_warn( - &cxlr->dev, + dev_WARN_ONCE(&cxlr->dev, 1, "Bypassing cpu_cache_invalidate_memregion() for testing!\n"); clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); return 0;
Davidlohr Bueso wrote: > On Wed, 14 Dec 2022, Luis Chamberlain wrote: > > >Groovy thanks. Progress, but still fails now on the cxl-xor-region.sh line 29 > > > >https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493 > > Unrelated, but looking at this log I noticed that we should limit the amount > of noise "Bypassing cpu_cache_invalidate_memregion() for testing" emits. > > Thanks, > Davidlohr > > -----8<------------------------------------- > [PATCH] cxl/region: Only warn about cpu_cache_invalidate_memregion() once > > No need for more than once per region. Hmm, the below isn't once per region, it's once per cxl_core module load. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/core/region.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index b1281f528d7f..f367d1c10357 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2006,8 +2006,7 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr) > > if (!cpu_cache_has_invalidate_memregion()) { > if (IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) { > - dev_warn( > - &cxlr->dev, > + dev_WARN_ONCE(&cxlr->dev, 1, > "Bypassing cpu_cache_invalidate_memregion() for testing!\n"); This will end up failing the unit tests because those want to have a clean kernel log from a "Call Trace" perspective. So either dev_warn_once() or just live with the noise since the message is more for awareness in production environments and test environments can ignore it.
On Thu, 15 Dec 2022, Dan Williams wrote: >This will end up failing the unit tests because those want to have a >clean kernel log from a "Call Trace" perspective. So either >dev_warn_once() or just live with the noise since the message is more >for awareness in production environments and test environments can >ignore it. Yeah I quickly realized that dev_warn_once() is what I wanted. I'll send a v2.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index ad0849af42d7..cf5d1a455efc 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -633,6 +633,9 @@ static int cxl_acpi_probe(struct platform_device *pdev) struct acpi_device *adev = ACPI_COMPANION(host); struct cxl_cfmws_context ctx; + if (!adev) + return -ENODEV; + device_lock_set_class(&pdev->dev, &cxl_root_key); rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, &pdev->dev);
Simply loading cxl_test ends up triggering a null pointer dereference on next-20221207, and it happens because the fetched ACPI_COMPANION() can end up not correct / missing. As with other code which uses ACPI_COMPANION() (drivers/acpi/device_pm.c comes to mind) be defensive over the assumption the companion is always present and bail right away. This can be easily reproduced with kdevops [0] with linux next-20221207 [1] and cxl enabled workflows: make menuconfig # enable cxl and linux-next make # sets up variables, builds qemu from source make linux # builds and install next-20221207 make cxl # installs cxl tool make cxl-test-probe # loads cxl_test The oops: # modprobe cxl_test No TPM handle discovered. failed to open file /etc/ndctl/keys/nvdimm-master.blob: No such file or directory [0] https://github.com/linux-kdevops/kdevops [1] https://github.com/linux-kdevops/kdevops/blob/master/playbooks/roles/bootlinux/templates/config-next-20221207 cxl_mock: loading out-of-tree module taints kernel. cxl_mock: loading test module taints kernel. cxl_mem mem0: at cxl_root_port.0 no parent for dport: platform cxl_mem mem1: at cxl_root_port.1 no parent for dport: platform cxl_mem mem2: at cxl_root_port.2 no parent for dport: platform cxl_mem mem3: at cxl_root_port.3 no parent for dport: platform cxl_mem mem4: at cxl_root_port.0 no parent for dport: platform cxl_mem mem5: at cxl_root_port.1 no parent for dport: platform cxl_mem mem6: at cxl_root_port.2 no parent for dport: platform cxl_mem mem7: at cxl_root_port.3 no parent for dport: platform cxl_mem mem8: at cxl_root_port.4 no parent for dport: platform cxl_mem mem9: at cxl_root_port.4 no parent for dport: platform cxl_mem mem10: CXL port topology not found BUG: kernel NULL pointer dereference, address: 00000000000002c0 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP PTI CPU: 4 PID: 1644 Comm: systemd-udevd Kdump: loaded Tainted: G O N 6.1.0-rc8-next-20221207 #5 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014 RIP: 0010:cxl_acpi_probe+0xeb/0x2f0 [cxl_acpi] Code: ff ff ff 48 c7 40 08 ff ff ff ff 48 c7 40 18 00 02 00 00 e8 57 29 fd ff 49 89 c7 41 89 c4 48 3d 00 f0 ff ff 0f 87 73 ff ff ff <49> 8b bd c0 02 00 00 48 c7 c1 c0 64 e4 c0 48 89 c2 31 f6 e8 bd f1 RSP: 0018:ffffbe6d008b7c30 EFLAGS: 00010287 RAX: ffff97a7c6e01000 RBX: ffff97a7c51fd810 RCX: 0000000000000000 RDX: 0000000000000001 RSI: 0000000000000282 RDI: 00000000ffffffff RBP: 0000000000000000 R08: ffff97a7c51fdaa8 R09: 0000000000000010 R10: 0000000000000002 R11: 00000000000013c7 R12: 00000000c6e01000 R13: 0000000000000000 R14: ffff97a7d9c653a8 R15: ffff97a7c6e01000 FS: 00007f34b038ed00(0000) GS:ffff97a83bd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000000002c0 CR3: 0000000102f7e005 CR4: 0000000000770ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: <TASK> ? kernfs_create_link+0x5d/0xa0 platform_probe+0x41/0x90 really_probe+0xdb/0x380 ? pm_runtime_barrier+0x50/0x90 __driver_probe_device+0x78/0x170 driver_probe_device+0x1f/0x90 __driver_attach+0xce/0x1c0 ? __pfx___driver_attach+0x10/0x10 bus_for_each_dev+0x73/0xc0 bus_add_driver+0x1ae/0x200 driver_register+0x89/0xe0 ? __pfx_init_module+0x10/0x10 [cxl_acpi] do_one_initcall+0x43/0x220 ? kmalloc_trace+0x26/0x90 do_init_module+0x4a/0x1f0 __do_sys_init_module+0x17f/0x1b0 do_syscall_64+0x37/0x90 entry_SYSCALL_64_after_hwframe+0x72/0xdc RIP: 0033:0x7f34b061baaa Code: 48 8b 0d 59 83 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 26 83 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007fff6a198408 EFLAGS: 00000246 ORIG_RAX: 00000000000000af RAX: ffffffffffffffda RBX: 00005635afc7e5e0 RCX: 00007f34b061baaa RDX: 00007f34b07a5efd RSI: 0000000000060a29 RDI: 00005635afdd6510 RBP: 00007f34b07a5efd R08: 000000000001f5b3 R09: 0000000000000000 R10: 000000000000eb81 R11: 0000000000000246 R12: 00005635afdd6510 R13: 0000000000000000 R14: 00005635afca6f40 R15: 00005635af874e50 </TASK> Modules linked in: cxl_acpi(+) cxl_pmem cxl_mem cxl_port cxl_mock_mem(ON) cxl_test(ON) cxl_mock(ON) cxl_core libnvdimm cbc encrypted_keys kvm_intel kvm 9p netfs irqbypass crct10dif_pclmul ghash_clmulni_intel sha512_ssse3 sha512_generic aesni_intel crypto_simd cryptd cirrus drm_shmem_helper 9pnet_virtio virtio_balloon i6300esb drm_kms_helper joydev evdev button serio_raw drm configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 md_mod virtio_net net_failover virtio_blk failover psmouse virtio_pci virtio_pci_legacy_dev nvme virtio_pci_modern_dev crc32_pclmul nvme_core virtio crc32c_intel t10_pi virtio_ring crc64_rocksoft crc64 And gdb: (gdb) l *(cxl_acpi_probe+0xeb) 0xa8b is in cxl_acpi_probe (tools/testing/cxl/../../../drivers/cxl/acpi.c:648). 643 644 root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); 645 if (IS_ERR(root_port)) 646 return PTR_ERR(root_port); 647 648 rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, 649 add_host_bridge_dport); 650 if (rc < 0) 651 return rc; 652 Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- Note: kdevops also suports now the target: make cxl-test-meson But that does not *at least* crash the kernel although the tests fail too... This is likely a misconfiguration of some sort, but the same kernel works fine when I enable a Type 3 memory device (also supported on kdevops via CONFIG_QEMU_ENABLE_CXL_DEMO_TOPOLOGY_1). This test was run without that enabled, so a naked cxl system. Even if it *was* a mis-configuration, such things should not crash the kernel. drivers/cxl/acpi.c | 3 +++ 1 file changed, 3 insertions(+)