[PATCH] c/r: Take uts_sem during checkpoint (v2)

[PATCH] c/r: Take uts_sem during checkpoint (v2)

[Dan Smith]

Fix the potential for breakage if our UTS changes during checkpoint
by grabbing uts_sem and copying those strings to temporary buffers.

Cc: orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org
Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Changes in v2:
  - Be less stupid about holding the system-wide uts_sem during
    checkpoint (!)
  - Don't hold it during restart
  - Hold uts_sem only while copying out the strings
  - Calculate the length of the saved buffers outside of the semaphore and
    avoid the duplicate _len variables by cleaning up the cr_hdr_utsns
    after the cr_write_string() of the two buffers
---
 checkpoint/ckpt_task.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/checkpoint/ckpt_task.c b/checkpoint/ckpt_task.c
index 4d19e31..30858d2 100644
--- a/checkpoint/ckpt_task.c
+++ b/checkpoint/ckpt_task.c
@@ -171,8 +171,8 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns)
 {
 	struct cr_hdr h;
 	struct cr_hdr_utsns *hh;
-	int domainname_len;
-	int nodename_len;
+	char nodename[__NEW_UTS_LEN + 1];
+	char domainname[__NEW_UTS_LEN + 1];
 	int ret;
 
 	h.type = CR_HDR_UTSNS;
@@ -182,22 +182,25 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns)
 	if (!hh)
 		return -ENOMEM;
 
-	nodename_len = strlen(uts_ns->name.nodename) + 1;
-	domainname_len = strlen(uts_ns->name.domainname) + 1;
+	down_read(&uts_sem);
+	memcpy(nodename, uts_ns->name.nodename, sizeof(nodename));
+	memcpy(domainname, uts_ns->name.domainname, sizeof(nodename));
+	up_read(&uts_sem);
 
-	hh->nodename_len = nodename_len;
-	hh->domainname_len = domainname_len;
+	hh->nodename_len = strlen(nodename) + 1;
+	hh->domainname_len = strlen(domainname) + 1;
 
 	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);
+	ret = cr_write_string(ctx, nodename, hh->nodename_len);
 	if (ret < 0)
-		return ret;
+		goto out;
 
-	ret = cr_write_string(ctx, uts_ns->name.domainname, domainname_len);
+	ret = cr_write_string(ctx, domainname, hh->domainname_len);
+ out:
+ 	cr_hbuf_put(ctx, sizeof(*hh));
 	return ret;
 }
 
-- 
1.5.6.3

Re: [PATCH] c/r: Take uts_sem during checkpoint (v2)

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Fix the potential for breakage if our UTS changes during checkpoint
> by grabbing uts_sem and copying those strings to temporary buffers.
> 
> Cc: orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Looks good.  The only thing I'd add is that you are depending on
__NEW_UTS_LEN+1 being something very specific, so if utsname.h
gets a change, checkpoint/ckpt_task.c needs a corresponding
change.  So it would be robust to future code changes if you

#define MAX_UTS_LEN (__NEW_UTS_LEN+1)

in utsname.h, so that anyone expanding the size of hostname
doesn't need to look for this usage.

Still,
Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

-serge

> Changes in v2:
>   - Be less stupid about holding the system-wide uts_sem during
>     checkpoint (!)
>   - Don't hold it during restart
>   - Hold uts_sem only while copying out the strings
>   - Calculate the length of the saved buffers outside of the semaphore and
>     avoid the duplicate _len variables by cleaning up the cr_hdr_utsns
>     after the cr_write_string() of the two buffers
> ---
>  checkpoint/ckpt_task.c |   25 ++++++++++++++-----------
>  1 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/checkpoint/ckpt_task.c b/checkpoint/ckpt_task.c
> index 4d19e31..30858d2 100644
> --- a/checkpoint/ckpt_task.c
> +++ b/checkpoint/ckpt_task.c
> @@ -171,8 +171,8 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns)
>  {
>  	struct cr_hdr h;
>  	struct cr_hdr_utsns *hh;
> -	int domainname_len;
> -	int nodename_len;
> +	char nodename[__NEW_UTS_LEN + 1];
> +	char domainname[__NEW_UTS_LEN + 1];
>  	int ret;
> 
>  	h.type = CR_HDR_UTSNS;
> @@ -182,22 +182,25 @@ static int cr_write_utsns(struct cr_ctx *ctx, struct uts_namespace *uts_ns)
>  	if (!hh)
>  		return -ENOMEM;
> 
> -	nodename_len = strlen(uts_ns->name.nodename) + 1;
> -	domainname_len = strlen(uts_ns->name.domainname) + 1;
> +	down_read(&uts_sem);
> +	memcpy(nodename, uts_ns->name.nodename, sizeof(nodename));
> +	memcpy(domainname, uts_ns->name.domainname, sizeof(nodename));
> +	up_read(&uts_sem);
> 
> -	hh->nodename_len = nodename_len;
> -	hh->domainname_len = domainname_len;
> +	hh->nodename_len = strlen(nodename) + 1;
> +	hh->domainname_len = strlen(domainname) + 1;
> 
>  	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);
> +	ret = cr_write_string(ctx, nodename, hh->nodename_len);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
> 
> -	ret = cr_write_string(ctx, uts_ns->name.domainname, domainname_len);
> +	ret = cr_write_string(ctx, domainname, hh->domainname_len);
> + out:
> + 	cr_hbuf_put(ctx, sizeof(*hh));
>  	return ret;
>  }
> 
> -- 
> 1.5.6.3
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] c/r: Take uts_sem during checkpoint (v2)

[Dan Smith]

SH> So it would be robust to future code changes if you

SH> #define MAX_UTS_LEN (__NEW_UTS_LEN+1)

SH> in utsname.h, so that anyone expanding the size of hostname
SH> doesn't need to look for this usage.

Yeah, I think that came up in a prior round of comments on the UTS
stuff, but got dropped when we started depending on strlen() of the
names instead.  I suppose I should actually do it now... :)

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH] c/r: Take uts_sem during checkpoint (v2)

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> SH> So it would be robust to future code changes if you
> 
> SH> #define MAX_UTS_LEN (__NEW_UTS_LEN+1)
> 
> SH> in utsname.h, so that anyone expanding the size of hostname
> SH> doesn't need to look for this usage.
> 
> Yeah, I think that came up in a prior round of comments on the UTS
> stuff, but got dropped when we started depending on strlen() of the
> names instead.  I suppose I should actually do it now... :)

Ok, please keep my Ack on the result.

thanks,
-serge