Re: gigaset: memory leak in gigaset_initcshw

[Paul Bolle <pebolle@tiscali.nl>]

Hi Dmitry,

(If anyone is confused by this conversation: Dmitry replied to an off
list message.)

On vr, 2016-02-05 at 14:28 +0100, Dmitry Vyukov wrote:
> I wonder why you don't see the leak I am seeing...

So do I, for a few days now.

> are you suing qemu or real hardware? I am using qemu.

Real hardware (a ThinkPad). Probably less powerful that your VM.

What is the rate you're seeing leakage of a struct ser_cardstate? I'm
running your latest test at about 2.000 TIOCSETD's per second - which is
by itself not very useful for our driver - and notice no _obvious_
leakage when I do that for a few minutes. I do note the hardware
screaming to just keep up with the abuse, though.

> 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()?

gigaset_device_release() is the release operation that is run when our
struct device goes away. The core code is responsible for calling it, we
can't be certain when that will happen. At least, we should not expect
it to happen directly after calling platform_device_unregister().

(It was actually syzkaller that warned us that we did just that until
recently. See commit 4c5e354a9742 ("ser_gigaset: fix deallocation of
platform device structure").)

> If that would happen, then cs can be reused while the previous
> cs->hw.ser is not freed yet. Just a guess.

I'll have to ponder on that a bit, sorry.


Paul Bolle