Re: gigaset: memory leak in gigaset_initcshw

[Dmitry Vyukov]

On Thu, Feb 4, 2016 at 4:06 PM, Paul Bolle <pebolle@xxxxxxxxxx> wrote:
> On do, 2016-02-04 at 15:54 +0100, Dmitry Vyukov wrote:
>> One TIOCSETD is enough to trigger the leak.
>> I've tested with different line disciplines and only N_GIGASET_M101
>> triggers the leak.
>
> So things appear to be just on my plate now. I'll see what I can come up
> with. Feel free to prod me if I stay silent for too long.
>
> Thanks for narrowing things down!

> 0) It's way too late here. And I can't really reproduce, and too many
> things jumps to 100% when running your reproducer. Anyhow, this is all I
> came up with (for drivers/isdn/gigaset/common.c):
>
> @@ -427,7 +427,11 @@ exit:
>
>  static void free_cs(struct cardstate *cs)
>  {
> -       cs->flags = 0;
> +       unsigned long flags;
> +       struct gigaset_driver *drv = cs->driver;
> +       spin_lock_irqsave(&drv->lock, flags);
> +       cs->flags &= ~VALID_MINOR;
> +       spin_unlock_irqrestore(&drv->lock, flags);
>  }
>
>  static void make_valid(struct cardstate *cs, unsigned mask)
>
> 1) On my side the logs do seem more sensible, for what it's worth. But
> does this fix the leak?
>
> 2) Note that I fear your reproducer allows to DOS the box (even if the
> leak turns out to be fixed) so the mess might be getting much worse. I
> need a clear hear to think this through. Ie, I need some sleep.

No, it does not help.

I wonder why you don't see the leak I am seeing... are you suing qemu
or real hardware? I am using qemu.

I've added the following change:

--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -396,6 +396,7 @@ static int gigaset_initcshw(struct cardstate *cs)
                pr_err("out of memory\n");
                return -ENOMEM;
        }
+       WARN_ON(cs->hw.ser != NULL);
        cs->hw.ser = scs;

        cs->hw.ser->dev.name = GIGASET_MODULENAME;

and it does fire.
Can it be a case that free_cs() runs before gigaset_device_release()?
If that would happen, then cs can be reused while the previous
cs->hw.ser is not freed yet. Just a guess.