Re: [PATCH] iommu: Allocate dev_iommu before accessing priv data

[Robin Murphy <robin.murphy@arm.com>]

On 2020-09-02 06:32, Torsten Hilbrich wrote:
> After updating from v5.8 to v5.9-rc2 I noticed some problems when
> booting a system with kernel cmdline "intel_iommu=on,igfx_off".
> 
> The following stacktrace was produced:
> 
> <6>[    0.000000] Command line: BOOT_IMAGE=/isolinux/bzImage console=tty1 intel_iommu=on,igfx_off
> ...
> <6>[    3.341682] DMAR: Host address width 39
> <6>[    3.341684] DMAR: DRHD base: 0x000000fed90000 flags: 0x0
> <6>[    3.341702] DMAR: dmar0: reg_base_addr fed90000 ver 1:0 cap 1c0000c40660462 ecap 19e2ff0505e
> <6>[    3.341705] DMAR: DRHD base: 0x000000fed91000 flags: 0x1
> <6>[    3.341711] DMAR: dmar1: reg_base_addr fed91000 ver 1:0 cap d2008c40660462 ecap f050da
> <6>[    3.341713] DMAR: RMRR base: 0x0000009aa9f000 end: 0x0000009aabefff
> <6>[    3.341716] DMAR: RMRR base: 0x0000009d000000 end: 0x0000009f7fffff
> <6>[    3.341726] DMAR: No ATSR found
> <1>[    3.341772] BUG: kernel NULL pointer dereference, address: 0000000000000038
> <1>[    3.341774] #PF: supervisor write access in kernel mode
> <1>[    3.341776] #PF: error_code(0x0002) - not-present page
> <6>[    3.341777] PGD 0 P4D 0
> <4>[    3.341780] Oops: 0002 [#1] SMP PTI
> <4>[    3.341783] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.9.0-devel+ #2
> <4>[    3.341785] Hardware name: LENOVO 20HGS0TW00/20HGS0TW00, BIOS N1WET46S (1.25s ) 03/30/2018
> <4>[    3.341790] RIP: 0010:intel_iommu_init+0xed0/0x1136
> <4>[    3.341792] Code: fe e9 61 02 00 00 bb f4 ff ff ff e9 57 02 00 00 48 63 d1 48 c1 e2 04 48 03 50 20 48 8b 12 48 85 d2 74 0b 48 8b 92 d0 02 00 00 <48> 89 7a 38 ff c1 e9 15 f5 ff ff 48 c7 c7 60 99 ac a7 49 c7 c7 a0
> <4>[    3.341796] RSP: 0000:ffff96d180073dd0 EFLAGS: 00010282
> <4>[    3.341798] RAX: ffff8c91037a7d20 RBX: 0000000000000000 RCX: 0000000000000000
> <4>[    3.341800] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff
> <4>[    3.341802] RBP: ffff96d180073e90 R08: 0000000000000001 R09: ffff8c91039fe3c0
> <4>[    3.341804] R10: 0000000000000226 R11: 0000000000000226 R12: 000000000000000b
> <4>[    3.341806] R13: ffff8c910367c650 R14: ffffffffa8426d60 R15: 0000000000000000
> <4>[    3.341808] FS:  0000000000000000(0000) GS:ffff8c9107480000(0000) knlGS:0000000000000000
> <4>[    3.341810] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[    3.341812] CR2: 0000000000000038 CR3: 00000004b100a001 CR4: 00000000003706e0
> <4>[    3.341814] Call Trace:
> <4>[    3.341820]  ? _raw_spin_unlock_irqrestore+0x1f/0x30
> <4>[    3.341824]  ? call_rcu+0x10e/0x320
> <4>[    3.341828]  ? trace_hardirqs_on+0x2c/0xd0
> <4>[    3.341831]  ? rdinit_setup+0x2c/0x2c
> <4>[    3.341834]  ? e820__memblock_setup+0x8b/0x8b
> <4>[    3.341836]  pci_iommu_init+0x16/0x3f
> <4>[    3.341839]  do_one_initcall+0x46/0x1e4
> <4>[    3.341842]  kernel_init_freeable+0x169/0x1b2
> <4>[    3.341845]  ? rest_init+0x9f/0x9f
> <4>[    3.341847]  kernel_init+0xa/0x101
> <4>[    3.341849]  ret_from_fork+0x22/0x30
> <4>[    3.341851] Modules linked in:
> <4>[    3.341854] CR2: 0000000000000038
> <4>[    3.341860] ---[ end trace 3653722a6f936f18 ]---
> 
> I could track the problem down to the dev_iommu_priv_set call in the function
> init_no_remapping_devices in the path where !dmar_map_gfx. It turned out that
> the dev->iommu entry is NULL at this time.
> 
> Lu Baolu <baolu.lu@linux.intel.com> suggested for dev_iommu_priv_set
> to automatically allocate the iommu entry by using the function
> dev_iommu_get to retrieve that pointer. This function allocates the
> entry if needed.
> 
> Fixes: 01b9d4e21148 ("iommu/vt-d: Use dev_iommu_priv_get/set()")
> Signed-off-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> Tested-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> Link: https://lists.linuxfoundation.org/pipermail/iommu/2020-August/048098.html
> ---
>   drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
>   include/linux/iommu.h | 11 ++---------
>   2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 609bd25bf154..3edca2a31296 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2849,3 +2849,25 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
>   	return ops->sva_get_pasid(handle);
>   }
>   EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
> +
> +void *dev_iommu_priv_get(struct device *dev)
> +{
> +       struct dev_iommu *param = dev_iommu_get(dev);
> +
> +       if (WARN_ON(!param))
> +               return ERR_PTR(-ENOMEM);
> +
> +        return param->priv;
> +}
> +EXPORT_SYMBOL_GPL(dev_iommu_priv_get);

Hmm, I'm not convinced by this - it looks it would only paper over real 
driver bugs. If the driver's calling dev_iommu_priv_get(), it presumably 
wants to actually *do* something with its private data - if it somehow 
manages to make that call before it's processed ->probe_device(), it 
can't possibly get *meaningful* data, so even if we stop that call from 
crashing how can it result in correct behaviour?

And if the device isn't managed by that IOMMU driver, then it shouldn't 
be calling dev_iommu_priv_get() blindly in the first place (and 
allocating redundant structures would just be a waste).

> +void dev_iommu_priv_set(struct device *dev, void *priv)
> +{
> +       struct dev_iommu *param = dev_iommu_get(dev);
> +
> +       if (WARN_ON(!param))
> +               return;
> +
> +        param->priv = priv;
> +}
> +EXPORT_SYMBOL_GPL(dev_iommu_priv_set);

In this direction it's at least not completely illogical, but it's still 
indicative of a driver operating very much outside the expected API 
flow. If a driver has special knowledge of devices it manages before 
it's seen ->probe_device() for them then fair enough, but it should 
probably be that driver's responsibility to manage any "out of order" 
usage of its private data as a special case, rather than pretending that 
this is expected common behaviour. Again, for most drivers it's more 
likely to just mask bugs than be genuinely useful.

Robin.

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fee209efb756..e3e725cf64b3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -609,15 +609,8 @@ static inline void dev_iommu_fwspec_set(struct device *dev,
>   	dev->iommu->fwspec = fwspec;
>   }
>   
> -static inline void *dev_iommu_priv_get(struct device *dev)
> -{
> -	return dev->iommu->priv;
> -}
> -
> -static inline void dev_iommu_priv_set(struct device *dev, void *priv)
> -{
> -	dev->iommu->priv = priv;
> -}
> +void *dev_iommu_priv_get(struct device *dev);
> +void dev_iommu_priv_set(struct device *dev, void *priv);
>   
>   int iommu_probe_device(struct device *dev);
>   void iommu_release_device(struct device *dev);
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu