[PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[Maxime Coquelin]

At removal time, when testing whether the IOTLB entry has
shared pages with the previous and next entries in the
cache, it checks whether the start address of the entry to
be removed is on the same page as the start address of the
next entry in the cache.

This is not correct, as an entry could cover several page
so the end address of the entry to be remove should be
used. This patch address this issue.

Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/iotlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
index 11785392ac..3f45bc6061 100644
--- a/lib/vhost/iotlb.c
+++ b/lib/vhost/iotlb.c
@@ -182,8 +182,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtque
 					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
 				next_node = RTE_TAILQ_NEXT(node, next);
 				/* Don't disable coredump if the next node is in the same page */
-				if (next_node == NULL ||
-						(node->uaddr & mask) != (next_node->uaddr & mask))
+				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
+						(next_node->uaddr & mask))
 					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
 							false, alignment);
 			}
@@ -287,8 +287,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq
 					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
 				next_node = RTE_TAILQ_NEXT(node, next);
 				/* Don't disable coredump if the next node is in the same page */
-				if (next_node == NULL ||
-						(node->uaddr & mask) != (next_node->uaddr & mask))
+				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
+						(next_node->uaddr & mask))
 					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
 							false, alignment);
 			}
-- 
2.39.2

RE: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, March 15, 2023 7:40 PM
> To: dev@dpdk.org; mkp@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check
> 
> At removal time, when testing whether the IOTLB entry has
> shared pages with the previous and next entries in the
> cache, it checks whether the start address of the entry to
> be removed is on the same page as the start address of the
> next entry in the cache.
> 
> This is not correct, as an entry could cover several page
> so the end address of the entry to be remove should be
> used. This patch address this issue.
> 
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/iotlb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
> index 11785392ac..3f45bc6061 100644
> --- a/lib/vhost/iotlb.c
> +++ b/lib/vhost/iotlb.c
> @@ -182,8 +182,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net
> *dev, struct vhost_virtque
>  					(node->uaddr & mask) != (prev_node->uaddr &
> mask)) {
>  				next_node = RTE_TAILQ_NEXT(node, next);
>  				/* Don't disable coredump if the next node is in
> the same page */
> -				if (next_node == NULL ||
> -						(node->uaddr & mask) != (next_node-
> >uaddr & mask))
> +				if (next_node == NULL || ((node->uaddr + node-
> >size - 1) & mask) !=
> +						(next_node->uaddr & mask))
>  					mem_set_dump((void *)(uintptr_t)node->uaddr,
> node->size,
>  							false, alignment);
>  			}
> @@ -287,8 +287,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev,
> struct vhost_virtqueue *vq
>  					(node->uaddr & mask) != (prev_node->uaddr &
> mask)) {
>  				next_node = RTE_TAILQ_NEXT(node, next);
>  				/* Don't disable coredump if the next node is in
> the same page */
> -				if (next_node == NULL ||
> -						(node->uaddr & mask) != (next_node-
> >uaddr & mask))
> +				if (next_node == NULL || ((node->uaddr + node-
> >size - 1) & mask) !=
> +						(next_node->uaddr & mask))
>  					mem_set_dump((void *)(uintptr_t)node->uaddr,
> node->size,
>  							false, alignment);
>  			}
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> 

Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[David Marchand]

Hello Maxime,

On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> At removal time, when testing whether the IOTLB entry has
> shared pages with the previous and next entries in the
> cache, it checks whether the start address of the entry to
> be removed is on the same page as the start address of the
> next entry in the cache.
>
> This is not correct, as an entry could cover several page
> so the end address of the entry to be remove should be
> used. This patch address this issue.

I'm trying to understand the logic, so I needed to write this down :-).

Let's imagine the cache contained 3 nodes, "prev", "node" and "next".
All those nodes (in this example) do not start or end on a page boundary.
Prior to touching those entries, all pages of the nodes are marked as DODUMP.

"prev" spans over two pages, "a" and "b".
"node" spans over three pages, "b", "c" and "d".
"next" spans over two pages, "d" and "e".

IOW, "prev" and "node" are sharing the "b" page.
IOW, "node" and "next" are sharing the "d" page.

Something like (better displayed with fixed-width chars):
   prev      node      next
  <----> <----------> <---->
|  a  |  b  |  c  |  d  |  e  |



Previous to this fix, since we were testing the first page of each
node, it resulted in page "b" being marked as DONTDUMP, while it was
still in use for "prev".
And for the same reason, page "d" would be marked as DONTDUMP too.

After this fix, all pages are left with DODUMP.

Is my understanding correct?

If so, there is still one (minor?) issue to look into: we leave the
"c" page as DODUMP while it won't contain useful information.

>
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>


-- 
David Marchand

Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[Maxime Coquelin]

On 3/16/23 09:13, David Marchand wrote:
> Hello Maxime,
> 
> On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> At removal time, when testing whether the IOTLB entry has
>> shared pages with the previous and next entries in the
>> cache, it checks whether the start address of the entry to
>> be removed is on the same page as the start address of the
>> next entry in the cache.
>>
>> This is not correct, as an entry could cover several page
>> so the end address of the entry to be remove should be
>> used. This patch address this issue.
> 
> I'm trying to understand the logic, so I needed to write this down :-).
> 
> Let's imagine the cache contained 3 nodes, "prev", "node" and "next".
> All those nodes (in this example) do not start or end on a page boundary.
> Prior to touching those entries, all pages of the nodes are marked as DODUMP.
> 
> "prev" spans over two pages, "a" and "b".
> "node" spans over three pages, "b", "c" and "d".
> "next" spans over two pages, "d" and "e".
> 
> IOW, "prev" and "node" are sharing the "b" page.
> IOW, "node" and "next" are sharing the "d" page.
> 
> Something like (better displayed with fixed-width chars):
>     prev      node      next
>    <----> <----------> <---->
> |  a  |  b  |  c  |  d  |  e  |
> 
> 
> 
> Previous to this fix, since we were testing the first page of each
> node, it resulted in page "b" being marked as DONTDUMP, while it was
> still in use for "prev".
> And for the same reason, page "d" would be marked as DONTDUMP too.
> 
> After this fix, all pages are left with DODUMP.
> 
> Is my understanding correct?

It is correct, that's the other bug I mentioned you yesterday.
I should have mentioned it in the commit log.

> If so, there is still one (minor?) issue to look into: we leave the
> "c" page as DODUMP while it won't contain useful information.

In my opinion, this is a minor issue as it indeed keeps some pages as 
DODUMP while they should be set as DONTDUMP. And the changes required to
fix it seems too big at the stage of the release, and I would prefer to
fix it in v23.07 to be on the safe side.

It is the opposite for this fix, which is trivial and prevent missing
pages in the coredump.

Does that sounds good to you? I can add a note in the commit message if
you want.

Thanks,
Maxime

>>
>> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> 

Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[David Marchand]

On Thu, Mar 16, 2023 at 9:38 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 3/16/23 09:13, David Marchand wrote:
> > On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> At removal time, when testing whether the IOTLB entry has
> >> shared pages with the previous and next entries in the
> >> cache, it checks whether the start address of the entry to
> >> be removed is on the same page as the start address of the
> >> next entry in the cache.
> >>
> >> This is not correct, as an entry could cover several page
> >> so the end address of the entry to be remove should be
> >> used. This patch address this issue.
> >
> > I'm trying to understand the logic, so I needed to write this down :-).
> >
> > Let's imagine the cache contained 3 nodes, "prev", "node" and "next".
> > All those nodes (in this example) do not start or end on a page boundary.
> > Prior to touching those entries, all pages of the nodes are marked as DODUMP.
> >
> > "prev" spans over two pages, "a" and "b".
> > "node" spans over three pages, "b", "c" and "d".
> > "next" spans over two pages, "d" and "e".
> >
> > IOW, "prev" and "node" are sharing the "b" page.
> > IOW, "node" and "next" are sharing the "d" page.
> >
> > Something like (better displayed with fixed-width chars):
> >     prev      node      next
> >    <----> <----------> <---->
> > |  a  |  b  |  c  |  d  |  e  |
> >
> >
> >
> > Previous to this fix, since we were testing the first page of each
> > node, it resulted in page "b" being marked as DONTDUMP, while it was
> > still in use for "prev".
> > And for the same reason, page "d" would be marked as DONTDUMP too.
> >
> > After this fix, all pages are left with DODUMP.
> >
> > Is my understanding correct?
>
> It is correct, that's the other bug I mentioned you yesterday.

Probably, but I did not catch it at the time :-).


> I should have mentioned it in the commit log.
>
> > If so, there is still one (minor?) issue to look into: we leave the
> > "c" page as DODUMP while it won't contain useful information.
>
> In my opinion, this is a minor issue as it indeed keeps some pages as
> DODUMP while they should be set as DONTDUMP. And the changes required to
> fix it seems too big at the stage of the release, and I would prefer to
> fix it in v23.07 to be on the safe side.
>
> It is the opposite for this fix, which is trivial and prevent missing
> pages in the coredump.
>
> Does that sounds good to you? I can add a note in the commit message if
> you want.

Ok for me with a note yes.
This code is not trivial :-).


Thanks.


-- 
David Marchand

Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[Maxime Coquelin]

On 3/16/23 09:52, David Marchand wrote:
> On Thu, Mar 16, 2023 at 9:38 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> On 3/16/23 09:13, David Marchand wrote:
>>> On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin
>>> <maxime.coquelin@redhat.com> wrote:
>>>>
>>>> At removal time, when testing whether the IOTLB entry has
>>>> shared pages with the previous and next entries in the
>>>> cache, it checks whether the start address of the entry to
>>>> be removed is on the same page as the start address of the
>>>> next entry in the cache.
>>>>
>>>> This is not correct, as an entry could cover several page
>>>> so the end address of the entry to be remove should be
>>>> used. This patch address this issue.
>>>
>>> I'm trying to understand the logic, so I needed to write this down :-).
>>>
>>> Let's imagine the cache contained 3 nodes, "prev", "node" and "next".
>>> All those nodes (in this example) do not start or end on a page boundary.
>>> Prior to touching those entries, all pages of the nodes are marked as DODUMP.
>>>
>>> "prev" spans over two pages, "a" and "b".
>>> "node" spans over three pages, "b", "c" and "d".
>>> "next" spans over two pages, "d" and "e".
>>>
>>> IOW, "prev" and "node" are sharing the "b" page.
>>> IOW, "node" and "next" are sharing the "d" page.
>>>
>>> Something like (better displayed with fixed-width chars):
>>>      prev      node      next
>>>     <----> <----------> <---->
>>> |  a  |  b  |  c  |  d  |  e  |
>>>
>>>
>>>
>>> Previous to this fix, since we were testing the first page of each
>>> node, it resulted in page "b" being marked as DONTDUMP, while it was
>>> still in use for "prev".
>>> And for the same reason, page "d" would be marked as DONTDUMP too.
>>>
>>> After this fix, all pages are left with DODUMP.
>>>
>>> Is my understanding correct?
>>
>> It is correct, that's the other bug I mentioned you yesterday.
> 
> Probably, but I did not catch it at the time :-).
> 
> 
>> I should have mentioned it in the commit log.
>>
>>> If so, there is still one (minor?) issue to look into: we leave the
>>> "c" page as DODUMP while it won't contain useful information.
>>
>> In my opinion, this is a minor issue as it indeed keeps some pages as
>> DODUMP while they should be set as DONTDUMP. And the changes required to
>> fix it seems too big at the stage of the release, and I would prefer to
>> fix it in v23.07 to be on the safe side.
>>
>> It is the opposite for this fix, which is trivial and prevent missing
>> pages in the coredump.
>>
>> Does that sounds good to you? I can add a note in the commit message if
>> you want.
> 
> Ok for me with a note yes.

Added this:

"
     Note there is another issue not fixed by this patch, but
     delayed to next release given its minor impact and the
     complexity of the fix it requires. If a removed IOTLB entry
     is spanned on several pages and one of the pages is shared
     with another entry, all the pages will remain as DODUMP
     while only the shared page should be. It would result in
     non-shared pages to be part of the coredump while it would
     not be needed.
"

> This code is not trivial :-).

Yes, I have some ideas to simplify it, but it will wait v23.07

Thanks,
Maxime

> 
> Thanks.
> 
> 

Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[Maxime Coquelin]

On 3/15/23 12:40, Maxime Coquelin wrote:
> At removal time, when testing whether the IOTLB entry has
> shared pages with the previous and next entries in the
> cache, it checks whether the start address of the entry to
> be removed is on the same page as the start address of the
> next entry in the cache.
> 
> This is not correct, as an entry could cover several page
> so the end address of the entry to be remove should be
> used. This patch address this issue.
> 
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   lib/vhost/iotlb.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
> index 11785392ac..3f45bc6061 100644
> --- a/lib/vhost/iotlb.c
> +++ b/lib/vhost/iotlb.c
> @@ -182,8 +182,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtque
>   					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
>   				next_node = RTE_TAILQ_NEXT(node, next);
>   				/* Don't disable coredump if the next node is in the same page */
> -				if (next_node == NULL ||
> -						(node->uaddr & mask) != (next_node->uaddr & mask))
> +				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
> +						(next_node->uaddr & mask))
>   					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
>   							false, alignment);
>   			}
> @@ -287,8 +287,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq
>   					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
>   				next_node = RTE_TAILQ_NEXT(node, next);
>   				/* Don't disable coredump if the next node is in the same page */
> -				if (next_node == NULL ||
> -						(node->uaddr & mask) != (next_node->uaddr & mask))
> +				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
> +						(next_node->uaddr & mask))
>   					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
>   							false, alignment);
>   			}


Applied to dpdk-next-virtio/main.

Thanks,
Maxime