Messages in this thread | | | Date | Tue, 24 Feb 2009 01:47:39 -0600 | From | Nathan Lynch <> | Subject | Re: [RFC v13][PATCH 05/14] x86 support for checkpoint/restart |
| |
Hi, this is an old thread I guess, but I just noticed some issues while looking at this code.
On Tue, 27 Jan 2009 12:08:03 -0500 Oren Laadan <orenl@cs.columbia.edu> wrote: > +static int cr_read_cpu_fpu(struct cr_ctx *ctx, struct task_struct *t) > +{ > + void *xstate_buf = cr_hbuf_get(ctx, xstate_size); > + int ret; > + > + ret = cr_kread(ctx, xstate_buf, xstate_size); > + if (ret < 0) > + goto out; > + > + /* i387 + MMU + SSE */ > + preempt_disable(); > + > + /* init_fpu() also calls set_used_math() */ > + ret = init_fpu(current); > + if (ret < 0) > + return ret;
Several problems here: * init_fpu can call kmem_cache_alloc(GFP_KERNEL), but is called here with preempt disabled (init_fpu could use a might_sleep annotation?) * if init_fpu returns an error, we get preempt imbalance * if init_fpu returns an error, we "leak" the cr_hbuf_get for xstate_buf
Speaking of cr_hbuf_get... I'd prefer to see that "allocator" go away and its users converted to kmalloc/kfree (this is what I've done for the powerpc C/R code, btw).
Using the slab allocator would:
* make the code less obscure and easier to review * make the code more amenable to static analysis * gain the benefits of slab debugging at runtime
But I think this has been pointed out before. If I understand the justification for cr_hbuf_get correctly, the allocations it services are somehow known to be bounded in size and nesting. But even if that is the case, it's not much of a reason to avoid using kmalloc, is it?
| |