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

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Robin,

On 9/2/20 7:31 PM, Robin Murphy wrote:
> 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).

Fair enough. The dev_iommu_priv_g(s)et() apis shouldn't be abused.

> 
>> +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.

Agreed. I will post a fix by addressing this bug in the VT-d driver.

Best regards,
baolu

> 
> 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