Re: [PATCH v4 4/5] iommu/vt-d: Fail probe on ATS configuration failure

[Pranjal Shrivastava <praan@google.com>]

On Thu, May 28, 2026 at 01:19:13PM +0800, Baolu Lu wrote:
> On 5/26/26 04:56, sashiko-bot@kernel.org wrote:
> > > @@ -3288,7 +3294,10 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
> > >   	dev_iommu_priv_set(dev, info);
> > >   	if (pdev && pci_ats_supported(pdev)) {
> > > -		pci_prepare_ats(pdev, VTD_PAGE_SHIFT);
> > > +		ret = pci_prepare_ats(pdev, VTD_PAGE_SHIFT);
> > > +		if (ret)
> > > +			goto free;
> > > +
> > >   		ret = device_rbtree_insert(iommu, info);
> > >   		if (ret)
> > >   			goto free;
> > [Severity: High]
> > This is a pre-existing issue, but does unconditional removal of the rb-node
> > on the error path cause tree corruption for non-ats devices?
> > 
> > If a device does not support ats, the info structure skips the
> > device_rbtree_insert() call here, leaving info->node zero-initialized.
> > 
> > However, if a subsequent operation like intel_pasid_alloc_table() fails,
> > the error path jumps to clear_rbtree:
> > 
> > intel_iommu_probe_device()
> >      ...
> >      clear_rbtree:
> >              device_rbtree_remove(info);
> >      free:
> >              kfree(info);
> > 
> > Calling device_rbtree_remove() on this zero-initialized node causes the
> > rb-tree logic to treat it as a root node with no parent, ultimately
> > overwriting the actual tree root with null and corrupting the device_rbtree.
> 
> Perhaps we can fix it like this?
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 4d0e65bc131d..849d06dfe1ae 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -157,7 +157,10 @@ static void device_rbtree_remove(struct
> device_domain_info *info)
>         unsigned long flags;
> 
>         spin_lock_irqsave(&iommu->device_rbtree_lock, flags);
> -       rb_erase(&info->node, &iommu->device_rbtree);
> +       if (!RB_EMPTY_NODE(&info->node)) {
> +               rb_erase(&info->node, &iommu->device_rbtree);
> +               RB_CLEAR_NODE(&info->node);
> +       }
>         spin_unlock_irqrestore(&iommu->device_rbtree_lock, flags);
>  }
> 
> @@ -3254,6 +3257,7 @@ static struct iommu_device
> *intel_iommu_probe_device(struct device *dev)
> 
>         info->dev = dev;
>         info->iommu = iommu;
> +       RB_CLEAR_NODE(&info->node);
>         if (dev_is_pci(dev)) {
>                 if (ecap_dev_iotlb_support(iommu->ecap) &&
>                     pci_ats_supported(pdev) &&

Ack. Yes. I'll add it to the next version.

Thanks,
Praan