linux-kernel - Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

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: <20190814102032.5265e1da@jacob-builder>
Date:   Wed, 14 Aug 2019 10:20:32 -0700
From:   Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     iommu@...ts.linux-foundation.org,
        LKML <linux-kernel@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Eric Auger <eric.auger@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        Yi Liu <yi.l.liu@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Christoph Hellwig <hch@...radead.org>,
        Andriy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

On Fri, 5 Jul 2019 10:21:27 +0800
Lu Baolu <baolu.lu@...ux.intel.com> wrote:

> Hi Jacob,
> 
> On 6/28/19 4:22 AM, Jacob Pan wrote:
> >>> +		}
> >>> +		refcount_set(&svm->refs, 0);
> >>> +		ioasid_set_data(data->hpasid, svm);
> >>> +		INIT_LIST_HEAD_RCU(&svm->devs);
> >>> +		INIT_LIST_HEAD(&svm->list);
> >>> +
> >>> +		mmput(svm->mm);
> >>> +	}
> >>> +	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
> >>> +	if (!sdev) {
> >>> +		ret = -ENOMEM;
> >>> +		goto out;  
> >> I think you need to clean up svm if its device list is empty here,
> >> as you said above:
> >>  
> > No, we come here only if the device list is not empty and the new
> > device to bind is different than any existing device in the list.
> > If we cannot allocate memory for the new device, should not free
> > the existing SVM, right?
> >   
> 
> I'm sorry, but the code doesn't show this. We come here even an svm
> data structure was newly created with an empty device list. I post
> the code below to ensure that we are reading a same piece of code.
> 
Sorry for the delay. You are right, I need to clean up svm if device
list is empty.

Thanks!
>          mutex_lock(&pasid_mutex);
>          svm = ioasid_find(NULL, data->hpasid, NULL);
>          if (IS_ERR(svm)) {
>                  ret = PTR_ERR(svm);
>                  goto out;
>          }
>          if (svm) {
>                  /*
>                   * If we found svm for the PASID, there must be at
>                   * least one device bond, otherwise svm should be
> freed. */
>                  BUG_ON(list_empty(&svm->devs));
> 
>                  for_each_svm_dev() {
>                          /* In case of multiple sub-devices of the
> same pdev assigned, we should
>                           * allow multiple bind calls with the same 
> PASID and pdev.
>                           */
>                          sdev->users++;
>                          goto out;
>                  }
>          } else {
>                  /* We come here when PASID has never been bond to a 
> device. */
>                  svm = kzalloc(sizeof(*svm), GFP_KERNEL);
>                  if (!svm) {
>                          ret = -ENOMEM;
>                          goto out;
>                  }
>                  /* REVISIT: upper layer/VFIO can track host process 
> that bind the PASID.
>                   * ioasid_set = mm might be sufficient for vfio to 
> check pasid VMM
>                   * ownership.
>                   */
>                  svm->mm = get_task_mm(current);
>                  svm->pasid = data->hpasid;
>                  if (data->flags & IOMMU_SVA_GPASID_VAL) {
>                          svm->gpasid = data->gpasid;
>                          svm->flags &= SVM_FLAG_GUEST_PASID;
>                  }
>                  refcount_set(&svm->refs, 0);
>                  ioasid_set_data(data->hpasid, svm);
>                  INIT_LIST_HEAD_RCU(&svm->devs);
>                  INIT_LIST_HEAD(&svm->list);
> 
>                  mmput(svm->mm);
>          }
>          sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
>          if (!sdev) {
>                  ret = -ENOMEM;
>                  goto out;
>          }
>          sdev->dev = dev;
>          sdev->users = 1;
> 
> Best regards,
> Baolu

[Jacob Pan]