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

[Pranjal Shrivastava <praan@google.com>]

On Mon, Jun 01, 2026 at 03:30:31PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Unconditional call to `device_rbtree_remove(info)` in the error path causes rbtree corruption for non-ATS devices.
> --
> 
> commit 73f5ec2d0aa9d509723e686b4cbf5b853c3a39c6
> Author: Pranjal Shrivastava <praan@google.com>
> 
>     iommu/vt-d: Fail probe on ATS configuration failure
>     
>     Update the Intel VT-d driver to handle ATS configuration and enablement
>     more strictly. Specifically, update the device probe to fail if
>     pci_prepare_ats() returns an error. This ensures that any ATS-capable
>     master reaching the attach phase is guaranteed to have a valid config.
> 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 4d0e65bc131d7..22308e4911e1f 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> [ ... ]
> > @@ -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 the error path in this function
> unconditionally call device_rbtree_remove() on failure, even for non-ATS
> devices?
> 
> If intel_pasid_alloc_table() fails later in intel_iommu_probe_device(), it
> jumps to clear_rbtree:
> 
>         if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
>                 ret = intel_pasid_alloc_table(dev);
>                 if (ret) {
>                         dev_err(dev, "PASID table allocation failed\n");
>                         goto clear_rbtree;
>                 }
> 
> clear_rbtree unconditionally removes the info from the rbtree:
> 
> clear_rbtree:
>         device_rbtree_remove(info);
> 
> Since info->node is zero-initialized via kzalloc_obj(), calling rb_erase()
> on it for a non-ATS device (which was never inserted into the rbtree) causes
> the rbtree implementation to treat it as a root node and overwrite
> iommu->device_rbtree.rb_node with NULL.
> 
> This silently destroys the entire rbtree and detaches all previously
> registered ATS devices. Could we add a check to verify if the device was
> actually inserted before calling device_rbtree_remove(info)?

All of these have already been fixed in the Intel series:
https://lore.kernel.org/all/20260531170254.60493-1-praan@google.com/

Thanks,
Praan