linux-kernel - Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <10a96a85-d7e9-7ac6-9c8d-f0e8b4597f01@linux.intel.com>
Date:   Thu, 24 Feb 2022 13:21:24 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     Robin Murphy <robin.murphy@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joerg Roedel <joro@...tes.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>
Cc:     baolu.lu@...ux.intel.com, Will Deacon <will@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>, rafael@...nel.org,
        Diana Craciun <diana.craciun@....nxp.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Stuart Yoder <stuyoder@...il.com>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Li Yang <leoyang.li@....com>,
        Dmitry Osipenko <digetx@...il.com>,
        iommu@...ts.linux-foundation.org, linux-pci@...r.kernel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 01/11] iommu: Add dma ownership management interfaces

On 2/24/22 2:00 AM, Robin Murphy wrote:
> On 2022-02-18 00:55, Lu Baolu wrote:
> [...]
>> +/**
>> + * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>> + * @group: The group.
>> + * @owner: Caller specified pointer. Used for exclusive ownership.
>> + *
>> + * This is to support backward compatibility for vfio which manages
>> + * the dma ownership in iommu_group level. New invocations on this
>> + * interface should be prohibited.
>> + */
>> +int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&group->mutex);
>> +    if (group->owner_cnt) {
> 
> To clarify the comment buried in the other thread, I really think we 
> should just unconditionally flag the error here...
> 
>> +        if (group->owner != owner) {
>> +            ret = -EPERM;
>> +            goto unlock_out;
>> +        }
>> +    } else {
>> +        if (group->domain && group->domain != group->default_domain) {
>> +            ret = -EBUSY;
>> +            goto unlock_out;
>> +        }
>> +
>> +        group->owner = owner;
>> +        if (group->domain)
>> +            __iommu_detach_group(group->domain, group);
>> +    }
>> +
>> +    group->owner_cnt++;
>> +unlock_out:
>> +    mutex_unlock(&group->mutex);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>> +
>> +/**
>> + * iommu_group_release_dma_owner() - Release DMA ownership of a group
>> + * @group: The group.
>> + *
>> + * Release the DMA ownership claimed by iommu_group_claim_dma_owner().
>> + */
>> +void iommu_group_release_dma_owner(struct iommu_group *group)
>> +{
>> +    mutex_lock(&group->mutex);
>> +    if (WARN_ON(!group->owner_cnt || !group->owner))
>> +        goto unlock_out;
>> +
>> +    if (--group->owner_cnt > 0)
>> +        goto unlock_out;
> 
> ...and equivalently just set owner_cnt directly to 0 here. I don't see a 
> realistic use-case for any driver to claim the same group more than 
> once, and allowing it in the API just feels like opening up various 
> potential corners for things to get out of sync.

Yeah! Both make sense to me. I will also drop the owner token in the API
as it's unnecessary anymore after the change.

> I think that's the only significant concern I have left with the series 
> as a whole - you can consider my other grumbles non-blocking :)

Thank you and very appreciated for your time!

Best regards,
baolu