| Message ID | 20230724110247.10520-21-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
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,
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@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. 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
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; + 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) { + 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;
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(-)