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@xxxxxxxxxxxxxxx> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
