cr: fix ckpt_obj_fetch return values (v2)
PATCH 1/1 cr: fix ckpt_obj_fetch return values (v2)
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.
--- 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(-)
checkpoint/files.c +1 -3
@@ -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; }
checkpoint/memory.c +1 -5
@@ -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)
checkpoint/objhash.c +1 -1
@@ -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 )); }
checkpoint/process.c +1 -3
@@ -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))
fs/pipe.c +4 -5
@@ -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 ) {
Signature · 1 line
1.6.1
A message that is not in the archive (sent off-list or not yet received): 20090513210641.GA22784-r/[email protected]
This looks clearer - applied after some modifications (see comments below).
Quoted 50 lines · Serge E. Hallyn
Sorry for the noise, but here is a corrected version of my patch from earlier today.
ckpt_obj_fetchreturnedERR_PTR(error)on some failures, NULL on others. Not all of its callers were checking for NULL, which would lead to NULL dereferences.Return
-EINVALif the object is not in the hash table. Fix uppipe_file_restoreto do the right thing.Changelog: May 13: fix typo in
ckpt_obj_fetch.Signed-off-by Serge E. Hallyn <serue-r/[email protected]>--- 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(-)checkpoint/files.c +1
-3@@ -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; }checkpoint/memory.c +0
-2@@ -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);
checkpoint/memory.c int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref) · line 1248
Quoted 8 lines
checkpoint/memory.c +1
-3@@ -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);
Quoted 38 lines
checkpoint/memory.c +0
-0if (mm == current->mm)checkpoint/objhash.c +1
-1@@ -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 )); }checkpoint/process.c +1
-3@@ -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))fs/pipe.c +0
-0@@ -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 );
fs/pipe.c struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr) · line 990
Quoted 10 lines
fs/pipe.c +2
-4-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 */)
fs/pipe.c struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr) · line 1030
Quoted 13 lines
fs/pipe.c +2
-1/* 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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · Oren Laadan
checkpoint/memory.c int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref) · line 1248
Quoted 9 lines
checkpoint/memory.c +1
-3@@ -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.
Quoted 7 lines
Quoted 7 lines
fs/pipe.c +0
-0@@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)return ERR_PTR (-EINVAL );
Quoted 16 lines
fs/pipe.c struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr) · line 990
Quoted 11 lines
fs/pipe.c +2
-4file =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-EINVALif 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
A message that is not in the archive (sent off-list or not yet received): 20090514141449.GA4972-r/[email protected]
Quoted 38 lines · Serge E. Hallyn
Quoted 11 lines · Oren Laadan
checkpoint/memory.c int restore_mm_obj(struct ckpt_ctx *ctx, int mm_objref) · line 1248
Quoted 9 lines
checkpoint/memory.c +1
-3@@ -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.
Quoted 7 lines
Quoted 7 lines
fs/pipe.c +0
-0@@ -982,14 +982,12 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)return ERR_PTR (-EINVAL );Quoted 15 lines
fs/pipe.c struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr) · line 990
Quoted 11 lines
fs/pipe.c +2
-4file =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-EINVALif 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 */)
Quoted 3 lines
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.