Re: [Xen-devel] [PATCH] xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: oleksandr_tyshchenko@epam.com,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH] xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs
Date: Tue, 20 Aug 2019 18:04:57 +0300	[thread overview]
Message-ID: <1ec280ed-bc3f-9334-ac8d-833b3c323c5d@gmail.com> (raw)
In-Reply-To: <20190812202735.23411-1-julien.grall@arm.com>

On 12.08.19 23:27, Julien Grall wrote:

Hi, Julien

> When freeing a p2m entry, all the sub-tree behind it will also be freed.
> This may include intermediate page-tables or any l3 entry requiring to
> drop a reference (e.g for foreign pages). As soon as pages are freed,
> they may be re-used by Xen or another domain. Therefore it is necessary
> to flush *all* the TLBs beforehand.
>
> While CPU TLBs will be flushed before freeing the pages, this is not
> the case for IOMMU TLBs. This can be solved by moving the IOMMU TLBs
> flush earlier in the code.
>
> This wasn't considered as a security issue as device passthrough on Arm
> is not security supported.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> Cc: olekstysh@gmail.com
> Cc: oleksandr_tyshchenko@epam.com
>
>      I discovered it while looking at the code, so I don't have any
>      reproducer of the issue. There is a small windows where page could
>      be reallocated to Xen or another domain but still present in the
>      IOMMU TLBs.

I haven't reproduced this issue as well.

So, my testing here is rather formal to be sure that patch doesn't break 
anything.

You can add (if needed):

Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

>      This patch only address the case where the flush succeed. In the
>      unlikely case where it does not succeed, then we will still free the
>      pages. The IOMMU helper will crash domain, but the device may still
>      not be quiescent. So there are a potentially issues do DMA on wrong
>      things.
>
>      At the moment, none of the Arm IOMMUs drivers (including the IPMMU
>      one under review) are return an error here. Note that flush may
>      still fail (see timeout), but is ignored. This is not great as it
>      means a device may DMA into something that does not belong to the
>      domain. So we probably want to return an error here.

Makes sense.

[I haven't been facing flush timeout issue since start playing with 
IPMMU...]

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel