lkml.org 
[lkml]   [2022]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
    Date
    > From: Baolu Lu <baolu.lu@linux.intel.com>
    > Sent: Wednesday, June 1, 2022 7:02 PM
    >
    > On 2022/6/1 17:28, Tian, Kevin wrote:
    > >> From: Lu Baolu <baolu.lu@linux.intel.com>
    > >> Sent: Friday, May 27, 2022 2:30 PM
    > >>
    > >> When the IOMMU domain is about to be freed, it should not be set on
    > any
    > >> device. Instead of silently dealing with some bug cases, it's better to
    > >> trigger a warning to report and fix any potential bugs at the first time.
    > >>
    > >
    > >
    > >> static void domain_exit(struct dmar_domain *domain)
    > >> {
    > >> -
    > >> - /* Remove associated devices and clear attached or cached domains
    > >> */
    > >> - domain_remove_dev_info(domain);
    > >> + if (WARN_ON(!list_empty(&domain->devices)))
    > >> + return;
    > >>
    > >
    > > warning is good but it doesn't mean the driver shouldn't deal with
    > > that situation to make it safer e.g. blocking DMA from all attached
    > > device...
    >
    > I have ever thought the same thing. :-)
    >
    > Blocking DMA from attached device should be done when setting blocking
    > domain to the device. It should not be part of freeing a domain.

    yes but here we are talking about some bug scenario.

    >
    > Here, the caller asks the driver to free the domain, but the driver
    > finds that something is wrong. Therefore, it warns and returns directly.
    > The domain will still be there in use until the next set_domain().
    >

    at least it'd look safer if we always try to unmap the entire domain i.e.:

    static void domain_exit(struct dmar_domain *domain)
    {
    -
    - /* Remove associated devices and clear attached or cached domains */
    - domain_remove_dev_info(domain);

    if (domain->pgd) {
    LIST_HEAD(freelist);

    domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw), &freelist);
    put_pages_list(&freelist);
    }

    + if (WARN_ON(!list_empty(&domain->devices)))
    + return;

    kfree(domain);
    }

    Thanks
    Kevin
    \
     
     \ /
      Last update: 2022-06-02 08:31    [W:3.398 / U:1.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site