Re: [bug report] vfio: Can't find phys by iova in vfio_unmap_unpin()

[Alex Williamson <alex.williamson@redhat.com>]

[cc +iommu]

On Tue, 11 Jun 2019 20:40:41 +0800
Jiangyiwen <jiangyiwen@huawei.com> wrote:

> Hi Alex,
> 
> I found this problem is not very easy to solve, for
> now, in arm64 platform, the "0" physical address
> is a valid system memory address, so in function
> arm_smmu_iova_to_phys() I think it should not use
> "0" as abnormal return value.
> 
> Do you have any idea?

I think you're going to need to redefine iommu_iova_to_phys() and fix
all the IOMMU implementations of it to comply.  Currently AMD and Intel
IOMMU driver return zero if a mapping is not found.  You could make the
function return 0/errno and return the physical address via a pointer
arg.  You could also keep the existing definition, but introduce a test
for a valid result that might use an architecture specific value (akin
to IS_ERR()).  You could also just reserve the zero page from userspace
allocation.  I really don't want #ifdef in the vfio iommu driver trying
to discern the correct invalid value though.  Thanks,

Alex

> On 2019/6/11 11:21, jiangyiwen wrote:
> > On 2019/5/21 3:28, Alex Williamson wrote:  
> >> On Mon, 20 May 2019 15:50:11 +0800
> >> jiangyiwen <jiangyiwen@huawei.com> wrote:
> >>  
> >>> Hello alex,
> >>>
> >>> We test a call trace as follows use ARM64 architecture,
> >>> it prints a WARN_ON() when find not physical address by
> >>> iova in vfio_unmap_unpin(), I can't find the cause of
> >>> problem now, do you have any ideas?  
> >> Is it reproducible?  Can you explain how to reproduce it?  The stack
> >> trace indicates a KVM VM is being shutdown and we're trying to clean
> >> out the IOMMU mappings from the domain and find a page that we think
> >> should be mapped that the IOMMU doesn't have mapped.  What device(s) was
> >> assigned to the VM?  This could be an IOMMU driver bug or a
> >> vfio_iommu_type1 bug.  Have you been able to reproduce this on other
> >> platforms?
> >>  
> > Hello Alex,
> >
> > Sorry to reply you so late because of some things,
> > this problem's reason is in some platform (like ARM64),
> > the "0" physical address is valid and can be used for
> > system memory, so in this case it should not print a
> > WARN_ON() and continue, we should unmap and unpin this
> > "0" physical address in these platform.
> >
> > So I want to return FFFFFFFFFFFFFFFFL instead of "0" as invalid
> > physical address in function iommu_iova_to_phys(). Do you think
> > it's appropriate?
> >
> > Thanks,
> > Yiwen.
> >  
> >>> In addition, I want to know why there is a WARN_ON() instead
> >>> of BUG_ON()? Does it affect the follow-up process?  
> >> We're removing an IOMMU page mapping entry and find that it's not
> >> present, so ultimately the effect at the IOMMU is the same, there's no
> >> mapping at that address, but I can't say without further analysis
> >> whether that means a page remains pinned or if that inconsistency was
> >> resolved previously elsewhere.  We WARN_ON because this is not what we
> >> expect, but potentially leaking a page of memory doesn't seem worthy of
> >> crashing the host, nor would a crash dump at that point necessarily aid
> >> in resolving the missing page as it potentially occurred well in the
> >> past.  Thanks,
> >>
> >> Alex
> >>
> >> .
> >>  
> >
> >
> > .
> >  
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu