[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e25c8360-41f6-4105-877f-dc5b6051541b@amd.com>
Date: Mon, 16 Sep 2024 23:12:23 +0700
From: "Suthikulpanit, Suravee" <suravee.suthikulpanit@....com>
To: Jason Gunthorpe <jgg@...dia.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 2/5] iommu/amd: Introduce helper functions to access
and update 256-bit DTE
On 9/7/2024 12:00 AM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 12:13:05PM +0000, Suravee Suthikulpanit wrote:
>
>> +static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
>> + struct dev_table_entry *new)
>> +{
>> + struct dev_table_entry *dev_table = get_dev_table(iommu);
>> + struct dev_table_entry *ptr = &dev_table[dev_data->devid];
>> + struct dev_table_entry old;
>> + u128 tmp;
>> +
>> + lockdep_assert_held(&dev_data->dte_lock);
>> +
>> + old.data128[0] = ptr->data128[0];
>> + old.data128[1] = ptr->data128[1];
>> +
>> + tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
>> + if (tmp == old.data128[1]) {
>> + if (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0])) {
>> + /* Restore hi 128-bit */
>> + cmpxchg128(&ptr->data128[1], new->data128[1], tmp);
>
> Like I said before, you can't fix this. Just go fowards. Keeping an
> old DTE will UAF things, that is much worse than just forcing a new
> DTE and loosing some interrupt settings.
>
>> @@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
>> if (!iommu)
>> return 0;
>>
>> - amd_iommu_set_rlookup_table(iommu, alias);
>> - dev_table = get_dev_table(iommu);
>> - memcpy(dev_table[alias].data,
>> - dev_table[devid].data,
>> - sizeof(dev_table[alias].data));
>> + /* Get DTE for pdev */
>> + dev_data = dev_iommu_priv_get(&pdev->dev);
>> + if (!dev_data)
>> + return -EINVAL;
>>
>> - return 0;
>> + spin_lock(&dev_data->dte_lock);
>> + get_dte256(iommu, dev_data, &dte);
>> + spin_unlock(&dev_data->dte_lock);
>
> You can't unlock after reading the DTE and the relock it to program
> it. The interrupt data can have changed while unlocked.
>
> Put the lock inside update_dte256() and force the interrupt bits
> under the lock.
>
> Something like this is what I'm expecting:
>
> static void write_upper(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> struct dev_table_entry old;
>
> lockdep_assert_held(&dev_data->dte_lock);
>
> do {
> old->data128[1] = ptr->data128[1];
> new->data64[2] &= ~INTR_MASK;
> new->data64[2] |= old->data64[2] & INTR_MASK;
> } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
> new->data128[1]));
> }
>
> static void write_lower(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> struct dev_table_entry old;
>
> do {
> old->data128[0] = ptr->data128[0];
> } while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
> new->data128[0]));
> }
>
> static void update_dte256(struct amd_iommu *iommu,
> struct iommu_dev_data *dev_data,
> struct dev_table_entry *new)
> {
>
> spin_lock(&dev_data->dte_lock);
> if (!(ptr->data64[0] & V)) {
> write_upper(ptr, new);
> /* NO FLUSH assume V=0 never cached */
> write_lower(ptr, new);
> /* FLUSH */
> } else if (!(new->data64[0] & V) {
> write_lower(ptr, new);
> /* FLUSH */
> write_upper(ptr, new);
> /* NO FLUSH assume V=0 never cached */
> } else {
> /* both are valid */
> if (FIELD_GET(ptr->data[2], GUEST_PAGING_MODE) ==
> FIELD_GET(new->data[2], GUEST_PAGING_MODE)) {
> /* Upper doesn't change */
> write_upper(ptr, new);
> write_lower(ptr, new);
> /* FLUSH */
> else if (old has no guest page table) {
> write_upper(ptr, new);
> /* FLUSH */
> write_lower(ptr, new);
> /* FLUSH */
> else if (new has no guest page table) {
> write_lower(ptr, new);
> /* FLUSH */
> write_upper(ptr, new);
> /* FLUSH */
> } else {
> struct dev_table_entry clear = {};
>
> write_lower(ptr, &clear);
> /* FLUSH to set V=0 */
> write_upper(ptr, new);
> /* NO FLUSH assume V=0 never cached */
> write_lower(ptr, new);
> /* FLUSH */
> }
> }
>
> spin_unlock(&dev_data->dte_lock);
> }
Got your point. I will update based on your suggestion here.
> And it probably needs more logic to accomodate the VIOMMU valid bits
> in the 2nd 128.
I'll take care of this when enable the HW-vIOMMU stuff.
Thanks,
Suravee
Powered by blists - more mailing lists