Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Anthony PERARD <anthony.perard@vates.tech>]

On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> This fixes the clobbering of the entry->next pointer when
> unmapping the first entry in a bucket of a mapcache.
> 
> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  hw/xen/xen-mapcache.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> index 5f23b0adbe..18ba7b1d8f 100644
> --- a/hw/xen/xen-mapcache.c
> +++ b/hw/xen/xen-mapcache.c
> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
>          pentry->next = entry->next;
>          g_free(entry);
>      } else {
> -        memset(entry, 0, sizeof *entry);
> +        /*
> +         * Invalidate mapping but keep entry->next pointing to the rest
> +         * of the list.
> +         *
> +         * Note that lock is already zero here, otherwise we don't unmap.
> +         */
> +        entry->paddr_index = 0;
> +        entry->vaddr_base = NULL;
> +        entry->valid_mapping = NULL;
> +        entry->flags = 0;
> +        entry->size = 0;

This kind of feels like mc->entry should be an array of pointer rather
than an array of MapCacheEntry but that seems to work well enough and
not the first time entries are been cleared like that.

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech