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@suse.com>
> ---
> 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