[PATCH] pci, dmar: flush IOTLB before exit domain

[PATCH] pci, dmar: flush IOTLB before exit domain

[Yinghai Lu]

during one hotplug testing on system that support iommu/dmar.
got memory corruption.

[  578.279327] pci 0000:c4:00.0: no hotplug settings from platform
[  578.299240] scsi11 : Fusion MPT SAS Host
[  578.301033] mpt2sas2: mpt2sas_base_attach
[  578.302643] mpt2sas2: mpt2sas_base_map_resources
[  578.302797] BUG: Bad page state in process udevd  pfn:dffe23e
[  578.302801] page:ffffea030ff97d90 count:0 mapcount:-27 mapping:          (null) index:0xffff88dffe23eec0
[  578.302803] page flags: 0x1a00000000000000()
[  578.302807] Pid: 18215, comm: udevd Not tainted 2.6.39-rc5-tip-yh-03961-gced6a85-dirty #898
[  578.302809] Call Trace:
[  578.302825]  [<ffffffff81101528>] ? dump_page+0xbb/0xc0
[  578.302831]  [<ffffffff8110160a>] bad_page+0xdd/0xf2
[  578.302838]  [<ffffffff811023e6>] prep_new_page+0x70/0x141
[  578.302844]  [<ffffffff811028da>] get_page_from_freelist+0x423/0x59f
[  578.302851]  [<ffffffff81102c0a>] __alloc_pages_nodemask+0x1b4/0x7fe
[  578.302864]  [<ffffffff810a276a>] ? local_clock+0x2b/0x3c
[  578.302879]  [<ffffffff8111929e>] ? __pud_alloc+0x73/0x84
[  578.302885]  [<ffffffff810a276a>] ? local_clock+0x2b/0x3c
[  578.302896]  [<ffffffff8112d5a1>] alloc_pages_current+0xba/0xdd
[  578.302903]  [<ffffffff810ff774>] __get_free_pages+0xe/0x4b
[  578.302909]  [<ffffffff810ff7c7>] get_zeroed_page+0x16/0x18
[  578.302915]  [<ffffffff811192d1>] __pmd_alloc+0x22/0x85
[  578.302922]  [<ffffffff8111a6ad>] copy_page_range+0x238/0x3d8
[  578.302938]  [<ffffffff8107dd1b>] dup_mmap+0x2b9/0x375
[  578.302944]  [<ffffffff8107e3c5>] dup_mm+0xab/0x171
[  578.302951]  [<ffffffff8107eb99>] copy_process+0x6ea/0xd8e
[  578.302959]  [<ffffffff810b1a87>] ? __lock_release+0x166/0x16f
[  578.302965]  [<ffffffff8107f396>] do_fork+0x130/0x2dd
[  578.302976]  [<ffffffff811541c2>] ? mntput_no_expire+0x27/0xc8
[  578.302982]  [<ffffffff81154289>] ? mntput+0x26/0x28
[  578.302994]  [<ffffffff8113c429>] ? __fput+0x1b9/0x1c8
[  578.303004]  [<ffffffff81c2f69c>] ? sysret_check+0x27/0x62
[  578.303015]  [<ffffffff81040f41>] sys_clone+0x28/0x2a
[  578.303021]  [<ffffffff81c2f953>] stub_clone+0x13/0x20
[  578.303027]  [<ffffffff81c2f66b>] ? system_call_fastpath+0x16/0x1b

the bug is uncoverred by

| commit a97590e56d0d58e1dd262353f7cbd84e81d8e600
| Author: Alex Williamson <alex.williamson@redhat.com>
| Date:   Fri Mar 4 14:52:16 2011 -0700
|
|    intel-iommu: Unlink domain from iommu
|    
|    When we remove a device, we unlink the iommu from the domain, but
|    we never do the reverse unlinking of the domain from the iommu.
|    This means that we never clear iommu->domain_ids, eventually leading
|    to resource exhaustion if we repeatedly bind and unbind a device
|    to a driver.  Also free empty domains to avoid a resource leak.

that will remove domain really...
It exposes the problem that defer flushing is not handled properly during hot removing.

Try to flush unmaps before exit.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/intel-iommu.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/drivers/pci/intel-iommu.c
===================================================================
--- linux-2.6.orig/drivers/pci/intel-iommu.c
+++ linux-2.6/drivers/pci/intel-iommu.c
@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
 		return 0;
 
 	if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+		/* before we remove dev with domain, flush IOTLB */
+		flush_unmaps();
+
 		domain_remove_one_dev_info(domain, pdev);
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[David Woodhouse]

On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
> @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
>                 return 0;
>  
>         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
> +               /* before we remove dev with domain, flush IOTLB */
> +               flush_unmaps();
> +
>                 domain_remove_one_dev_info(domain, pdev);
>  
>                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 

That calls flush_unmaps() without the async_umap_flush_lock held,
doesn't it? A few days ago I asked someone else to test this candidate
patch for a similar issue:

http://david.woodhou.se/flush-unmaps-on-unbind.patch

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Yinghai Lu]

On 05/09/2011 07:48 AM, David Woodhouse wrote:
> On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
>> @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
>>                 return 0;
>>  
>>         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
>> +               /* before we remove dev with domain, flush IOTLB */
>> +               flush_unmaps();
>> +
>>                 domain_remove_one_dev_info(domain, pdev);
>>  
>>                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
> 
> That calls flush_unmaps() without the async_umap_flush_lock held,
> doesn't it? A few days ago I asked someone else to test this candidate
> patch for a similar issue:
> 
> http://david.woodhou.se/flush-unmaps-on-unbind.patch
> 

Your patch works.

Thanks

Yinghai

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Alex Williamson]

On Mon, 2011-05-09 at 15:48 +0100, David Woodhouse wrote:
> On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
> > @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
> >                 return 0;
> >  
> >         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
> > +               /* before we remove dev with domain, flush IOTLB */
> > +               flush_unmaps();
> > +
> >                 domain_remove_one_dev_info(domain, pdev);
> >  
> >                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
> 
> That calls flush_unmaps() without the async_umap_flush_lock held,
> doesn't it? A few days ago I asked someone else to test this candidate
> patch for a similar issue:
> 
> http://david.woodhou.se/flush-unmaps-on-unbind.patch

Copying here:

> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index d552d2c..7e606d6 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
>  
>  		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
>  		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
> -		    list_empty(&domain->devices))
> +		    list_empty(&domain->devices)) {
> +			flush_unmaps_timeout(0);
>  			domain_exit(domain);
> +		}
>  	}
>  
>  	return 0;
> @@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
>  	struct dmar_domain *dmar_domain = domain->priv;
>  
>  	domain->priv = NULL;
> +	flush_unmaps_timeout(0);
>  	vm_domain_exit(dmar_domain);
> }

David, would it be worthwhile to push the unmaps into the
{vm_}domain_exit() functions to avoid races like this in the future?  I
can verify the above resolves a panic after unbinding a device from
snd_hda_intel that I hit recently.  Do you plan to push this for .39?
Thanks,

Alex

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Alex Williamson]

On Mon, 2011-05-16 at 09:13 -0600, Alex Williamson wrote:
> On Mon, 2011-05-09 at 15:48 +0100, David Woodhouse wrote:
> > On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
> > > @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
> > >                 return 0;
> > >  
> > >         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
> > > +               /* before we remove dev with domain, flush IOTLB */
> > > +               flush_unmaps();
> > > +
> > >                 domain_remove_one_dev_info(domain, pdev);
> > >  
> > >                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
> > 
> > That calls flush_unmaps() without the async_umap_flush_lock held,
> > doesn't it? A few days ago I asked someone else to test this candidate
> > patch for a similar issue:
> > 
> > http://david.woodhou.se/flush-unmaps-on-unbind.patch
> 
> Copying here:
> 
> > diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> > index d552d2c..7e606d6 100644
> > --- a/drivers/pci/intel-iommu.c
> > +++ b/drivers/pci/intel-iommu.c
> > @@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
> >  
> >  		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
> >  		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
> > -		    list_empty(&domain->devices))
> > +		    list_empty(&domain->devices)) {
> > +			flush_unmaps_timeout(0);
> >  			domain_exit(domain);
> > +		}
> >  	}
> >  
> >  	return 0;
> > @@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
> >  	struct dmar_domain *dmar_domain = domain->priv;
> >  
> >  	domain->priv = NULL;
> > +	flush_unmaps_timeout(0);
> >  	vm_domain_exit(dmar_domain);
> > }
> 
> David, would it be worthwhile to push the unmaps into the
> {vm_}domain_exit() functions to avoid races like this in the future?  I
> can verify the above resolves a panic after unbinding a device from
> snd_hda_intel that I hit recently.  Do you plan to push this for .39?

BTW, is this second chunk really needed?  VM iommu mappings don't seem
to use the lazy unmap path.  Thanks,

Alex

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Alex Williamson]

On Tue, 2011-05-17 at 13:41 -0600, Alex Williamson wrote:
> On Mon, 2011-05-16 at 09:13 -0600, Alex Williamson wrote:
> > On Mon, 2011-05-09 at 15:48 +0100, David Woodhouse wrote:
> > > On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
> > > > @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
> > > >                 return 0;
> > > >  
> > > >         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
> > > > +               /* before we remove dev with domain, flush IOTLB */
> > > > +               flush_unmaps();
> > > > +
> > > >                 domain_remove_one_dev_info(domain, pdev);
> > > >  
> > > >                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
> > > 
> > > That calls flush_unmaps() without the async_umap_flush_lock held,
> > > doesn't it? A few days ago I asked someone else to test this candidate
> > > patch for a similar issue:
> > > 
> > > http://david.woodhou.se/flush-unmaps-on-unbind.patch
> > 
> > Copying here:
> > 
> > > diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> > > index d552d2c..7e606d6 100644
> > > --- a/drivers/pci/intel-iommu.c
> > > +++ b/drivers/pci/intel-iommu.c
> > > @@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
> > >  
> > >  		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
> > >  		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
> > > -		    list_empty(&domain->devices))
> > > +		    list_empty(&domain->devices)) {
> > > +			flush_unmaps_timeout(0);
> > >  			domain_exit(domain);
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
> > >  	struct dmar_domain *dmar_domain = domain->priv;
> > >  
> > >  	domain->priv = NULL;
> > > +	flush_unmaps_timeout(0);
> > >  	vm_domain_exit(dmar_domain);
> > > }
> > 
> > David, would it be worthwhile to push the unmaps into the
> > {vm_}domain_exit() functions to avoid races like this in the future?  I
> > can verify the above resolves a panic after unbinding a device from
> > snd_hda_intel that I hit recently.  Do you plan to push this for .39?
> 
> BTW, is this second chunk really needed?  VM iommu mappings don't seem
> to use the lazy unmap path.  Thanks,

David, what do you think of this instead?  Thanks,

Alex

intel-iommu: Flush unmaps at domain_exit

From: Alex Williamson <alex.williamson@redhat.com>

We typically batch unmaps to be lazily flushed out at
regular intervals.  When we destroy a domain, we need
to force a flush of these lazy unmaps to be sure none
reference the domain we're about to free.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/intel-iommu.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index d552d2c..b04f84e 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -1416,6 +1416,10 @@ static void domain_exit(struct dmar_domain *domain)
 	if (!domain)
 		return;
 
+	/* Flush any lazy unmaps that may reference this domain */
+	if (!intel_iommu_strict)
+		flush_unmaps_timeout(0);
+
 	domain_remove_dev_info(domain);
 	/* destroy iovas */
 	put_iova_domain(&domain->iovad);

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]

On 2011-05-18 03:57, Alex Williamson wrote:
> On Tue, 2011-05-17 at 13:41 -0600, Alex Williamson wrote:
>> On Mon, 2011-05-16 at 09:13 -0600, Alex Williamson wrote:
>>> On Mon, 2011-05-09 at 15:48 +0100, David Woodhouse wrote:
>>>> On Thu, 2011-05-05 at 18:13 -0700, Yinghai Lu wrote:
>>>>> @@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
>>>>>                 return 0;
>>>>>  
>>>>>         if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
>>>>> +               /* before we remove dev with domain, flush IOTLB */
>>>>> +               flush_unmaps();
>>>>> +
>>>>>                 domain_remove_one_dev_info(domain, pdev);
>>>>>  
>>>>>                 if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
>>>>
>>>> That calls flush_unmaps() without the async_umap_flush_lock held,
>>>> doesn't it? A few days ago I asked someone else to test this candidate
>>>> patch for a similar issue:
>>>>
>>>> http://david.woodhou.se/flush-unmaps-on-unbind.patch
>>>
>>> Copying here:
>>>
>>>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
>>>> index d552d2c..7e606d6 100644
>>>> --- a/drivers/pci/intel-iommu.c
>>>> +++ b/drivers/pci/intel-iommu.c
>>>> @@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
>>>>  
>>>>  		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
>>>>  		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
>>>> -		    list_empty(&domain->devices))
>>>> +		    list_empty(&domain->devices)) {
>>>> +			flush_unmaps_timeout(0);
>>>>  			domain_exit(domain);
>>>> +		}
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> @@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
>>>>  	struct dmar_domain *dmar_domain = domain->priv;
>>>>  
>>>>  	domain->priv = NULL;
>>>> +	flush_unmaps_timeout(0);
>>>>  	vm_domain_exit(dmar_domain);
>>>> }
>>>
>>> David, would it be worthwhile to push the unmaps into the
>>> {vm_}domain_exit() functions to avoid races like this in the future?  I
>>> can verify the above resolves a panic after unbinding a device from
>>> snd_hda_intel that I hit recently.  Do you plan to push this for .39?
>>
>> BTW, is this second chunk really needed?  VM iommu mappings don't seem
>> to use the lazy unmap path.  Thanks,
> 
> David, what do you think of this instead?  Thanks,
> 
> Alex
> 
> intel-iommu: Flush unmaps at domain_exit
> 
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> We typically batch unmaps to be lazily flushed out at
> regular intervals.  When we destroy a domain, we need
> to force a flush of these lazy unmaps to be sure none
> reference the domain we're about to free.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  drivers/pci/intel-iommu.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index d552d2c..b04f84e 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -1416,6 +1416,10 @@ static void domain_exit(struct dmar_domain *domain)
>  	if (!domain)
>  		return;
>  
> +	/* Flush any lazy unmaps that may reference this domain */
> +	if (!intel_iommu_strict)
> +		flush_unmaps_timeout(0);
> +
>  	domain_remove_dev_info(domain);
>  	/* destroy iovas */
>  	put_iova_domain(&domain->iovad);
> 
> 

Can we get this resolved for 2.6.39.1? Via David's patch, or Alex's
(which works for me), or whatever. It's a really annoying regression of
.39 when you want to run with IOMMU enabled.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Woodhouse, David]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Tue, 2011-05-17 at 19:57 -0600, Alex Williamson wrote:
> 
> David, what do you think of this instead?  Thanks,

Yes, that makes more sense. Thanks.

Sorry for not getting this in before the 2.6.39 release. I'm merging it
now (with Cc: stable) and will push to Linus in a couple of days.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6242 bytes --]

Re: [PATCH] pci, dmar: flush IOTLB before exit domain

[Alex Williamson]

On Tue, 2011-05-24 at 12:04 +0100, Woodhouse, David wrote:
> On Tue, 2011-05-17 at 19:57 -0600, Alex Williamson wrote:
> > 
> > David, what do you think of this instead?  Thanks,
> 
> Yes, that makes more sense. Thanks.
> 
> Sorry for not getting this in before the 2.6.39 release. I'm merging it
> now (with Cc: stable) and will push to Linus in a couple of days.

Thanks David.  Please also review and include the domain unlinking patch
I just sent.  It should go in stable too.  Thanks,

Alex