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 <[email protected]>
---
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);
--
2.35.1
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 <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
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 <[email protected]>
> ---
>
> 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.
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 <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
>
> 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);
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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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/[email protected] [1]
Reported-by: Luis Chamberlain <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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_ */
--
2.38.1
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 <[email protected]>
> 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/[email protected] [1]
> Reported-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
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 <[email protected]>
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 <[email protected]>
---
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.
--
2.35.1
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 <[email protected]>
> > 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/[email protected] [1]
> > Reported-by: Luis Chamberlain <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
>
> 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 <[email protected]>
> > > 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/[email protected] [1]
> > > Reported-by: Luis Chamberlain <[email protected]>
> > > Signed-off-by: Dan Williams <[email protected]>
> >
> > 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 <[email protected]>
> > > > 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/[email protected] [1]
> > > > Reported-by: Luis Chamberlain <[email protected]>
> > > > Signed-off-by: Dan Williams <[email protected]>
> > >
> > > 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 <[email protected]>
---
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;
--
2.38.1
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 <[email protected]>
> ---
> 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.