Re: [PATCH 1/6] iommu/virtio: publish the endpoint before replaying it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu/virtio: reverted domain lacks mappings on failed attach
--

commit 2adc3f48a11c16ff36ed27a26a044ebb6cd0e06f
Author: Anlai Lu <agicy@qq.com>

iommu/virtio: publish the endpoint before replaying it

The device drops a domain's mappings together with its last endpoint, so
the first endpoint attaching to a domain has to replay them. This commit
publishes the endpoint first, with the decision moved into
viommu_add_mapping() under mappings_lock so that it cannot race the insert,
and lets the replay pick up the rest.

> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
[ ... ]
> @@ -730,6 +787,96 @@ static struct iommu_domain *viommu_domain_alloc_identity(struct device *dev)
[ ... ]
> +/*
> + * Give the endpoint back to a domain a failed ATTACH had retired it from: the
> + * device never took the endpoint, so the count has to keep matching what it
> + * holds.  If retiring it had dropped the last endpoint, a replay is pending
> + * again - the device is not known to hold the domain's mappings, so a later
> + * attach replays instead of assuming they are there.
> + */
> +static void viommu_restore_endpoint(struct viommu_domain *vdomain)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vdomain->mappings_lock, flags);
> +	if (!vdomain->nr_endpoints)
> +		vdomain->replay_pending = true;
> +	vdomain->nr_endpoints++;

[Severity: High]
This function marks the domain as needing a replay for future endpoint
attaches by setting replay_pending to true.

Does this ignore the immediate need to restore mappings for the current
reverted endpoint?

[ ... ]
> @@ -762,20 +916,57 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev,
[ ... ]
> +			/*
> +			 * The ATTACH reached the device, so the endpoint has
> +			 * moved to the new domain even though its mappings did
> +			 * not all make it.  Report the failure and ask the
> +			 * device to take the endpoint back to the old domain,
> +			 * where the core believes it still is.  If it refuses,
> +			 * or there was no old domain, leave the endpoint where
> +			 * it is and record no domain for it: a domain the core
> +			 * does not keep for this endpoint must not be counted,
> +			 * and the endpoint is not left unattached either (a
> +			 * device that enables bypass would pass it through).
> +			 */
> +			if (old_vdomain &&
> +			    !viommu_revert_attach(vdev, old_vdomain))
> +				/* the old domain lost its mappings with it */
> +				viommu_restore_endpoint(old_vdomain);
> +			else
> +				vdev->vdomain = NULL;
> +			viommu_put_endpoint(vdomain);
>  			return ret;
> +		}
> +		viommu_replayed(vdomain);
>  	}

[Severity: High]
If viommu_replay_mappings() fails during viommu_attach_dev(), the endpoint is
reverted to its old domain on the hardware. 

Since the old domain's mappings were dropped by the device when the endpoint
initially moved, does the reverted domain now lack all mappings on the
physical device?

The driver calls viommu_restore_endpoint() which sets replay_pending to true,
but does not actually issue a replay for the restored endpoint. 

Could this cause all subsequent DMA from this device to fault silently, as
the OS assumes the old domain is still fully intact? This sequence appears
triggerable when device attachment fails due to memory pressure or
device-side rejection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_85AC6E749F6A912B7BB9FF360F8ECB3F5C09@qq.com?part=1