Re: lvm in 2.5.1

[Andrew Morton]

[e-mail] wrote:
> 
> Hi Jens and other kernelhackers,
> 
> I'm now running 2.5.1 with lvm. The following patch makes some minor changes
> for bio-support and removes allocation of a lv_t on the stack, which made
> the stack overflow and gave me something to spend my last 24 hours
> debugging.

That's a worry, because an lv_t is only 420 bytes.  If that's triggering
a stack overflow then we're way too close.  Think interrupts....

There must be other sources of stack bloat.

lvm_chr_ioctl() calls lvm_do_vg_create(), and it has has another lv_t
on the stack.  That's 840 bytes - still not enough.  Maybe lvm_do_vg_create()
is calling something which uses lots of stack?  Can't see it.  Odd.

> ...
>         /* get the logical volume structures */
>         vg_ptr->lv_cur = 0;
>         for (l = 0; l < vg_ptr->lv_max; l++) {
>                 lv_t *lvp;
> +
>                 /* user space address */
>                 if ((lvp = vg_ptr->lv[l]) != NULL) {
> -                       if (copy_from_user(&lv, lvp, sizeof(lv_t)) != 0) {
> +                       if (copy_from_user(tmplv, lvp, sizeof(lv_t)) != 0) {
>                                 P_IOCTL("ERROR: copying LV ptr %p (%d bytes)\n",
>                                         lvp, sizeof(lv_t));
>                                 lvm_do_vg_remove(minor);
>                                 return -EFAULT;

Seems that in various places here you've forgotten to free the lv_t storage
on error paths?

Which means you must painfully go through and put a kfree() in front
of each and every `return' statement, hope you don't miss any, hope
you don't break anything, and hope that people don't forget to do this
next time they change the code.  Or you need to go through and restructure
the code so you need only one kfree.

Which is a fine and shining example of why one should never, ever, ever,
ever put more than one `return' statement in a C function.  Dammit.  It
is the single worst feature of C and should not be used.

BTW, it appears that lvm_do_vg_create() leaks the vmalloced snap_lv_ptr
on an error path...

-