Re: [PATCH v3] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU

[Malcolm Crossley <malcolm.crossley@citrix.com>]

On 18/06/14 11:18, Jan Beulich wrote:
>>>> Malcolm Crossley <malcolm.crossley@citrix.com> 06/17/14 6:15 PM >>>
>> On 17/06/14 16:06, Jan Beulich wrote:
>>>>>> On 17.06.14 at 16:37, <malcolm.crossley@citrix.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/ats.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
>>>> @@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
>>>>      {
>>>>          sid = (pdev->bus << 8) | pdev->devfn;
>>>>  
>>>> +        /* Only invalidate devices that belong to this IOMMU */
>>>> +        if ( !pdev->iommu || pdev->iommu != iommu )
>>>> +            continue;
>>>
>>> I meant to ask before and then forgot: What is the first half of this
>>> condition good/needed for?
>>>
>> Defensive coding to prevent a NULL pointer deference if the Intel or
>> core ATS code goes wrong.
> 
> But there is no de-reference of the pointer anywhere - you added it just as
> a token for comparison purposes. If pdev->iommu was NULL, the right side
> comparison should still produce "false"...
> 

Your right, I'm going crazy and thinking I'm dereferencing the IOMMU
pointer when I'm not. (A non submitted version of the code was looking
at the index field of struct IOMMU) Do you want me to resubmit an
updated patch or are you happy to fix it up on commit?

Malcolm

> Jan
>