Re: [PATCH v3 20/25] tools/xenstore: alloc new memory in domain_adjust_node_perms()

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 24/07/2023 12:02, Juergen Gross wrote:
> In order to avoid modifying the node data in the data base in case a
> domain is gone, let domain_adjust_node_perms() allocate new memory for
> the permissions in case they need to be modified. As this should
> happen only in very rare cases, it is fine to do this even when having
> copied the node data already.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V3:
> - new patch
> ---
>   tools/xenstore/xenstored_core.c   | 10 +++++-----
>   tools/xenstore/xenstored_domain.c | 19 +++++++++++++++----
>   2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 404ecd0c62..ea3d20a372 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -751,6 +751,11 @@ struct node *read_node(struct connection *conn, const void 
> *ctx,
>                 goto error;
>         }
>   
> +	/* Data is binary blob (usually ascii, no nul). */
> +       node->data = node->perms + hdr->num_perms;
> +       /* Children is strings, nul separated. */
> +       node->children = node->data + node->hdr.datalen;
> +
It took me a while to understand why you move the lines above. I tihnk 
it would be worth documenting in the code (possibly on top of the 
declaration domain_adjust_node_perms()) that domain_adjust_node_perms() 
may re-allocate the permissions.
>         if (domain_adjust_node_perms(node))
>                 goto error;
>   
> @@ -758,11 +763,6 @@ struct node *read_node(struct connection *conn, const void *ctx,
>         if (node->acc.domid != get_node_owner(node))
>                 node->acc.memory = 0;
>   
> -	/* Data is binary blob (usually ascii, no nul). */
> -       node->data = node->perms + hdr->num_perms;
> -       /* Children is strings, nul separated. */
> -       node->children = node->data + node->hdr.datalen;
> -
>         if (access_node(conn, node, NODE_ACCESS_READ, NULL))
>                 goto error;
>   
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index fdf1095acb..cdef6efef4 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -1334,13 +1334,24 @@ int domain_alloc_permrefs(struct node_perms *perms)
>   int domain_adjust_node_perms(struct node *node)
>   {
>         unsigned int i;
> +       struct xs_permissions *perms = node->perms;
> +       bool copied = false;
>   
>   	for (i = 1; i < node->hdr.num_perms; i++) {
> -               if (node->perms[i].perms & XS_PERM_IGNORE)
> +               if ((perms[i].perms & XS_PERM_IGNORE) ||
> +                   chk_domain_generation(perms[i].id, node->hdr.generation))
>                         continue;
> -               if (!chk_domain_generation(node->perms[i].id,
> -                                          node->hdr.generation))
> -                       node->perms[i].perms |= XS_PERM_IGNORE;
> +
> +               if (!copied) {
This wants a coment explain why you need to copy it.

> +                       perms = talloc_memdup(node, node->perms,
> +                                       node->hdr.num_perms * sizeof(*perms));
> +                       if (!perms)
> +                               return ENOMEM;
> +                       node->perms = perms;
> +                       copied = true;
> +               }
> +
> +               perms[i].perms |= XS_PERM_IGNORE;
>         }
>   
>   	return 0;
Cheers,

--
Julien Grall