Message ID | CACT4Y+aNN8AAtRSvxxrP5RfsTZdd49odnQ_GRq86Y=jXr=pANw@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
Hi Dmitry, On vr, 2016-02-05 at 17:06 +0100, Paul Bolle wrote: > 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. 0) I finally managed to reliably trigger this leak on an i686, single core machine (yet another ThinkPad). 1) Note that on that machine the leak was noticeable under the kmalloc -512 line (struct ser_cardstate is 456 bytes on that machine). I'm _guessing_ the kmalloc-2048 line, which I stared at for quite some time, is only relevant here for x86_64 and when there's a bit of instrumentation, or whatever, added to the slab objects (as they are in your VM?). 2) More important was that this i686 machine ran a tree that actually included the offending commit: 25cad69f21f5 ("base/platform: Fix platform drivers with no probe callback"). See, after staring at the gigaset code for way too long I decided to just use brute force. Ie, I bisected this issue. 2) Anyhow, thanks again for the report. Now on to the next question: how on earth does that commit make ser_gigaset leak struct ser_cardstate? To be continued, Paul Bolle
--- 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;