Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
xen_swiotlb_free_coherent" only fixed memory address check condition
on xen_swiotlb_free_coherent(), when memory was not physically
contiguous and tried to exchanged with Xen via
xen_destroy_contiguous_region it will lead kernel panic.
The correct check condition should be memory is in DMA area and
physically contiguous.
Thank you Boris for pointing it out.
Signed-off-by: Joe Jin <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Christoph Helwig <[email protected]>
Cc: Dongli Zhang <[email protected]>
Cc: John Sobecki <[email protected]>
---
drivers/xen/swiotlb-xen.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index f5c1af4ce9ab..aed92fa019f9 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
- if (((dev_addr + size - 1 <= dma_mask)) ||
- range_straddles_page_boundary(phys, size))
+ if ((dev_addr + size - 1 <= dma_mask) &&
+ !range_straddles_page_boundary(phys, size))
xen_destroy_contiguous_region(phys, order);
xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
--
2.17.1 (Apple Git-112)
On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
> xen_swiotlb_free_coherent" only fixed memory address check condition
> on xen_swiotlb_free_coherent(), when memory was not physically
> contiguous and tried to exchanged with Xen via
> xen_destroy_contiguous_region it will lead kernel panic.
s/it will lead/which lead to/?
>
> The correct check condition should be memory is in DMA area and
> physically contiguous.
"The correct check condition to make Xen hypercall to revert the
memory back from its 32-bit pool is if it is:
1) Above its DMA bit mask (for example 32-bit devices can only address
up to 4GB, and we may want 4GB+2K), and
2) If it not physically contingous
N.B. The logic in the code is inverted, which leads to all sorts of
confusions."
Does that sound correct?
>
> Thank you Boris for pointing it out.
>
Fixes: 4855c92dbb7 ("xen-sw..") ?
> Signed-off-by: Joe Jin <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Boris Ostrovsky <[email protected]>
Reported-by: Boris Ostrovs... ?
> Cc: Christoph Helwig <[email protected]>
> Cc: Dongli Zhang <[email protected]>
> Cc: John Sobecki <[email protected]>
> ---
> drivers/xen/swiotlb-xen.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index f5c1af4ce9ab..aed92fa019f9 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> /* Convert the size to actually allocated. */
> size = 1UL << (order + XEN_PAGE_SHIFT);
>
> - if (((dev_addr + size - 1 <= dma_mask)) ||
> - range_straddles_page_boundary(phys, size))
> + if ((dev_addr + size - 1 <= dma_mask) &&
> + !range_straddles_page_boundary(phys, size))
> xen_destroy_contiguous_region(phys, order);
>
> xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
> --
> 2.17.1 (Apple Git-112)
>
On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>> xen_swiotlb_free_coherent" only fixed memory address check condition
>> on xen_swiotlb_free_coherent(), when memory was not physically
>> contiguous and tried to exchanged with Xen via
>> xen_destroy_contiguous_region it will lead kernel panic.
> s/it will lead/which lead to/?
>
>> The correct check condition should be memory is in DMA area and
>> physically contiguous.
> "The correct check condition to make Xen hypercall to revert the
> memory back from its 32-bit pool is if it is:
> 1) Above its DMA bit mask (for example 32-bit devices can only address
> up to 4GB, and we may want 4GB+2K), and
Is this "and' or 'or'?
> 2) If it not physically contingous
>
> N.B. The logic in the code is inverted, which leads to all sorts of
> confusions."
I would, in fact, suggest to make the logic the same in both
xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
this. This will involve swapping if and else in the former.
>
> Does that sound correct?
>
>> Thank you Boris for pointing it out.
>>
> Fixes: 4855c92dbb7 ("xen-sw..") ?
>
>> Signed-off-by: Joe Jin <[email protected]>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Cc: Boris Ostrovsky <[email protected]>
> Reported-by: Boris Ostrovs... ?
>> Cc: Christoph Helwig <[email protected]>
>> Cc: Dongli Zhang <[email protected]>
>> Cc: John Sobecki <[email protected]>
>> ---
>> drivers/xen/swiotlb-xen.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> index f5c1af4ce9ab..aed92fa019f9 100644
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>> /* Convert the size to actually allocated. */
>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>
>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>> - range_straddles_page_boundary(phys, size))
>> + if ((dev_addr + size - 1 <= dma_mask) &&
>> + !range_straddles_page_boundary(phys, size))
>> xen_destroy_contiguous_region(phys, order);
I don't think this is right.
if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
No?
-boris
>>
>> xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
>> --
>> 2.17.1 (Apple Git-112)
>>
On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>> contiguous and tried to exchanged with Xen via
>>> xen_destroy_contiguous_region it will lead kernel panic.
>> s/it will lead/which lead to/?
>>
>>> The correct check condition should be memory is in DMA area and
>>> physically contiguous.
>> "The correct check condition to make Xen hypercall to revert the
>> memory back from its 32-bit pool is if it is:
>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>> up to 4GB, and we may want 4GB+2K), and
>
> Is this "and' or 'or'?
>
>> 2) If it not physically contingous
>>
>> N.B. The logic in the code is inverted, which leads to all sorts of
>> confusions."
>
>
> I would, in fact, suggest to make the logic the same in both
> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
> this. This will involve swapping if and else in the former.
>
>
>>
>> Does that sound correct?
>>
>>> Thank you Boris for pointing it out.
>>>
>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>
>>> Signed-off-by: Joe Jin <[email protected]>
>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>> Cc: Boris Ostrovsky <[email protected]>
>> Reported-by: Boris Ostrovs... ?
>>> Cc: Christoph Helwig <[email protected]>
>>> Cc: Dongli Zhang <[email protected]>
>>> Cc: John Sobecki <[email protected]>
>>> ---
>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>> --- a/drivers/xen/swiotlb-xen.c
>>> +++ b/drivers/xen/swiotlb-xen.c
>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>> /* Convert the size to actually allocated. */
>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>
>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>> - range_straddles_page_boundary(phys, size))
>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>> + !range_straddles_page_boundary(phys, size))
>>> xen_destroy_contiguous_region(phys, order);
>
>
> I don't think this is right.
>
> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>
> No?
No this is not correct.
When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
326 phys = *dma_handle;
327 dev_addr = xen_phys_to_bus(phys);
328 if (((dev_addr + size - 1 <= dma_mask)) &&
329 !range_straddles_page_boundary(phys, size))
330 *dma_handle = dev_addr;
331 else {
332 if (xen_create_contiguous_region(phys, order,
333 fls64(dma_mask), dma_handle) != 0) {
334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
335 return NULL;
336 }
337 }
On freeing, need to return the memory to Xen, otherwise DMA memory will be used
up(this is the issue the patch intend to fix), so when memory is DMAable and
contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
Thanks,
Joe
On 10/24/18 6:02 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>> xen_swiotlb_free_coherent" only fixed memory address check condition
>> on xen_swiotlb_free_coherent(), when memory was not physically
>> contiguous and tried to exchanged with Xen via
>> xen_destroy_contiguous_region it will lead kernel panic.
>
> s/it will lead/which lead to/?
>
>>
>> The correct check condition should be memory is in DMA area and
>> physically contiguous.
>
> "The correct check condition to make Xen hypercall to revert the
> memory back from its 32-bit pool is if it is:
> 1) Above its DMA bit mask (for example 32-bit devices can only address
> up to 4GB, and we may want 4GB+2K), and
> 2) If it not physically contingous
Here should be physically contiguous? on allocating, it asked physically
contiguous memory for DMA, if not contiguous, it can not do DMA,
am I right?
Andy range_straddles_page_boundary() return 0 means is physically contiguous,
is that correct?
Thanks,
Joe
>
> N.B. The logic in the code is inverted, which leads to all sorts of
> confusions."
>
> Does that sound correct?
>
>>
>> Thank you Boris for pointing it out.
>>
>
> Fixes: 4855c92dbb7 ("xen-sw..") ?
>
>> Signed-off-by: Joe Jin <[email protected]>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Cc: Boris Ostrovsky <[email protected]>
>
> Reported-by: Boris Ostrovs... ?
>> Cc: Christoph Helwig <[email protected]>
>> Cc: Dongli Zhang <[email protected]>
>> Cc: John Sobecki <[email protected]>
>> ---
>> drivers/xen/swiotlb-xen.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> index f5c1af4ce9ab..aed92fa019f9 100644
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>> /* Convert the size to actually allocated. */
>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>
>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>> - range_straddles_page_boundary(phys, size))
>> + if ((dev_addr + size - 1 <= dma_mask) &&
>> + !range_straddles_page_boundary(phys, size))
>> xen_destroy_contiguous_region(phys, order);
>>
>> xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
>> --
>> 2.17.1 (Apple Git-112)
>>
On 10/24/18 10:43 AM, Joe Jin wrote:
> On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
>> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>>> contiguous and tried to exchanged with Xen via
>>>> xen_destroy_contiguous_region it will lead kernel panic.
>>> s/it will lead/which lead to/?
>>>
>>>> The correct check condition should be memory is in DMA area and
>>>> physically contiguous.
>>> "The correct check condition to make Xen hypercall to revert the
>>> memory back from its 32-bit pool is if it is:
>>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>>> up to 4GB, and we may want 4GB+2K), and
>> Is this "and' or 'or'?
>>
>>> 2) If it not physically contingous
>>>
>>> N.B. The logic in the code is inverted, which leads to all sorts of
>>> confusions."
>>
>> I would, in fact, suggest to make the logic the same in both
>> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
>> this. This will involve swapping if and else in the former.
>>
>>
>>> Does that sound correct?
>>>
>>>> Thank you Boris for pointing it out.
>>>>
>>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>>
>>>> Signed-off-by: Joe Jin <[email protected]>
>>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>>> Cc: Boris Ostrovsky <[email protected]>
>>> Reported-by: Boris Ostrovs... ?
>>>> Cc: Christoph Helwig <[email protected]>
>>>> Cc: Dongli Zhang <[email protected]>
>>>> Cc: John Sobecki <[email protected]>
>>>> ---
>>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>>> --- a/drivers/xen/swiotlb-xen.c
>>>> +++ b/drivers/xen/swiotlb-xen.c
>>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>>> /* Convert the size to actually allocated. */
>>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>>
>>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>>> - range_straddles_page_boundary(phys, size))
>>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>>> + !range_straddles_page_boundary(phys, size))
>>>> xen_destroy_contiguous_region(phys, order);
>>
>> I don't think this is right.
>>
>> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>>
>> No?
> No this is not correct.
>
> When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
> address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
>
> 326 phys = *dma_handle;
> 327 dev_addr = xen_phys_to_bus(phys);
> 328 if (((dev_addr + size - 1 <= dma_mask)) &&
> 329 !range_straddles_page_boundary(phys, size))
> 330 *dma_handle = dev_addr;
> 331 else {
> 332 if (xen_create_contiguous_region(phys, order,
> 333 fls64(dma_mask), dma_handle) != 0) {
> 334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
> 335 return NULL;
> 336 }
> 337 }
>
>
> On freeing, need to return the memory to Xen, otherwise DMA memory will be used
> up(this is the issue the patch intend to fix), so when memory is DMAable and
> contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
So if you want to allocate 1 byte at address 0 (and dev_addr=phys),
xen_create_contiguous_region() will not be called. And yet you will call
xen_destroy_contiguous_region() in the free path.
Is this the expected behavior?
-boris
On 10/25/18 4:45 AM, Boris Ostrovsky wrote:
> On 10/24/18 10:43 AM, Joe Jin wrote:
>> On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
>>> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>>>> contiguous and tried to exchanged with Xen via
>>>>> xen_destroy_contiguous_region it will lead kernel panic.
>>>> s/it will lead/which lead to/?
>>>>
>>>>> The correct check condition should be memory is in DMA area and
>>>>> physically contiguous.
>>>> "The correct check condition to make Xen hypercall to revert the
>>>> memory back from its 32-bit pool is if it is:
>>>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>>>> up to 4GB, and we may want 4GB+2K), and
>>> Is this "and' or 'or'?
>>>
>>>> 2) If it not physically contingous
>>>>
>>>> N.B. The logic in the code is inverted, which leads to all sorts of
>>>> confusions."
>>>
>>> I would, in fact, suggest to make the logic the same in both
>>> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
>>> this. This will involve swapping if and else in the former.
>>>
>>>
>>>> Does that sound correct?
>>>>
>>>>> Thank you Boris for pointing it out.
>>>>>
>>>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>>>
>>>>> Signed-off-by: Joe Jin <[email protected]>
>>>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>>>> Cc: Boris Ostrovsky <[email protected]>
>>>> Reported-by: Boris Ostrovs... ?
>>>>> Cc: Christoph Helwig <[email protected]>
>>>>> Cc: Dongli Zhang <[email protected]>
>>>>> Cc: John Sobecki <[email protected]>
>>>>> ---
>>>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>>>> --- a/drivers/xen/swiotlb-xen.c
>>>>> +++ b/drivers/xen/swiotlb-xen.c
>>>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>>>> /* Convert the size to actually allocated. */
>>>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>>>
>>>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>>>> - range_straddles_page_boundary(phys, size))
>>>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>>>> + !range_straddles_page_boundary(phys, size))
>>>>> xen_destroy_contiguous_region(phys, order);
>>>
>>> I don't think this is right.
>>>
>>> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>>>
>>> No?
>> No this is not correct.
>>
>> When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
>> address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
>>
>> 326 phys = *dma_handle;
>> 327 dev_addr = xen_phys_to_bus(phys);
>> 328 if (((dev_addr + size - 1 <= dma_mask)) &&
>> 329 !range_straddles_page_boundary(phys, size))
>> 330 *dma_handle = dev_addr;
>> 331 else {
>> 332 if (xen_create_contiguous_region(phys, order,
>> 333 fls64(dma_mask), dma_handle) != 0) {
>> 334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
>> 335 return NULL;
>> 336 }
>> 337 }
>>
>>
>> On freeing, need to return the memory to Xen, otherwise DMA memory will be used
>> up(this is the issue the patch intend to fix), so when memory is DMAable and
>> contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
>
> So if you want to allocate 1 byte at address 0 (and dev_addr=phys),
> xen_create_contiguous_region() will not be called. And yet you will call
> xen_destroy_contiguous_region() in the free path.
>
> Is this the expected behavior?
I could not say it's expected behavior, but I think it's reasonable.
On allocating, it used __get_free_pages() to allocate memory, if lucky the memory is
DMAable, will not exchange memory with hypervisor, obviously this is not guaranteed.
And on freeing it could not be identified if memory from Dom0/guest own memory
or hypervisor, if don't back memory to hypervisor which will lead hypervisor DMA
memory be used up, then on Dom0/guest, DMA request maybe failed, the worse thing is
could not start any new guest.
Thanks,
Joe
>
> -boris
>
On 10/25/18 10:23 AM, Joe Jin wrote:
> On 10/25/18 4:45 AM, Boris Ostrovsky wrote:
>> On 10/24/18 10:43 AM, Joe Jin wrote:
>>> On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
>>>> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>>>>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>>>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>>>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>>>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>>>>> contiguous and tried to exchanged with Xen via
>>>>>> xen_destroy_contiguous_region it will lead kernel panic.
>>>>> s/it will lead/which lead to/?
>>>>>
>>>>>> The correct check condition should be memory is in DMA area and
>>>>>> physically contiguous.
>>>>> "The correct check condition to make Xen hypercall to revert the
>>>>> memory back from its 32-bit pool is if it is:
>>>>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>>>>> up to 4GB, and we may want 4GB+2K), and
>>>> Is this "and' or 'or'?
>>>>
>>>>> 2) If it not physically contingous
>>>>>
>>>>> N.B. The logic in the code is inverted, which leads to all sorts of
>>>>> confusions."
>>>> I would, in fact, suggest to make the logic the same in both
>>>> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
>>>> this. This will involve swapping if and else in the former.
>>>>
>>>>
>>>>> Does that sound correct?
>>>>>
>>>>>> Thank you Boris for pointing it out.
>>>>>>
>>>>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>>>>
>>>>>> Signed-off-by: Joe Jin <[email protected]>
>>>>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>>>>> Cc: Boris Ostrovsky <[email protected]>
>>>>> Reported-by: Boris Ostrovs... ?
>>>>>> Cc: Christoph Helwig <[email protected]>
>>>>>> Cc: Dongli Zhang <[email protected]>
>>>>>> Cc: John Sobecki <[email protected]>
>>>>>> ---
>>>>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>>>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>>>>> --- a/drivers/xen/swiotlb-xen.c
>>>>>> +++ b/drivers/xen/swiotlb-xen.c
>>>>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>>>>> /* Convert the size to actually allocated. */
>>>>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>>>>
>>>>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>>>>> - range_straddles_page_boundary(phys, size))
>>>>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>>>>> + !range_straddles_page_boundary(phys, size))
>>>>>> xen_destroy_contiguous_region(phys, order);
>>>> I don't think this is right.
>>>>
>>>> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>>>>
>>>> No?
>>> No this is not correct.
>>>
>>> When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
>>> address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
>>>
>>> 326 phys = *dma_handle;
>>> 327 dev_addr = xen_phys_to_bus(phys);
>>> 328 if (((dev_addr + size - 1 <= dma_mask)) &&
>>> 329 !range_straddles_page_boundary(phys, size))
>>> 330 *dma_handle = dev_addr;
>>> 331 else {
>>> 332 if (xen_create_contiguous_region(phys, order,
>>> 333 fls64(dma_mask), dma_handle) != 0) {
>>> 334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
>>> 335 return NULL;
>>> 336 }
>>> 337 }
>>>
>>>
>>> On freeing, need to return the memory to Xen, otherwise DMA memory will be used
>>> up(this is the issue the patch intend to fix), so when memory is DMAable and
>>> contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
>> So if you want to allocate 1 byte at address 0 (and dev_addr=phys),
>> xen_create_contiguous_region() will not be called. And yet you will call
>> xen_destroy_contiguous_region() in the free path.
>>
>> Is this the expected behavior?
> I could not say it's expected behavior, but I think it's reasonable.
I would expect xen_create_contiguous_region() and
xen_destroy_contiguous_region() to come in pairs. If a region is
created, it needs to be destroyed. And vice versa.
>
> On allocating, it used __get_free_pages() to allocate memory, if lucky the memory is
> DMAable, will not exchange memory with hypervisor, obviously this is not guaranteed.
>
> And on freeing it could not be identified if memory from Dom0/guest own memory
> or hypervisor
I think it can be. if (!(dev_addr + size - 1 <= dma_mask) ||
range_straddles_page_boundary()) then it must have come from the
hypervisor, because that's the check we make in
xen_swiotlb_alloc_coherent().
-boris
> , if don't back memory to hypervisor which will lead hypervisor DMA
> memory be used up, then on Dom0/guest, DMA request maybe failed, the worse thing is
> could not start any new guest.
>
> Thanks,
> Joe
>
>> -boris
>>
On 10/25/18 9:10 AM, Boris Ostrovsky wrote:
> On 10/25/18 10:23 AM, Joe Jin wrote:
>> On 10/25/18 4:45 AM, Boris Ostrovsky wrote:
>>> On 10/24/18 10:43 AM, Joe Jin wrote:
>>>> On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
>>>>> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>>>>>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>>>>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>>>>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>>>>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>>>>>> contiguous and tried to exchanged with Xen via
>>>>>>> xen_destroy_contiguous_region it will lead kernel panic.
>>>>>> s/it will lead/which lead to/?
>>>>>>
>>>>>>> The correct check condition should be memory is in DMA area and
>>>>>>> physically contiguous.
>>>>>> "The correct check condition to make Xen hypercall to revert the
>>>>>> memory back from its 32-bit pool is if it is:
>>>>>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>>>>>> up to 4GB, and we may want 4GB+2K), and
>>>>> Is this "and' or 'or'?
>>>>>
>>>>>> 2) If it not physically contingous
>>>>>>
>>>>>> N.B. The logic in the code is inverted, which leads to all sorts of
>>>>>> confusions."
>>>>> I would, in fact, suggest to make the logic the same in both
>>>>> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
>>>>> this. This will involve swapping if and else in the former.
>>>>>
>>>>>
>>>>>> Does that sound correct?
>>>>>>
>>>>>>> Thank you Boris for pointing it out.
>>>>>>>
>>>>>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>>>>>
>>>>>>> Signed-off-by: Joe Jin <[email protected]>
>>>>>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>>>>>> Cc: Boris Ostrovsky <[email protected]>
>>>>>> Reported-by: Boris Ostrovs... ?
>>>>>>> Cc: Christoph Helwig <[email protected]>
>>>>>>> Cc: Dongli Zhang <[email protected]>
>>>>>>> Cc: John Sobecki <[email protected]>
>>>>>>> ---
>>>>>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>>>>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>>>>>> --- a/drivers/xen/swiotlb-xen.c
>>>>>>> +++ b/drivers/xen/swiotlb-xen.c
>>>>>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>>>>>> /* Convert the size to actually allocated. */
>>>>>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>>>>>
>>>>>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>>>>>> - range_straddles_page_boundary(phys, size))
>>>>>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>>>>>> + !range_straddles_page_boundary(phys, size))
>>>>>>> xen_destroy_contiguous_region(phys, order);
>>>>> I don't think this is right.
>>>>>
>>>>> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>>>>>
>>>>> No?
>>>> No this is not correct.
>>>>
>>>> When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
>>>> address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
>>>>
>>>> 326 phys = *dma_handle;
>>>> 327 dev_addr = xen_phys_to_bus(phys);
>>>> 328 if (((dev_addr + size - 1 <= dma_mask)) &&
>>>> 329 !range_straddles_page_boundary(phys, size))
>>>> 330 *dma_handle = dev_addr;
>>>> 331 else {
>>>> 332 if (xen_create_contiguous_region(phys, order,
>>>> 333 fls64(dma_mask), dma_handle) != 0) {
>>>> 334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
>>>> 335 return NULL;
>>>> 336 }
>>>> 337 }
>>>>
>>>>
>>>> On freeing, need to return the memory to Xen, otherwise DMA memory will be used
>>>> up(this is the issue the patch intend to fix), so when memory is DMAable and
>>>> contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
>>> So if you want to allocate 1 byte at address 0 (and dev_addr=phys),
>>> xen_create_contiguous_region() will not be called. And yet you will call
>>> xen_destroy_contiguous_region() in the free path.
>>>
>>> Is this the expected behavior?
>> I could not say it's expected behavior, but I think it's reasonable.
>
> I would expect xen_create_contiguous_region() and
> xen_destroy_contiguous_region() to come in pairs. If a region is
> created, it needs to be destroyed. And vice versa.
>
>
>>
>> On allocating, it used __get_free_pages() to allocate memory, if lucky the memory is
>> DMAable, will not exchange memory with hypervisor, obviously this is not guaranteed.
>>
>> And on freeing it could not be identified if memory from Dom0/guest own memory
>> or hypervisor
>
>
> I think it can be. if (!(dev_addr + size - 1 <= dma_mask) ||
> range_straddles_page_boundary()) then it must have come from the
> hypervisor, because that's the check we make in
> xen_swiotlb_alloc_coherent().
This is not true.
dev_addr was came from dma_handle, *dma_handle will be changed after called
xen_create_contiguous_region():
2590 int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
2591 unsigned int address_bits,
2592 dma_addr_t *dma_handle)
2593 {
......
2617 success = xen_exchange_memory(1UL << order, 0, in_frames,
2618 1, order, &out_frame,
2619 address_bits);
2620
2621 /* 3. Map the new extent in place of old pages. */
2622 if (success)
2623 xen_remap_exchanged_ptes(vstart, order, NULL, out_frame);
2624 else
2625 xen_remap_exchanged_ptes(vstart, order, in_frames, 0);
2626
2627 spin_unlock_irqrestore(&xen_reservation_lock, flags);
2628
2629 *dma_handle = virt_to_machine(vstart).maddr;
2630 return success ? 0 : -ENOMEM;
2631 }
So means dev_addr check on xen_swiotlb_alloc_coherent() is not same one on
xen_swiotlb_free_coherent().
Thanks,
Joe
>
>
> -boris
>
>
>> , if don't back memory to hypervisor which will lead hypervisor DMA
>> memory be used up, then on Dom0/guest, DMA request maybe failed, the worse thing is
>> could not start any new guest.
>>
>> Thanks,
>> Joe
>>
>>> -boris
>>>
>
Hi all,
I just discussed this patch with Boris in private, his opinions(Boris,
please correct me if any misunderstood) are:
1. With/without the check, both are incorrect, he thought we need to
prevented unalloc'd free at here.
2. On freeing, if upper layer already checked the memory was DMA-able,
the checking at here does not make sense, we can remove all checks.
3. xen_create_contiguous_region() and xen_destroy_contiguous_region()
to come in pairs.
For #1 and #3, I think we need something associate it, like a list, on
allocating, add addr to it, on freeing, check if in the list.
For #2, I'm was not found anywhere validated the address on
dma_free_coherent() callpath, not just xen-swiotlb.
From my side, I think the checks are make sense, it prevented to exchange
non-contiguous memory with Xen also make sure Xen has enough DMA memory
for DMA also for guest creation. I'm not sure if we can merge this patch
to avoid exchanged non-contiguous memory with Xen?
Any input will appreciate.
Thanks,
Joe
On 10/25/18 9:28 AM, Joe Jin wrote:
> On 10/25/18 9:10 AM, Boris Ostrovsky wrote:
>> On 10/25/18 10:23 AM, Joe Jin wrote:
>>> On 10/25/18 4:45 AM, Boris Ostrovsky wrote:
>>>> On 10/24/18 10:43 AM, Joe Jin wrote:
>>>>> On 10/24/18 6:57 AM, Boris Ostrovsky wrote:
>>>>>> On 10/24/18 9:02 AM, Konrad Rzeszutek Wilk wrote:
>>>>>>> On Tue, Oct 23, 2018 at 08:09:04PM -0700, Joe Jin wrote:
>>>>>>>> Commit 4855c92dbb7 "xen-swiotlb: fix the check condition for
>>>>>>>> xen_swiotlb_free_coherent" only fixed memory address check condition
>>>>>>>> on xen_swiotlb_free_coherent(), when memory was not physically
>>>>>>>> contiguous and tried to exchanged with Xen via
>>>>>>>> xen_destroy_contiguous_region it will lead kernel panic.
>>>>>>> s/it will lead/which lead to/?
>>>>>>>
>>>>>>>> The correct check condition should be memory is in DMA area and
>>>>>>>> physically contiguous.
>>>>>>> "The correct check condition to make Xen hypercall to revert the
>>>>>>> memory back from its 32-bit pool is if it is:
>>>>>>> 1) Above its DMA bit mask (for example 32-bit devices can only address
>>>>>>> up to 4GB, and we may want 4GB+2K), and
>>>>>> Is this "and' or 'or'?
>>>>>>
>>>>>>> 2) If it not physically contingous
>>>>>>>
>>>>>>> N.B. The logic in the code is inverted, which leads to all sorts of
>>>>>>> confusions."
>>>>>> I would, in fact, suggest to make the logic the same in both
>>>>>> xen_swiotlb_alloc_coherent() and xen_swiotlb_free_coherent() to avoid
>>>>>> this. This will involve swapping if and else in the former.
>>>>>>
>>>>>>
>>>>>>> Does that sound correct?
>>>>>>>
>>>>>>>> Thank you Boris for pointing it out.
>>>>>>>>
>>>>>>> Fixes: 4855c92dbb7 ("xen-sw..") ?
>>>>>>>
>>>>>>>> Signed-off-by: Joe Jin <[email protected]>
>>>>>>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>>>>>>> Cc: Boris Ostrovsky <[email protected]>
>>>>>>> Reported-by: Boris Ostrovs... ?
>>>>>>>> Cc: Christoph Helwig <[email protected]>
>>>>>>>> Cc: Dongli Zhang <[email protected]>
>>>>>>>> Cc: John Sobecki <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/xen/swiotlb-xen.c | 4 ++--
>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>>>>>>> index f5c1af4ce9ab..aed92fa019f9 100644
>>>>>>>> --- a/drivers/xen/swiotlb-xen.c
>>>>>>>> +++ b/drivers/xen/swiotlb-xen.c
>>>>>>>> @@ -357,8 +357,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>>>>>>>> /* Convert the size to actually allocated. */
>>>>>>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>>>>>>>
>>>>>>>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>>>>>>>> - range_straddles_page_boundary(phys, size))
>>>>>>>> + if ((dev_addr + size - 1 <= dma_mask) &&
>>>>>>>> + !range_straddles_page_boundary(phys, size))
>>>>>>>> xen_destroy_contiguous_region(phys, order);
>>>>>> I don't think this is right.
>>>>>>
>>>>>> if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size))
>>>>>>
>>>>>> No?
>>>>> No this is not correct.
>>>>>
>>>>> When allocate memory, it tried to allocated from Dom0/Guest, then check if physical
>>>>> address is DMA memory also contiguous, if no, exchange with Hypervisor, code as below:
>>>>>
>>>>> 326 phys = *dma_handle;
>>>>> 327 dev_addr = xen_phys_to_bus(phys);
>>>>> 328 if (((dev_addr + size - 1 <= dma_mask)) &&
>>>>> 329 !range_straddles_page_boundary(phys, size))
>>>>> 330 *dma_handle = dev_addr;
>>>>> 331 else {
>>>>> 332 if (xen_create_contiguous_region(phys, order,
>>>>> 333 fls64(dma_mask), dma_handle) != 0) {
>>>>> 334 xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
>>>>> 335 return NULL;
>>>>> 336 }
>>>>> 337 }
>>>>>
>>>>>
>>>>> On freeing, need to return the memory to Xen, otherwise DMA memory will be used
>>>>> up(this is the issue the patch intend to fix), so when memory is DMAable and
>>>>> contiguous then call xen_destroy_contiguous_region(), return DMA memory to Xen.
>>>> So if you want to allocate 1 byte at address 0 (and dev_addr=phys),
>>>> xen_create_contiguous_region() will not be called. And yet you will call
>>>> xen_destroy_contiguous_region() in the free path.
>>>>
>>>> Is this the expected behavior?
>>> I could not say it's expected behavior, but I think it's reasonable.
>>
>> I would expect xen_create_contiguous_region() and
>> xen_destroy_contiguous_region() to come in pairs. If a region is
>> created, it needs to be destroyed. And vice versa.
>>
>>
>>>
>>> On allocating, it used __get_free_pages() to allocate memory, if lucky the memory is
>>> DMAable, will not exchange memory with hypervisor, obviously this is not guaranteed.
>>>
>>> And on freeing it could not be identified if memory from Dom0/guest own memory
>>> or hypervisor
>>
>>
>> I think it can be. if (!(dev_addr + size - 1 <= dma_mask) ||
>> range_straddles_page_boundary()) then it must have come from the
>> hypervisor, because that's the check we make in
>> xen_swiotlb_alloc_coherent().
>
> This is not true.
>
> dev_addr was came from dma_handle, *dma_handle will be changed after called
> xen_create_contiguous_region():
>
> 2590 int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> 2591 unsigned int address_bits,
> 2592 dma_addr_t *dma_handle)
> 2593 {
> ......
> 2617 success = xen_exchange_memory(1UL << order, 0, in_frames,
> 2618 1, order, &out_frame,
> 2619 address_bits);
> 2620
> 2621 /* 3. Map the new extent in place of old pages. */
> 2622 if (success)
> 2623 xen_remap_exchanged_ptes(vstart, order, NULL, out_frame);
> 2624 else
> 2625 xen_remap_exchanged_ptes(vstart, order, in_frames, 0);
> 2626
> 2627 spin_unlock_irqrestore(&xen_reservation_lock, flags);
> 2628
> 2629 *dma_handle = virt_to_machine(vstart).maddr;
> 2630 return success ? 0 : -ENOMEM;
> 2631 }
>
>
> So means dev_addr check on xen_swiotlb_alloc_coherent() is not same one on
> xen_swiotlb_free_coherent().
>
> Thanks,
> Joe
>
>
>>
>>
>> -boris
>>
>>
>>> , if don't back memory to hypervisor which will lead hypervisor DMA
>>> memory be used up, then on Dom0/guest, DMA request maybe failed, the worse thing is
>>> could not start any new guest.
>>>
>>> Thanks,
>>> Joe
>>>
>>>> -boris
>>>>
>>
--
Oracle <http://www.oracle.com>
Joe Jin | Software Development Director
ORACLE | Linux and Virtualization
500 Oracle Parkway Redwood City, CA US 94065
On Thu, Oct 25, 2018 at 11:56:02AM -0700, Joe Jin wrote:
> I just discussed this patch with Boris in private, his opinions(Boris,
> please correct me if any misunderstood) are:
>
> 1. With/without the check, both are incorrect, he thought we need to
> prevented unalloc'd free at here.
> 2. On freeing, if upper layer already checked the memory was DMA-able,
> the checking at here does not make sense, we can remove all checks.
> 3. xen_create_contiguous_region() and xen_destroy_contiguous_region()
> to come in pairs.
>
> For #1 and #3, I think we need something associate it, like a list, on
> allocating, add addr to it, on freeing, check if in the list.
Is there any way to figure out based on an address if the exchange
operation happened?
> For #2, I'm was not found anywhere validated the address on
> dma_free_coherent() callpath, not just xen-swiotlb.
At least for simple direct mappings there is no easy way to verify that
without keeping a list, and for some of the ops that do vmap like
operations we have basic santiy checks, but nothing that really catches
a wrong free.
Hi Joe,
On 10/26/2018 03:48 PM, Christoph Helwig wrote:
> On Thu, Oct 25, 2018 at 11:56:02AM -0700, Joe Jin wrote:
>> I just discussed this patch with Boris in private, his opinions(Boris,
>> please correct me if any misunderstood) are:
>>
>> 1. With/without the check, both are incorrect, he thought we need to
>> prevented unalloc'd free at here.
>> 2. On freeing, if upper layer already checked the memory was DMA-able,
>> the checking at here does not make sense, we can remove all checks.
>> 3. xen_create_contiguous_region() and xen_destroy_contiguous_region()
>> to come in pairs.
>>
>> For #1 and #3, I think we need something associate it, like a list, on
>> allocating, add addr to it, on freeing, check if in the list.
If dom0 (or any domain) is happy, although it could try to exchange all its
continuous dma pages back to xen hypervisor. From the perspective of each
domain, they always would like to keep as much continuous dma page as possible.
I am thinking something different. If there is malicious domU keep exchanging
memory and allocating continuous pages from xen hypervisor, will the
continuously dma pages be used up (sort of DoS attack)?
I am not sure if there is anything in xen hypervisor to prevent such behavior?
Dongli Zhang
>
> Is there any way to figure out based on an address if the exchange
> operation happened?
>
>> For #2, I'm was not found anywhere validated the address on
>> dma_free_coherent() callpath, not just xen-swiotlb.
>
> At least for simple direct mappings there is no easy way to verify that
> without keeping a list, and for some of the ops that do vmap like
> operations we have basic santiy checks, but nothing that really catches
> a wrong free.
>
Hi Christoph,
On 10/26/18 12:48 AM, Christoph Helwig wrote:
> On Thu, Oct 25, 2018 at 11:56:02AM -0700, Joe Jin wrote:
>> I just discussed this patch with Boris in private, his opinions(Boris,
>> please correct me if any misunderstood) are:
>>
>> 1. With/without the check, both are incorrect, he thought we need to
>> prevented unalloc'd free at here.
>> 2. On freeing, if upper layer already checked the memory was DMA-able,
>> the checking at here does not make sense, we can remove all checks.
>> 3. xen_create_contiguous_region() and xen_destroy_contiguous_region()
>> to come in pairs.
>>
>> For #1 and #3, I think we need something associate it, like a list, on
>> allocating, add addr to it, on freeing, check if in the list.
>
> Is there any way to figure out based on an address if the exchange
> operation happened?
Read the code path and I was not found anywhere will store related info,
on current code, it assuming if memory in DMA area also contiguous then
it from Xen, most time it's true, but if lucky that __get_free_pages()
returned memory is DMAable, it will not exchange with Xen, during my testing
I observed same(Xen DMA heap increased).
>
>> For #2, I'm was not found anywhere validated the address on
>> dma_free_coherent() callpath, not just xen-swiotlb.
>
> At least for simple direct mappings there is no easy way to verify that
> without keeping a list, and for some of the ops that do vmap like> operations we have basic santiy checks, but nothing that really catches
> a wrong free.
I agree with you, add a list will help this issue, but it may introduce some
performance issue, especially on heavy DMA system.
Driver use DMA pool will help to avoid this issue, but not all kinds DMA ops
are suitable to create a pool.
Thanks,
Joe
On 10/26/18 1:54 AM, Dongli Zhang wrote:
> If dom0 (or any domain) is happy, although it could try to exchange all its
> continuous dma pages back to xen hypervisor. From the perspective of each
> domain, they always would like to keep as much continuous dma page as possible.
>
> I am thinking something different. If there is malicious domU keep exchanging
> memory and allocating continuous pages from xen hypervisor, will the
> continuously dma pages be used up (sort of DoS attack)?
This is a problem.
>
> I am not sure if there is anything in xen hypervisor to prevent such behavior?
I'm not sure but I guess it hard to prevent it, xen hypervisor could not identify
if the requirement is reasonable or no.
Maybe Xen reserve some low memory for guest start?
Thanks,
Joe
On 10/25/18 11:56 AM, Joe Jin wrote:
> I just discussed this patch with Boris in private, his opinions(Boris,
> please correct me if any misunderstood) are:
>
> 1. With/without the check, both are incorrect, he thought we need to
> prevented unalloc'd free at here.
> 2. On freeing, if upper layer already checked the memory was DMA-able,
> the checking at here does not make sense, we can remove all checks.
> 3. xen_create_contiguous_region() and xen_destroy_contiguous_region()
> to come in pairs.
I tried to added radix_tree to track allocating/freeing and I found some
memory only allocated but was not freed, I guess it caused by driver used
dma_pool, that means if lots of such requests, the list will consume lot
of memory for it. Will continue to work on it, if anyone have good idea
for it please let me know, I'd like to try it.
Thanks,
Joe