Message-ID: <CAGXv+5F1zj_sSkMSZuu8eV+nVT5wu-GChCNxr8cOE=rRQierhQ@mail.gmail.com>
Date: Fri, 11 Apr 2025 12:00:10 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: Robin Murphy <robin.murphy@....com>, Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, 
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu: Clear IOMMU DMA ops when detaching a device from
 its IOMMU group

On Fri, Apr 11, 2025 at 9:34 AM Baolu Lu <baolu.lu@...ux.intel.com> wrote:
>
> On 4/10/25 18:09, Chen-Yu Tsai wrote:
> > In recent changes to the IOMMU subsystem seen in v6.15-rc1, the process
> > and order of how IOMMU linked devices are brought up has changed. Now
> > when the IOMMU is probed, the IOMMU subsystem core will attempt to link
> > or attach devices described in the device tree as using the IOMMU to
> > the IOMMU's associated group or domain. If any part of this fails, the
> > whole process is (mostly) undone. The IOMMU failure is then treated as
> > a deferred probe; after sufficient time, the deferred probe times out
> > and the attached devices are brought out without the IOMMU.
> >
> > In the process of undoing changes, one part was missed. When a device
> > is initialized for use with the IOMMU, iommu_setup_dma_ops() is also
> > called, which signals that DMA operations should go through the IOMMU.
> > This part was not reverted when the IOMMU changes were undone. When
> > the device later probes without the IOMMU, DMA operations would attempt
> > to use IOMMU operations with no IOMMU domain present, causing a crash.
> >
> > The above was observed on an MT8188 Chromebook. The MT8188 device tree
> > had incorrectly described the IOMMU endpoint for part of its display
> > pipeline; the IOMMU driver would successfully attach a couple hardware
> > blocks, fail at the incorrectly described one, and roll back all
> > changes. Later when the deferred probe times out, the display pipleine
> > probes without the IOMMU, but when a framebuffer is allocated, it goes
> > through the IOMMU DMA ops, causing a NULL pointer dereference crash.
> >
> > Add a helper that is the opposite of iommu_setup_dma_ops(), and call it
> > when the IOMMU-enabled device is being detached from its IOMMU.
> >
> > Closes:https://lore.kernel.org/all/CAGXv+5HJpTYmQ2h-
> > GD7GjyeYT7bL9EBCvu0mz5LgpzJZtzfW0w@...l.gmail.com/
> > Signed-off-by: Chen-Yu Tsai<wenst@...omium.org>
> > ---
> > This patch should be applied for v6.15. It's not immediately clear to
> > me which commit is the actual cause, so I did not add a Fixes tag.
>
> I guess it might be:
>
> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe
> path")
>
> Or, something near that. Please verify before apply.

I can't really tell.

> > ---
> >   drivers/iommu/dma-iommu.c | 5 +++++
> >   drivers/iommu/dma-iommu.h | 5 +++++
> >   drivers/iommu/iommu.c     | 2 ++
> >   3 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index cb7e29dcac15..62a51d84ffe1 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -1739,6 +1739,11 @@ void iommu_setup_dma_ops(struct device *dev)
> >       dev->dma_iommu = false;
> >   }
> >
> > +void iommu_clear_dma_ops(struct device *dev)
> > +{
> > +     dev->dma_iommu = false;
> > +}
> > +
> >   static bool has_msi_cookie(const struct iommu_domain *domain)
> >   {
> >       return domain && (domain->cookie_type == IOMMU_COOKIE_DMA_IOVA ||
> > diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
> > index eca201c1f963..dfd31cb9e685 100644
> > --- a/drivers/iommu/dma-iommu.h
> > +++ b/drivers/iommu/dma-iommu.h
> > @@ -10,6 +10,7 @@
> >   #ifdef CONFIG_IOMMU_DMA
> >
> >   void iommu_setup_dma_ops(struct device *dev);
> > +void iommu_clear_dma_ops(struct device *dev);
> >
> >   int iommu_get_dma_cookie(struct iommu_domain *domain);
> >   void iommu_put_dma_cookie(struct iommu_domain *domain);
> > @@ -30,6 +31,10 @@ static inline void iommu_setup_dma_ops(struct device *dev)
> >   {
> >   }
> >
> > +static inline void iommu_clear_dma_ops(struct device *dev)
> > +{
> > +}
> > +
> >   static inline int iommu_dma_init_fq(struct iommu_domain *domain)
> >   {
> >       return -EINVAL;
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index c8033ca66377..498f8f48394c 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -677,6 +677,8 @@ static void __iommu_group_remove_device(struct device *dev)
> >       struct iommu_group *group = dev->iommu_group;
> >       struct group_device *device;
> >
> > +     iommu_clear_dma_ops(dev);
> > +
> >       mutex_lock(&group->mutex);
> >       for_each_group_device(group, device) {
> >               if (device->dev != dev)
>
> I don't think there needs to be an inline helper for this one-time call.
> Perhaps you can simply do it like this:
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c8033ca66377..cd9d7a0fcaa0 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -536,6 +536,7 @@ static void iommu_deinit_device(struct device *dev)
>
>          /* Caller must put iommu_group */
>          dev->iommu_group = NULL;
> +       dev->dma_iommu = false;

I wanted to keep the reverting part in the cleanup path corresponding
to where it was set up, with corresponding helpers, so that they are
symmetric. That said I'm not that familiar with the IOMMU code, but it
makes more sense to me than sticking a random line somewhere.


ChenYu

>          module_put(ops->owner);
>          dev_iommu_free(dev);
>   }
>
> ?
>
> Thanks,
> baolu

Powered by blists - more mailing lists