Quoting Dan Smith (danms@xxxxxxxxxx): > Fix the potential for breakage if our UTS changes during checkpoint or > restart by grabbing uts_sem during these operations. Thanks, but I think you're waving too big a stick here. At checkpoint, you only need the uts_sem right around the reading (and length-checking) of uts_ns->name. You're keeping it around writing into the checkpoint image (and cr_hbuf_get). At restart, you shouldn't actually need uts_sem - the nsproxy is not yet available to anyone else, so there is no chance of a race, right? Hmm, well maybe I'm being overly optimistic, since restart is coordinated by userspace which could mess up and somehow run sethostname(2) while it's supposed to be running sys_restart(). But then sys_restart() wouldn't yet have hooked up the task to the new nsproxy. So yeah, I don't think you need uts_sem at restart. thanks, -serge > Cc: orenl@xxxxxxxxxxxxxxx > Signed-off-by: Dan Smith <danms@xxxxxxxxxx> > --- > checkpoint/ckpt_task.c | 12 ++++++++---- > checkpoint/rstr_task.c | 17 +++++++++++------ > 2 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/checkpoint/ckpt_task.c b/checkpoint/ckpt_task.c > index 4d19e31..589f304 100644 > --- a/checkpoint/ckpt_task.c > +++ b/checkpoint/ckpt_task.c > @@ -173,14 +173,16 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns) > struct cr_hdr_utsns *hh; > int domainname_len; > int nodename_len; > - int ret; > + int ret = -ENOMEM; > + > + down_read(&uts_sem); > > h.type = CR_HDR_UTSNS; > h.len = sizeof(*hh); > > hh = cr_hbuf_get(ctx, sizeof(*hh)); > if (!hh) > - return -ENOMEM; > + goto out; > > nodename_len = strlen(uts_ns->name.nodename) + 1; > domainname_len = strlen(uts_ns->name.domainname) + 1; > @@ -191,13 +193,15 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns) > ret = cr_write_obj(ctx, &h, hh); > cr_hbuf_put(ctx, sizeof(*hh)); > if (ret < 0) > - return ret; > + goto out; > > ret = cr_write_string(ctx, uts_ns->name.nodename, nodename_len); > if (ret < 0) > - return ret; > + goto out; > > ret = cr_write_string(ctx, uts_ns->name.domainname, domainname_len); > + out: > + up_read(&uts_sem); > return ret; > } > > diff --git a/checkpoint/rstr_task.c b/checkpoint/rstr_task.c > index fe5c059..ccf60f8 100644 > --- a/checkpoint/rstr_task.c > +++ b/checkpoint/rstr_task.c > @@ -171,32 +171,37 @@ static int cr_read_utsns(struct cr_ctx *ctx) > { > struct cr_hdr_utsns *hh; > struct uts_namespace *ns; > - int ret; > + int ret = -ENOMEM; > + > + down_write(&uts_sem); > > hh = cr_hbuf_get(ctx, sizeof(*hh)); > if (!hh) > - return -ENOMEM; > + goto out; > > ret = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_UTSNS); > if (ret < 0) > - goto out; > + goto out_hbuf; > > ret = -EINVAL; > if (hh->nodename_len > sizeof(ns->name.nodename) || > hh->domainname_len > sizeof(ns->name.domainname)) > - goto out; > + goto out_hbuf; > > ns = current->nsproxy->uts_ns; > > memset(ns->name.nodename, 0, sizeof(ns->name.nodename)); > ret = cr_read_string(ctx, ns->name.nodename, hh->nodename_len); > if (ret < 0) > - goto out; > + goto out_hbuf; > > memset(ns->name.domainname, 0, sizeof(ns->name.domainname)); > ret = cr_read_string(ctx, ns->name.domainname, hh->domainname_len); > + > + out_hbuf: > + cr_hbuf_put(ctx, sizeof(*hh)); > out: > - cr_hbuf_put(ctx, sizeof(*hh)); > + up_write(&uts_sem); > return ret; > } > > -- > 1.5.6.3 > > _______________________________________________ > Containers mailing list > Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers