[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240906180735.GM1358970@nvidia.com>
Date: Fri, 6 Sep 2024 15:07:35 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev, joro@...tes.org,
robin.murphy@....com, vasant.hegde@....com, ubizjak@...il.com,
jon.grimm@....com, santosh.shukla@....com, pandoh@...gle.com,
kumaranand@...gle.com
Subject: Re: [PATCH v3 4/5] iommu/amd: Modify clear_dte_entry() to avoid
in-place update
On Fri, Sep 06, 2024 at 12:13:07PM +0000, Suravee Suthikulpanit wrote:
> Lock DTE and copy value to a temporary storage before update using
> cmpxchg128.
>
> Also, refactor the function to simplify logic for applying erratum 63.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> ---
> drivers/iommu/amd/amd_iommu_types.h | 2 ++
> drivers/iommu/amd/iommu.c | 27 ++++++++++++++++++++-------
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 1836da2d9e60..81a994471a30 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -425,6 +425,8 @@
>
> #define DTE_GPT_LEVEL_SHIFT 54
>
> +#define DTE_SYSMGT_MASK GENMASK_ULL(41, 40)
> +
> #define GCR3_VALID 0x01ULL
>
> #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f18ae6c077f4..15eb816d4313 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2025,19 +2025,32 @@ static void set_dte_entry(struct amd_iommu *iommu,
> }
> }
>
> -static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> +static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
> {
> - struct dev_table_entry *dev_table = get_dev_table(iommu);
> + struct dev_table_entry new;
> + struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
> +
> + /*
> + * Need to preserve DTE[96:106] because certain fields are
> + * programmed using value in IVRS table from early init phase.
> + */
> + spin_lock(&dev_data->dte_lock);
> + get_dte256(iommu, dev_data, &new);
I think there is no point in the get?
struct dev_table_entry new = {}
new.data[0] = DTE_FLAG_V;
if (FIELD_GET(DTE_SYSMGT_MASK, old.data[1]) == 0x01)
new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
new.data[1] = old.data[1] & DTE_FLAG_MASK
new.data[2..4] = 0
That is pretty clear and simple
> - dev_table[devid].data[1] &= DTE_FLAG_MASK;
> + new.data[1] &= DTE_FLAG_MASK;
Would be nice if DTE_FLAG_MASK was broken into fields someday..
> - amd_iommu_apply_erratum_63(iommu, devid);
> + /* Apply erratum 63 */
> + if (FIELD_GET(DTE_SYSMGT_MASK, new.data[1]) == 0x01)
> + new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
> +
> + WARN_ON(!try_cmpxchg128(&dte->data128[0], &dte->data128[0], new.data128[0]));
As before this has to move forward, we can't fail to clear the DTE, it
will open a UAF
This should also clear the top 128 bits, so I would call the
update_dte256 directly?
Jason
Powered by blists - more mailing lists