[PATCH 1/1] cr: fix ckpt_obj_fetch return values (v2)

[PATCH 1/1] cr: fix ckpt_obj_fetch return values (v2)

[Serge E. Hallyn]

Sorry for the noise, but here is a corrected version of my patch
from earlier today.

ckpt_obj_fetch returned ERR_PTR(error) on some failures, NULL on
others.  Not all of its callers were checking for NULL, which
would lead to NULL dereferences.

Return -EINVAL if the object is not in the hash table.  Fix up
pipe_file_restore to do the right thing.

Changelog: May 13: fix typo in ckpt_obj_fetch.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/files.c   |    4 +---
 checkpoint/memory.c  |    6 +-----
 checkpoint/objhash.c |    2 +-
 checkpoint/process.c |    4 +---
 fs/pipe.c            |    9 ++++-----
 5 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/checkpoint/files.c b/checkpoint/files.c
index c6a946b..bc9944c 100644
--- a/checkpoint/files.c
+++ b/checkpoint/files.c
@@ -484,9 +484,7 @@ static int restore_fd_ent(struct ckpt_ctx *ctx)
 		goto out;
 
 	file = ckpt_obj_fetch(ctx, h->fd_objref, CKPT_OBJ_FILE);
-	if (!file)
-		goto out;
-	else if (IS_ERR(file)) {
+	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);
 		goto out;
 	}
diff --git a/checkpoint/memory.c b/checkpoint/memory.c
index 92d4485..5f2930f 100644
--- a/checkpoint/memory.c
+++ b/checkpoint/memory.c
@@ -1207,8 +1207,6 @@ static struct mm_struct *do_restore_mm(struct ckpt_ctx *ctx)
 	/* restore the ->exe_file */
 	if (h->exefile_objref) {
 		file = ckpt_obj_fetch(ctx, h->exefile_objref, CKPT_OBJ_FILE);
-		if (!file)
-			file = ERR_PTR(-EINVAL);
 		if (IS_ERR(file)) {
 			up_write(&mm->mmap_sem);
 			ret = PTR_ERR(file);
@@ -1246,9 +1244,7 @@ int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref)
 	int ret;
 
 	mm = ckpt_obj_fetch(ctx, mm_objref, CKPT_OBJ_MM);
-	if (!mm)
-		return -EINVAL;
-	else if (IS_ERR(mm))
+	if (IS_ERR(mm))
 		return -EINVAL;
 
 	if (mm == current->mm)
diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 3a860aa..09364e2 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -575,7 +575,7 @@ void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
 
 	obj = obj_find_by_objref(ctx, objref);
 	if (!obj)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	ckpt_debug("%s ref %d\n", obj->ops->obj_name, obj->objref);
 	return (obj->ops->obj_type == type ? obj->ptr : ERR_PTR(-EINVAL));
 }
diff --git a/checkpoint/process.c b/checkpoint/process.c
index cf7a44a..1c36ae2 100644
--- a/checkpoint/process.c
+++ b/checkpoint/process.c
@@ -595,9 +595,7 @@ static int restore_ns_obj(struct ckpt_ctx *ctx, int ns_objref)
 	struct nsproxy *nsproxy;
 
 	nsproxy = ckpt_obj_fetch(ctx, ns_objref, CKPT_OBJ_NS);
-	if (!nsproxy)
-		return -EINVAL;
-	else if (IS_ERR(nsproxy))
+	if (IS_ERR(nsproxy))
 		return PTR_ERR(nsproxy);
 
 	if (nsproxy != task_nsproxy(current))
diff --git a/fs/pipe.c b/fs/pipe.c
index ab2de3c..b284dcb 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 		return ERR_PTR(-EINVAL);
 
 	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
-	if (IS_ERR(file))
-		return file;
 	/*
-	 * If ckpt_obj_fetch() returned NULL, then this is the first
+	 * If ckpt_obj_fetch() returned -EINVAL, then this is the first
 	 * time we see this pipe so need to restore the contents.
 	 * Otherwise, use the file pointer skip forward.
 	 */
-	if (!file) {
+	if (PTR_ERR(file) == -EINVAL) {
 		/* first encounter of this pipe: create it */
 		ret = do_pipe_flags(fds, 0);
 		if (ret < 0)
@@ -1025,7 +1023,8 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 		/* get rid of the file descriptors (caller sets that) */
 		sys_close(fds[which]);
 		sys_close(fds[1-which]);
-	}
+	} else if (IS_ERR(file))
+		return file;
 
 	ret = restore_file_common(ctx, file, ptr);
 	if (ret < 0) {
-- 
1.6.1

Re: [PATCH 1/1] cr: fix ckpt_obj_fetch return values (v2)

[Oren Laadan]

This looks clearer - applied after some modifications (see comments
below).

Serge E. Hallyn wrote:
> Sorry for the noise, but here is a corrected version of my patch
> from earlier today.
> 
> ckpt_obj_fetch returned ERR_PTR(error) on some failures, NULL on
> others.  Not all of its callers were checking for NULL, which
> would lead to NULL dereferences.
> 
> Return -EINVAL if the object is not in the hash table.  Fix up
> pipe_file_restore to do the right thing.
> 
> Changelog: May 13: fix typo in ckpt_obj_fetch.
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/files.c   |    4 +---
>  checkpoint/memory.c  |    6 +-----
>  checkpoint/objhash.c |    2 +-
>  checkpoint/process.c |    4 +---
>  fs/pipe.c            |    9 ++++-----
>  5 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/checkpoint/files.c b/checkpoint/files.c
> index c6a946b..bc9944c 100644
> --- a/checkpoint/files.c
> +++ b/checkpoint/files.c
> @@ -484,9 +484,7 @@ static int restore_fd_ent(struct ckpt_ctx *ctx)
>  		goto out;
>  
>  	file = ckpt_obj_fetch(ctx, h->fd_objref, CKPT_OBJ_FILE);
> -	if (!file)
> -		goto out;
> -	else if (IS_ERR(file)) {
> +	if (IS_ERR(file)) {
>  		ret = PTR_ERR(file);
>  		goto out;
>  	}
> diff --git a/checkpoint/memory.c b/checkpoint/memory.c
> index 92d4485..5f2930f 100644
> --- a/checkpoint/memory.c
> +++ b/checkpoint/memory.c
> @@ -1207,8 +1207,6 @@ static struct mm_struct *do_restore_mm(struct ckpt_ctx *ctx)
>  	/* restore the ->exe_file */
>  	if (h->exefile_objref) {
>  		file = ckpt_obj_fetch(ctx, h->exefile_objref, CKPT_OBJ_FILE);
> -		if (!file)
> -			file = ERR_PTR(-EINVAL);
>  		if (IS_ERR(file)) {
>  			up_write(&mm->mmap_sem);
>  			ret = PTR_ERR(file);
> @@ -1246,9 +1244,7 @@ int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref)
>  	int ret;
>  
>  	mm = ckpt_obj_fetch(ctx, mm_objref, CKPT_OBJ_MM);
> -	if (!mm)
> -		return -EINVAL;
> -	else if (IS_ERR(mm))
> +	if (IS_ERR(mm))
>  		return -EINVAL;
			^^^^^^
should be:		PTR_ERR(mm);

>  
>  	if (mm == current->mm)
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index 3a860aa..09364e2 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -575,7 +575,7 @@ void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
>  
>  	obj = obj_find_by_objref(ctx, objref);
>  	if (!obj)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	ckpt_debug("%s ref %d\n", obj->ops->obj_name, obj->objref);
>  	return (obj->ops->obj_type == type ? obj->ptr : ERR_PTR(-EINVAL));
>  }
> diff --git a/checkpoint/process.c b/checkpoint/process.c
> index cf7a44a..1c36ae2 100644
> --- a/checkpoint/process.c
> +++ b/checkpoint/process.c
> @@ -595,9 +595,7 @@ static int restore_ns_obj(struct ckpt_ctx *ctx, int ns_objref)
>  	struct nsproxy *nsproxy;
>  
>  	nsproxy = ckpt_obj_fetch(ctx, ns_objref, CKPT_OBJ_NS);
> -	if (!nsproxy)
> -		return -EINVAL;
> -	else if (IS_ERR(nsproxy))
> +	if (IS_ERR(nsproxy))
>  		return PTR_ERR(nsproxy);
>  
>  	if (nsproxy != task_nsproxy(current))
> diff --git a/fs/pipe.c b/fs/pipe.c
> index ab2de3c..b284dcb 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
>  		return ERR_PTR(-EINVAL);
>  
>  	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
> -	if (IS_ERR(file))
> -		return file;
>  	/*
> -	 * If ckpt_obj_fetch() returned NULL, then this is the first
> +	 * If ckpt_obj_fetch() returned -EINVAL, then this is the first
>  	 * time we see this pipe so need to restore the contents.
>  	 * Otherwise, use the file pointer skip forward.
>  	 */
> -	if (!file) {
> +	if (PTR_ERR(file) == -EINVAL) {

ckpt_obj_fetch() will also fail with -EINVAL if the type of the object
found doesn't match the type that was requested. Hence why originally
the two return values. So I changed that latter to report -ENOMSG (that
is:  /* No message of desired type */)

>  		/* first encounter of this pipe: create it */
>  		ret = do_pipe_flags(fds, 0);
>  		if (ret < 0)
> @@ -1025,7 +1023,8 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
>  		/* get rid of the file descriptors (caller sets that) */
>  		sys_close(fds[which]);
>  		sys_close(fds[1-which]);
> -	}
> +	} else if (IS_ERR(file))
> +		return file;
>  
>  	ret = restore_file_common(ctx, file, ptr);
>  	if (ret < 0) {

Thanks,

Oren.

Re: [PATCH 1/1] cr: fix ckpt_obj_fetch return values (v2)

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> > @@ -1246,9 +1244,7 @@ int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref)
> >  	int ret;
> >  
> >  	mm = ckpt_obj_fetch(ctx, mm_objref, CKPT_OBJ_MM);
> > -	if (!mm)
> > -		return -EINVAL;
> > -	else if (IS_ERR(mm))
> > +	if (IS_ERR(mm))
> >  		return -EINVAL;
> 			^^^^^^
> should be:		PTR_ERR(mm);

Oops, right.

> > diff --git a/fs/pipe.c b/fs/pipe.c
> > index ab2de3c..b284dcb 100644
> > --- a/fs/pipe.c
> > +++ b/fs/pipe.c
> > @@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
> >  		return ERR_PTR(-EINVAL);
> >  
> >  	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
> > -	if (IS_ERR(file))
> > -		return file;
> >  	/*
> > -	 * If ckpt_obj_fetch() returned NULL, then this is the first
> > +	 * If ckpt_obj_fetch() returned -EINVAL, then this is the first
> >  	 * time we see this pipe so need to restore the contents.
> >  	 * Otherwise, use the file pointer skip forward.
> >  	 */
> > -	if (!file) {
> > +	if (PTR_ERR(file) == -EINVAL) {
> 
> ckpt_obj_fetch() will also fail with -EINVAL if the type of the object
> found doesn't match the type that was requested. Hence why originally
> the two return values. So I changed that latter to report -ENOMSG (that
> is:  /* No message of desired type */)

Hmm, yes I was thinking of the checkpoint case where mismatched types
would be strictly an objhash bug.  You're right, this case can just
be a bad checkpoint image!

thanks,
-serge

Re: [PATCH 1/1] cr: fix ckpt_obj_fetch return values (v2)

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
>>> @@ -1246,9 +1244,7 @@ int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref)
>>>  	int ret;
>>>  
>>>  	mm = ckpt_obj_fetch(ctx, mm_objref, CKPT_OBJ_MM);
>>> -	if (!mm)
>>> -		return -EINVAL;
>>> -	else if (IS_ERR(mm))
>>> +	if (IS_ERR(mm))
>>>  		return -EINVAL;
>> 			^^^^^^
>> should be:		PTR_ERR(mm);
> 
> Oops, right.
> 
>>> diff --git a/fs/pipe.c b/fs/pipe.c
>>> index ab2de3c..b284dcb 100644
>>> --- a/fs/pipe.c
>>> +++ b/fs/pipe.c
>>> @@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>>  	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
>>> -	if (IS_ERR(file))
>>> -		return file;
>>>  	/*
>>> -	 * If ckpt_obj_fetch() returned NULL, then this is the first
>>> +	 * If ckpt_obj_fetch() returned -EINVAL, then this is the first
>>>  	 * time we see this pipe so need to restore the contents.
>>>  	 * Otherwise, use the file pointer skip forward.
>>>  	 */
>>> -	if (!file) {
>>> +	if (PTR_ERR(file) == -EINVAL) {
>> ckpt_obj_fetch() will also fail with -EINVAL if the type of the object
>> found doesn't match the type that was requested. Hence why originally
>> the two return values. So I changed that latter to report -ENOMSG (that
>> is:  /* No message of desired type */)
> 
> Hmm, yes I was thinking of the checkpoint case where mismatched types
> would be strictly an objhash bug.  You're right, this case can just
> be a bad checkpoint image!

Actually, there was also a bit of logic change required for uts_ns
and ipc_ns. Fixed that too.

Oren.