Re: [PATCH v2 11/19] iommu/arm-smmu-v3: Do not change the STE twice during arm_smmu_attach_dev()

[Michael Shavit <mshavit@google.com>]

On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> This was needed because the STE code required the STE to be in
> ABORT/BYPASS inorder to program a cdtable or S2 STE. Now that the STE code
> can automatically handle all transitions we can remove this step
> from the attach_dev flow.
>
> A few small bugs exist because of this:
>
> 1) If the core code does BLOCKED -> UNMANAGED with disable_bypass=false
>    then there will be a moment where the STE points at BYPASS. Since
>    this can be done by VFIO/IOMMUFD it is a small security race.
>
> 2) If the core code does IDENTITY -> DMA then any IOMMU_RESV_DIRECT
>    regions will temporarily become BLOCKED. We'd like drivers to
>    work in a way that allows IOMMU_RESV_DIRECT to be continuously
>    functional during these transitions.
>
> Make arm_smmu_release_device() put the STE back to the correct
> ABORT/BYPASS setting. Fix a bug where a IOMMU_RESV_DIRECT was ignored on
> this path.
>
> Notice this subtly depends on the prior arm_smmu_asid_lock change as the
> STE must be put to non-paging before removing the device for the linked
> list to avoid races with arm_smmu_share_asid().

I'm a little confused by this comment. Is this suggesting that
arm_smmu_detach_dev had a race condition before the arm_smmu_asid_lock
changes, since it deletes the list entry before deactivating the STE
that uses the domain and without grabbing the asid_lock, thus allowing
a gap where the ASID might be re-acquired by an SVA domain while an
STE with that ASID is still live on this device? Wouldn't that belong
on the asid_lock patch instead if so?

>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 4b157c2ddf9a80..f70862806211de 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2482,7 +2482,6 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  {
>         unsigned long flags;
> -       struct arm_smmu_ste target;
>         struct arm_smmu_domain *smmu_domain = master->domain;
>
>         if (!smmu_domain)
> @@ -2496,11 +2495,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>
>         master->domain = NULL;
>         master->ats_enabled = false;
> -       if (disable_bypass)
> -               arm_smmu_make_abort_ste(&target);
> -       else
> -               arm_smmu_make_bypass_ste(&target);
> -       arm_smmu_install_ste_for_dev(master, &target);
>         /*
>          * Clearing the CD entry isn't strictly required to detach the domain
>          * since the table is uninstalled anyway, but it helps avoid confusion
> @@ -2852,9 +2846,18 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  static void arm_smmu_release_device(struct device *dev)
>  {
>         struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +       struct arm_smmu_ste target;
>
>         if (WARN_ON(arm_smmu_master_sva_enabled(master)))
>                 iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> +
> +       /* Put the STE back to what arm_smmu_init_strtab() sets */

Hmmmm, it seems like checking iommu->require_direct may put STEs in
bypass in scenarios where arm_smmu_init_strtab() wouldn't have.
arm_smmu_init_strtab is calling iort_get_rmr_sids to pick streams to
put into bypass, but IIUC iommu->require_direct also applies to
dts-based reserved-memory regions, not just iort.

I'm not very familiar with the history behind disable_bypass; why is
putting an entire stream into bypass the correct behavior if a
reserved-memory (which may be for a small finite region) exists?

> +       if (disable_bypass && !dev->iommu->require_direct)
> +               arm_smmu_make_abort_ste(&target);
> +       else
> +               arm_smmu_make_bypass_ste(&target);
> +       arm_smmu_install_ste_for_dev(master, &target);
> +
>         arm_smmu_detach_dev(master);
>         arm_smmu_disable_pasid(master);
>         arm_smmu_remove_master(master);
> --
> 2.42.0
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel