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

[Juergen Gross <jgross@xxxxxxxx>]

On 01.08.23 23:46, Julien Grall wrote:
> 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.
Okay.

> >       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.
Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature