Re: [REGRESSION] qcom: iommu: nullpointer dereference on boot on apq8064

[Christian Schrefl <chrisi.schrefl@gmail.com>]

Hi Robin,

On 1/6/26 7:50 PM, Robin Murphy wrote:
> On 2026-01-05 6:02 pm, Jason Gunthorpe wrote:
>> Though this looks really weird:
>>
>>          struct msm_iommu_ctx_dev *master = dev_iommu_priv_get(dev);
>>          int sid;
>>
>>          if (list_empty(&(*iommu)->ctx_list)) {
>>                  master = kzalloc(sizeof(*master), GFP_ATOMIC);
>>
>> So maybe master is NULL and !list_empty()?
> 
> AFAICS that could happen if of_xlate has run once, and then for whatever
> reason dev->iommu is torn down and the whole process started from
> scratch a second time - although I'm struggling to see any obvious cause
> for that in the absence of any other visible errors or async probe
> races (and assuming that the IOMMUs on this platform ever actually
> worked at all, of course...)
> 
> However it certainly stands out as a bit wrong that of_xlate is touching
> the IOMMU instance itself - that should never have been expected to work
> well. Does the diff below help?
> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 819add75a665..e57780fc3287 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -360,14 +360,11 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
>  /* Must be called under msm_iommu_lock */
>  static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>  {
> +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>      struct msm_iommu_dev *iommu, *ret = NULL;
> -    struct msm_iommu_ctx_dev *master;
>  
>      list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
> -        master = list_first_entry(&iommu->ctx_list,
> -                      struct msm_iommu_ctx_dev,
> -                      list);
> -        if (master->of_node == dev->of_node) {
> +        if (iommu->dev->fwnode == fwspec->iommu_fwnode) {
>              ret = iommu;
>              break;
>          }
> @@ -378,6 +375,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>  
>  static struct iommu_device *msm_iommu_probe_device(struct device *dev)
>  {
> +    struct msm_iommu_ctx_dev *master;
>      struct msm_iommu_dev *iommu;
>      unsigned long flags;
>  
> @@ -388,6 +386,8 @@ static struct iommu_device *msm_iommu_probe_device(struct device *dev)
>      if (!iommu)
>          return ERR_PTR(-ENODEV);
>  
> +    master = dev_iommu_priv_get(dev);
> +    list_add(&master->list, &iommu->ctx_list);
>      return &iommu->iommu;
>  }
>  
> @@ -604,14 +604,13 @@ static int insert_iommu_master(struct device *dev,
>      struct msm_iommu_ctx_dev *master = dev_iommu_priv_get(dev);
>      int sid;
>  
> -    if (list_empty(&(*iommu)->ctx_list)) {
> +    if (!master) {
>          master = kzalloc(sizeof(*master), GFP_ATOMIC);
>          if (!master) {
>              dev_err(dev, "Failed to allocate iommu_master\n");
>              return -ENOMEM;
>          }
>          master->of_node = dev->of_node;
> -        list_add(&master->list, &(*iommu)->ctx_list);
>          dev_iommu_priv_set(dev, master);
>      }
>  

Sorry for taking so long until I got to trying it out.
The patch fixes the crash!

Cheers,
Christian