The err_restore_domain flow was accidently inserted into the success path in commit 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM"). It should only happen if iommu_create_device_direct_mappings() fails. This caused the domains the be wrongly changed and freed whenever the sysfs is used, resulting in an oops: BUG: kernel NULL pointer dereference, address: 0000000000000000 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 1 PID: 3417 Comm: avocado Not tainted 6.4.0-rc4-next-20230602 #3 Hardware name: Dell Inc. PowerEdge R6515/07PXPY, BIOS 2.3.6 07/06/2021 RIP: 0010:__iommu_attach_device+0xc/0xa0 Code: c0 c3 cc cc cc cc 48 89 f0 c3 cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 54 55 48 8b 47 08 <48> 8b 00 48 85 c0 74 74 48 89 f5 e8 64 12 49 00 41 89 c4 85 c0 74 RSP: 0018:ffffabae0220bd48 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff9ac04f70e410 RCX: 0000000000000001 RDX: ffff9ac044db20c0 RSI: ffff9ac044fa50d0 RDI: ffff9ac04f70e410 RBP: ffff9ac044fa50d0 R08: 1000000100209001 R09: 00000000000002dc R10: 0000000000000000 R11: 0000000000000000 R12: ffff9ac043d54700 R13: ffff9ac043d54700 R14: 0000000000000001 R15: 0000000000000001 FS: 00007f02e30ae000(0000) GS:ffff9afeb2440000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000000 CR3: 000000012afca006 CR4: 0000000000770ee0 PKRU: 55555554 Call Trace: <TASK> ? __die+0x24/0x70 ? page_fault_oops+0x82/0x150 ? __iommu_queue_command_sync+0x80/0xc0 ? exc_page_fault+0x69/0x150 ? asm_exc_page_fault+0x26/0x30 ? __iommu_attach_device+0xc/0xa0 ? __iommu_attach_device+0x1c/0xa0 __iommu_device_set_domain+0x42/0x80 __iommu_group_set_domain_internal+0x5d/0x160 iommu_setup_default_domain+0x318/0x400 iommu_group_store_type+0xb1/0x200 kernfs_fop_write_iter+0x12f/0x1c0 vfs_write+0x2a2/0x3b0 ksys_write+0x63/0xe0 do_syscall_64+0x3f/0x90 entry_SYSCALL_64_after_hwframe+0x6e/0xd8 RIP: 0033:0x7f02e2f14a6f Reorganize the error flow so that the success branch and error branches are clearer.
Unified · Split
| @@ -2891,14 +2891,11 @@ static int iommu_setup_default_domain(struct iommu_group *group, · 2890 unmodified lines | ||
| 2891 | 2891 | ret = __iommu_group_set_domain_internal( |
| 2892 | 2892 | group, dom, IOMMU_SET_DOMAIN_MUST_SUCCEED); |
| 2893 | 2893 | if (WARN_ON(ret)) |
| 2894 | − goto out_free; | |
| 2894 | + goto out_free_old; | |
| 2895 | 2895 | } else { |
| 2896 | 2896 | ret = __iommu_group_set_domain(group, dom); |
| 2897 | − if (ret) { | |
| 2898 | − iommu_domain_free(dom); | |
| 2899 | − group->default_domain = old_dom; | |
| 2900 | − return ret; | |
| 2901 | − } | |
| 2897 | + if (ret) | |
| 2898 | + goto err_restore_def_domain; | |
| 2902 | 2899 | } |
| 2903 | 2900 | |
| 2904 | 2901 | /* |
| @@ -2911,20 +2908,24 @@ static int iommu_setup_default_domain(struct iommu_group *group, · 6 unmodified lines | ||
| 2911 | 2908 | for_each_group_device(group, gdev) { |
| 2912 | 2909 | ret = iommu_create_device_direct_mappings(dom, gdev->dev); |
| 2913 | 2910 | if (ret) |
| 2914 | − goto err_restore; | |
| 2911 | + goto err_restore_domain; | |
| 2915 | 2912 | } |
| 2916 | 2913 | } |
| 2917 | 2914 | |
| 2918 | −err_restore: | |
| 2919 | − if (old_dom) { | |
| 2915 | +out_free_old: | |
| 2916 | + if (old_dom) | |
| 2917 | + iommu_domain_free(old_dom); | |
| 2918 | + return ret; | |
| 2919 | + | |
| 2920 | +err_restore_domain: | |
| 2921 | + if (old_dom) | |
| 2920 | 2922 | __iommu_group_set_domain_internal( |
| 2921 | 2923 | group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED); |
| 2924 | +err_restore_def_domain: | |
| 2925 | + if (old_dom) { | |
| 2922 | 2926 | iommu_domain_free(dom); |
| 2923 | − old_dom = NULL; | |
| 2927 | + group->default_domain = old_dom; | |
| 2924 | 2928 | } |
| 2925 | −out_free: | |
| 2926 | − if (old_dom) | |
| 2927 | − iommu_domain_free(old_dom); | |
| 2928 | 2929 | return ret; |
| 2929 | 2930 | } |
| 2930 | 2931 | |