Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325546 > unrolled thread

gigaset: memory leak in gigaset_initcshw

Back to article view | Back to linux.kernel

Contents

  gigaset: memory leak in gigaset_initcshw Dmitry Vyukov <dvyukov@google.com> - 2016-02-03 16:40 +0100
    Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-03 17:20 +0100
      Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-03 20:20 +0100
        Re: gigaset: memory leak in gigaset_initcshw Dmitry Vyukov <dvyukov@google.com> - 2016-02-04 11:50 +0100
          Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-04 14:20 +0100
            Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-04 15:00 +0100
              Re: gigaset: memory leak in gigaset_initcshw Dmitry Vyukov <dvyukov@google.com> - 2016-02-04 16:00 +0100
                Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-04 16:10 +0100
                  Re: gigaset: memory leak in gigaset_initcshw Dmitry Vyukov <dvyukov@google.com> - 2016-02-05 14:40 +0100
                    Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-05 17:10 +0100
                      Re: gigaset: memory leak in gigaset_initcshw Paul Bolle <pebolle@tiscali.nl> - 2016-02-05 19:40 +0100
                        Re: gigaset: memory leak in gigaset_initcshw Dmitry Vyukov <dvyukov@google.com> - 2016-02-05 22:30 +0100

#1325546 — gigaset: memory leak in gigaset_initcshw

Hello,

The following program causes a memory leak of ser_cardstate object
allocated in gigaset_initcshw:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <pthread.h>
#include <stdint.h>
#include <string.h>
#include <sys/syscall.h>
#include <unistd.h>

int main()
{
  long r[7];
  memset(r, -1, sizeof(r));
  r[0] = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x3ul, 0x32ul,
                 0xfffffffffffffffful, 0x0ul);
  r[2] = syscall(SYS_open, "/dev/ptmx", 0x8002ul, 0x0ul, 0, 0, 0);
  *(uint32_t*)0x20002b1e = (uint32_t)0x10;
  r[4] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20002b1eul, 0, 0, 0);
  *(uint32_t*)0x20009000 = (uint32_t)0x7;
  r[6] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20009000ul, 0, 0, 0);
  return 0;
}

unreferenced object 0xffff88002b4109d8 (size 2048):
  comm "a.out", pid 9565, jiffies 4301785161 (age 10.646s)
  hex dump (first 32 bytes):
    e0 30 0a 87 ff ff ff ff 00 00 00 00 00 00 00 00  .0..............
    80 af 0d 88 ff ff ff ff 88 08 5e 00 00 88 ff ff  ..........^.....
  backtrace:
    [<     inline     >] kzalloc include/linux/slab.h:607
    [<ffffffff84bfbeab>] gigaset_initcshw+0x4b/0x2b0
drivers/isdn/gigaset/ser-gigaset.c:394
    [<ffffffff84bb1e22>] gigaset_initcs+0xf82/0x1660
drivers/isdn/gigaset/common.c:748
    [<ffffffff84bfb65b>] gigaset_tty_open+0x9b/0x460
drivers/isdn/gigaset/ser-gigaset.c:516
    [<ffffffff82f9a648>] tty_ldisc_open.isra.2+0x78/0xd0
drivers/tty/tty_ldisc.c:454
    [<ffffffff82f9ac32>] tty_set_ldisc+0x292/0x8a0 drivers/tty/tty_ldisc.c:561
    [<     inline     >] tiocsetd drivers/tty/tty_io.c:2656
    [<ffffffff82f8390e>] tty_ioctl+0xb2e/0x2160 drivers/tty/tty_io.c:2911
    [<     inline     >] vfs_ioctl fs/ioctl.c:43
    [<ffffffff817fa9ec>] do_vfs_ioctl+0x18c/0xfb0 fs/ioctl.c:674
    [<     inline     >] SYSC_ioctl fs/ioctl.c:689
    [<ffffffff817fb89f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
    [<ffffffff8665ebb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

On commit 34229b277480f46c1e9a19f027f30b074512e68b.

[toc] | [next] | [standalone]

#1325601

On wo, 2016-02-03 at 16:31 +0100, Dmitry Vyukov wrote:
> The following program causes a memory leak of ser_cardstate object
> allocated in gigaset_initcshw:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <pthread.h>
> #include <stdint.h>
> #include <string.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> 
> int main()
> {
>   long r[7];
>   memset(r, -1, sizeof(r));
>   r[0] = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x3ul, 0x32ul,
>                  0xfffffffffffffffful, 0x0ul);
>   r[2] = syscall(SYS_open, "/dev/ptmx", 0x8002ul, 0x0ul, 0, 0, 0);
>   *(uint32_t*)0x20002b1e = (uint32_t)0x10;
>   r[4] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20002b1eul, 0, 0, 0);
>   *(uint32_t*)0x20009000 = (uint32_t)0x7;
>   r[6] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20009000ul, 0, 0, 0);

strace translated this for me to
	mmap(0x20000000, 65536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
	open("/dev/ptmx", O_RDWR|O_LARGEFILE)   = 3
	ioctl(3, TIOCSETD, [16])                = 0
	ioctl(3, TIOCSETD, [7])                 = 0

Where 16 is N_GIGASET_M101, while 7 is N_6PACK.

>   return 0;
> }
> 
> unreferenced object 0xffff88002b4109d8 (size 2048):
>   comm "a.out", pid 9565, jiffies 4301785161 (age 10.646s)
>   hex dump (first 32 bytes):
>     e0 30 0a 87 ff ff ff ff 00 00 00 00 00 00 00 00  .0..............
>     80 af 0d 88 ff ff ff ff 88 08 5e 00 00 88 ff ff  ..........^.....
>   backtrace:
>     [<     inline     >] kzalloc include/linux/slab.h:607
>     [<ffffffff84bfbeab>] gigaset_initcshw+0x4b/0x2b0
> drivers/isdn/gigaset/ser-gigaset.c:394
>     [<ffffffff84bb1e22>] gigaset_initcs+0xf82/0x1660
> drivers/isdn/gigaset/common.c:748
>     [<ffffffff84bfb65b>] gigaset_tty_open+0x9b/0x460
> drivers/isdn/gigaset/ser-gigaset.c:516
>     [<ffffffff82f9a648>] tty_ldisc_open.isra.2+0x78/0xd0
> drivers/tty/tty_ldisc.c:454
>     [<ffffffff82f9ac32>] tty_set_ldisc+0x292/0x8a0
> drivers/tty/tty_ldisc.c:561
>     [<     inline     >] tiocsetd drivers/tty/tty_io.c:2656
>     [<ffffffff82f8390e>] tty_ioctl+0xb2e/0x2160
> drivers/tty/tty_io.c:2911
>     [<     inline     >] vfs_ioctl fs/ioctl.c:43
>     [<ffffffff817fa9ec>] do_vfs_ioctl+0x18c/0xfb0 fs/ioctl.c:674
>     [<     inline     >] SYSC_ioctl fs/ioctl.c:689
>     [<ffffffff817fb89f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
>     [<ffffffff8665ebb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185

The above should provide me with enough information to figure out what's
going on here.

> On commit 34229b277480f46c1e9a19f027f30b074512e68b.

That's "Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net",
of two days ago.

Thanks for the report,


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1325885

Hi Dmitry,

On wo, 2016-02-03 at 17:16 +0100, Paul Bolle wrote:
> The above should provide me with enough information to figure out
> what's going on here.

I've instrumented ser_gigaset with some printk's. Basically I added the
stuff pasted at the end of this message. In 10.000 runs of the program
syzkaller generated the added printk's suggest that struct ser_cardstate
is freed every time.

(Note that this was done on a machine that, probably like the VM
syzkaller was running in, doesn't have the clunky hardware that this
driver manages attached.)

Before I dive deeper into this: can you reproduce this leak? Is it
perhaps a one in gazillion runs thing? Do you have the logs of a run
that warned about this leak at hand?

Thanks,


Paul Bolle

@@ -375,9 +377,12 @@ static void gigaset_device_release(struct device *dev)
 {
        struct cardstate *cs = dev_get_drvdata(dev);
 
-       if (!cs)
+       if (!cs) {
+               pr_info("%s: no cardstate", __func__);
                return;
+       }
        dev_set_drvdata(dev, NULL);
+       pr_info("%s: kfree(%p)", __func__, cs->hw.ser);
        kfree(cs->hw.ser);
        cs->hw.ser = NULL;
 }
@@ -392,6 +397,7 @@ static int gigaset_initcshw(struct cardstate *cs)
        struct ser_cardstate *scs;
 
        scs = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL);
+       pr_info("%s: scs = %p", __func__, scs);
        if (!scs) {
                pr_err("out of memory\n");
                return -ENOMEM;

[toc] | [prev] | [next] | [standalone]

#1326651

On Wed, Feb 3, 2016 at 8:11 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> Hi Dmitry,
>
> On wo, 2016-02-03 at 17:16 +0100, Paul Bolle wrote:
>> The above should provide me with enough information to figure out
>> what's going on here.
>
> I've instrumented ser_gigaset with some printk's. Basically I added the
> stuff pasted at the end of this message. In 10.000 runs of the program
> syzkaller generated the added printk's suggest that struct ser_cardstate
> is freed every time.
>
> (Note that this was done on a machine that, probably like the VM
> syzkaller was running in, doesn't have the clunky hardware that this
> driver manages attached.)
>
> Before I dive deeper into this: can you reproduce this leak? Is it
> perhaps a one in gazillion runs thing? Do you have the logs of a run
> that warned about this leak at hand?
>
> Thanks,
>
>
> Paul Bolle
>
> @@ -375,9 +377,12 @@ static void gigaset_device_release(struct device *dev)
>  {
>         struct cardstate *cs = dev_get_drvdata(dev);
>
> -       if (!cs)
> +       if (!cs) {
> +               pr_info("%s: no cardstate", __func__);
>                 return;
> +       }
>         dev_set_drvdata(dev, NULL);
> +       pr_info("%s: kfree(%p)", __func__, cs->hw.ser);
>         kfree(cs->hw.ser);
>         cs->hw.ser = NULL;
>  }
> @@ -392,6 +397,7 @@ static int gigaset_initcshw(struct cardstate *cs)
>         struct ser_cardstate *scs;
>
>         scs = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL);
> +       pr_info("%s: scs = %p", __func__, scs);
>         if (!scs) {
>                 pr_err("out of memory\n");
>                 return -ENOMEM;



Forgot to mention that you need to run it in a parallel loop, sorry.

This one should do:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <pthread.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>

void work()
{
  long r[7];
  memset(r, -1, sizeof(r));
  r[0] = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x3ul, 0x32ul,
                 0xfffffffffffffffful, 0x0ul);
  r[2] = syscall(SYS_open, "/dev/ptmx", 0x8002ul, 0x0ul, 0, 0, 0);
  *(uint32_t*)0x20002b1e = (uint32_t)0x10;
  r[4] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20002b1eul, 0, 0, 0);
  *(uint32_t*)0x20009000 = (uint32_t)0x7;
  r[6] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20009000ul, 0, 0, 0);
}

int main() {
  int running, status;

  for (;;) {
    while (running < 32) {
      if (fork() == 0) {
        work();
        exit(0);
      }
      running++;
    }
    if (wait(&status) > 0)
      running--;
  }
}


While running it, sample/proc/slabinfo with:

# cat /proc/slabinfo | egrep "^kmalloc-2048"

It constantly grows.

[toc] | [prev] | [next] | [standalone]

#1326785

On do, 2016-02-04 at 11:40 +0100, Dmitry Vyukov wrote:
> Forgot to mention that you need to run it in a parallel loop, sorry.

I see.

> This one should do:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <pthread.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> 
> void work()
> {
>   long r[7];
>   memset(r, -1, sizeof(r));
>   r[0] = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x3ul, 0x32ul,
>                  0xfffffffffffffffful, 0x0ul);
>   r[2] = syscall(SYS_open, "/dev/ptmx", 0x8002ul, 0x0ul, 0, 0, 0);
>   *(uint32_t*)0x20002b1e = (uint32_t)0x10;
>   r[4] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20002b1eul, 0, 0, 0);
>   *(uint32_t*)0x20009000 = (uint32_t)0x7;
>   r[6] = syscall(SYS_ioctl, r[2], 0x5423ul, 0x20009000ul, 0, 0, 0);
> }
> 
> int main() {
>   int running, status;

(gcc complained about "running" being used uninitialized, though a few
mock runs suggest it got initialized to 0 anyhow. I initialized
"running" to 0 explicitly for the real runs.)

>   for (;;) {
>     while (running < 32) {
>       if (fork() == 0) {
>         work();
>         exit(0);
>       }
>       running++;
>     }
>     if (wait(&status) > 0)
>       running--;
>   }
> }

(Note to self: this hammers my laptop with about 2.000 runs per second.
After some time systemd's logging appears to have trouble handling the
output this reproducer generates, so maybe messages end up getting
dropped.)

> While running it, sample/proc/slabinfo with:
> 
> # cat /proc/slabinfo | egrep "^kmalloc-2048"
> 
> It constantly grows.

I don't really know how /proc/slabinfo should be interpreted, sorry. But
the interesting fields appear to be "<active_objs>" and "<num_objs>".
"<num_objs>" seems to be stable here (during the runs of a few minutes
that I dare to inflict on my laptop). "<active_objs>" is more volatile.
But I also saw it going down while the reproducer was running.

What are you seeing here?

Thanks,


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1326840

Hi Dmitry,

On do, 2016-02-04 at 14:15 +0100, Dmitry Vyukov wrote:
> On Thu, Feb 4, 2016 at 2:09 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> > What are you seeing here?
> 
> I see that active_objs is slowly, constantly growing.
> 
> I've attached my config file, please try with it. You mentioned that
> "16 is N_GIGASET_M101, while 7 is N_6PACK", probably one of these ttys
> is not enabled in your config,

Both are. (A 6pack module is loaded when I run the reproducer. I have no
idea what 6pack is good for. ser_gigaset is familiar, to me.)

>  and so the reproducer is not doing
> anything useful.

I actually wonder whether N_6PACK is relevant. Do you also see this
issue with another line discipline in the second TIOCSETD ioctl? 

Maybe it even triggers with two totally different line disciplines in
both calls? A (slightly edited) copy of tty.h is pasted below. It lists
the useful values for the TIOCSETD ioctl.

Would you mind testing a few combinations?

Thanks,


Paul Bolle

/* line disciplines */
#define N_TTY		0
#define N_SLIP		1
#define N_MOUSE		2
#define N_PPP		3
				/* 4 is obsolete */
#define N_AX25		5
#define N_X25		6	/* X.25 async */
#define N_6PACK		7
				/* 8 is obsolete */
#define N_R3964		9	/* Simatic R3964 */
				/* 10 is obsolete */
#define N_IRDA		11	/* Linux IrDa - http://irda.sourceforge.net/ */
				/* 12 is obsolete */
#define N_HDLC		13	/* synchronous HDLC */
#define N_SYNC_PPP	14	/* synchronous PPP */
#define N_HCI		15	/* Bluetooth HCI UART */
#define N_GIGASET_M101	16	/* Siemens Gigaset M101 serial DECT adapter */
#define N_SLCAN		17	/* Serial / USB serial CAN Adaptors */
#define N_PPS		18	/* Pulse per Second */
#define N_V253		19	/* Codec control over voice modem */
#define N_CAIF		20      /* CAIF protocol for talking to modems */
#define N_GSM0710	21	/* GSM 0710 Mux */
#define N_TI_WL		22	/* for TI's WL BT, FM, GPS combo chips */
#define N_TRACESINK	23	/* Trace data routing for MIPI P1149.7 */
#define N_TRACEROUTER	24	/* Trace data routing for MIPI P1149.7 */
#define N_NCI		25	/* NFC NCI UART */

[toc] | [prev] | [next] | [standalone]

#1326892

On Thu, Feb 4, 2016 at 2:46 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> Hi Dmitry,
>
> On do, 2016-02-04 at 14:15 +0100, Dmitry Vyukov wrote:
>> On Thu, Feb 4, 2016 at 2:09 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
>> > What are you seeing here?
>>
>> I see that active_objs is slowly, constantly growing.
>>
>> I've attached my config file, please try with it. You mentioned that
>> "16 is N_GIGASET_M101, while 7 is N_6PACK", probably one of these ttys
>> is not enabled in your config,
>
> Both are. (A 6pack module is loaded when I run the reproducer. I have no
> idea what 6pack is good for. ser_gigaset is familiar, to me.)
>
>>  and so the reproducer is not doing
>> anything useful.
>
> I actually wonder whether N_6PACK is relevant. Do you also see this
> issue with another line discipline in the second TIOCSETD ioctl?
>
> Maybe it even triggers with two totally different line disciplines in
> both calls? A (slightly edited) copy of tty.h is pasted below. It lists
> the useful values for the TIOCSETD ioctl.
>
> Would you mind testing a few combinations?
>
> Thanks,
>
>
> Paul Bolle
>
> /* line disciplines */
> #define N_TTY           0
> #define N_SLIP          1
> #define N_MOUSE         2
> #define N_PPP           3
>                                 /* 4 is obsolete */
> #define N_AX25          5
> #define N_X25           6       /* X.25 async */
> #define N_6PACK         7
>                                 /* 8 is obsolete */
> #define N_R3964         9       /* Simatic R3964 */
>                                 /* 10 is obsolete */
> #define N_IRDA          11      /* Linux IrDa - http://irda.sourceforge.net/ */
>                                 /* 12 is obsolete */
> #define N_HDLC          13      /* synchronous HDLC */
> #define N_SYNC_PPP      14      /* synchronous PPP */
> #define N_HCI           15      /* Bluetooth HCI UART */
> #define N_GIGASET_M101  16      /* Siemens Gigaset M101 serial DECT adapter */
> #define N_SLCAN         17      /* Serial / USB serial CAN Adaptors */
> #define N_PPS           18      /* Pulse per Second */
> #define N_V253          19      /* Codec control over voice modem */
> #define N_CAIF          20      /* CAIF protocol for talking to modems */
> #define N_GSM0710       21      /* GSM 0710 Mux */
> #define N_TI_WL         22      /* for TI's WL BT, FM, GPS combo chips */
> #define N_TRACESINK     23      /* Trace data routing for MIPI P1149.7 */
> #define N_TRACEROUTER   24      /* Trace data routing for MIPI P1149.7 */
> #define N_NCI           25      /* NFC NCI UART */


One TIOCSETD is enough to trigger the leak.
I've tested with different line disciplines and only N_GIGASET_M101
triggers the leak.
The program prints:

ioctl failed with 19
ioctl failed with 19
ioctl failed with 19
ioctl failed with 19
ioctl failed with 19
ioctl failed with 19
ioctl failed with 19
ioctl failed with 19

And console output looks as follows:

[  107.311623] driver: 'ser_gigaset': driver_bound: bound to device
'ser_gigaset.0'
[  107.312502] bus: 'platform': really_probe: bound device
ser_gigaset.0 to driver ser_gigaset
[  107.313789] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.314335] gigaset: maximum number of devices exceeded
[  107.318272] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.318794] gigaset: maximum number of devices exceeded
[  107.319541] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.320072] gigaset: maximum number of devices exceeded
[  107.320792] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.321834] gigaset: maximum number of devices exceeded
[  107.322782] kcapi: controller [001] "ser_gigaset" ready.
[  107.324414] kcapi: controller [001] down.
[  107.327864] bus: 'platform': remove device ser_gigaset.0
[  107.328733] kcapi: controller [001]: ser_gigaset unregistered
[  107.331246] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.331934] kcapi: controller [001]: ser_gigaset attached
[  107.337741] Registering platform device 'ser_gigaset.0'. Parent at platform
[  107.340149] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.340664] gigaset: maximum number of devices exceeded
[  107.341470] bus: 'platform': add device ser_gigaset.0
[  107.342189] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.342702] gigaset: maximum number of devices exceeded
[  107.344661] bus: 'platform': driver_probe_device: matched device
ser_gigaset.0 with driver ser_gigaset
[  107.345420] bus: 'platform': really_probe: probing driver
ser_gigaset with device ser_gigaset.0
[  107.346185] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.346187] gigaset: maximum number of devices exceeded
[  107.351893] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.352888] gigaset: maximum number of devices exceeded
[  107.359171] devices_kset: Moving ser_gigaset.0 to end of list
[  107.359867] driver: 'ser_gigaset': driver_bound: bound to device
'ser_gigaset.0'
[  107.366296] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.366794] gigaset: maximum number of devices exceeded
[  107.367198] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
[  107.367778] gigaset: maximum number of devices exceeded
[  107.391643] bus: 'platform': really_probe: bound device
ser_gigaset.0 to driver ser_gigaset
[  107.402270] kcapi: controller [001] "ser_gigaset" ready.
[  107.404980] kcapi: controller [001] down.
[  107.407644] bus: 'platform': remove device ser_gigaset.0
[  107.408554] kcapi: controller [001]: ser_gigaset unregistered



Here is updated program:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <stdint.h>
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <errno.h>

int r;

void work()
{
  int fd;

  fd = open("/dev/ptmx", O_RDWR);
  if (fd == -1)
    exit(printf("open failed with %d\n", errno));
  if (ioctl(fd, TIOCSETD, &r))
    exit(printf("ioctl failed with %d\n", errno));
}

int main(int argc, char **argv) {
  int running, status;

  if (argc != 2)
    return;
  r = atoi(argv[1]);
  running = 0;
  for (;;) {
    while (running < 32) {
      if (fork() == 0) {
        work();
        exit(0);
      }
      running++;
    }
    if (wait(&status) > 0)
      running--;
  }
}

[toc] | [prev] | [next] | [standalone]

#1326906

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!


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1327682

On Thu, Feb 4, 2016 at 4:06 PM, Paul Bolle <pebolle@tiscali.nl> 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.

[toc] | [prev] | [next] | [standalone]

#1327865

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

[toc] | [prev] | [next] | [standalone]

#1327987

On vr, 2016-02-05 at 17:06 +0100, Paul Bolle wrote:
> 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.

This is from the hit-the-code-until-it-confesses department:
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -373,13 +373,9 @@ static void gigaset_freecshw(struct cardstate *cs)
 
 static void gigaset_device_release(struct device *dev)
 {
-       struct cardstate *cs = dev_get_drvdata(dev);
-
-       if (!cs)
-               return;
+       struct ser_cardstate *scs = dev_get_drvdata(dev);
        dev_set_drvdata(dev, NULL);
-       kfree(cs->hw.ser);
-       cs->hw.ser = NULL;
+       kfree(scs);
 }
 
 /*
@@ -408,7 +404,7 @@ static int gigaset_initcshw(struct cardstate *cs)
                cs->hw.ser = NULL;
                return rc;
        }
-       dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
+       dev_set_drvdata(&cs->hw.ser->dev.dev, scs);
 
        tasklet_init(&cs->write_tasklet,
                     gigaset_modem_fill, (unsigned long) cs);

Does that make any difference?


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1328084

On Fri, Feb 5, 2016 at 7:36 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> On vr, 2016-02-05 at 17:06 +0100, Paul Bolle wrote:
>> 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.
>
> This is from the hit-the-code-until-it-confesses department:
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -373,13 +373,9 @@ static void gigaset_freecshw(struct cardstate *cs)
>
>  static void gigaset_device_release(struct device *dev)
>  {
> -       struct cardstate *cs = dev_get_drvdata(dev);
> -
> -       if (!cs)
> -               return;
> +       struct ser_cardstate *scs = dev_get_drvdata(dev);
>         dev_set_drvdata(dev, NULL);
> -       kfree(cs->hw.ser);
> -       cs->hw.ser = NULL;
> +       kfree(scs);
>  }
>
>  /*
> @@ -408,7 +404,7 @@ static int gigaset_initcshw(struct cardstate *cs)
>                 cs->hw.ser = NULL;
>                 return rc;
>         }
> -       dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
> +       dev_set_drvdata(&cs->hw.ser->dev.dev, scs);
>
>         tasklet_init(&cs->write_tasklet,
>                      gigaset_modem_fill, (unsigned long) cs);
>
> Does that make any difference?


Nope.
Almost 500 objects leaked in less than 10 seconds:

# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        1992   2015   2520   13    8 : tunables    0    0
  0 : slabdata    155    155      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2024   2041   2520   13    8 : tunables    0    0
  0 : slabdata    157    157      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2061   2080   2520   13    8 : tunables    0    0
  0 : slabdata    160    160      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2091   2119   2520   13    8 : tunables    0    0
  0 : slabdata    163    163      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2147   2171   2520   13    8 : tunables    0    0
  0 : slabdata    167    167      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2228   2236   2520   13    8 : tunables    0    0
  0 : slabdata    172    172      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2261   2288   2520   13    8 : tunables    0    0
  0 : slabdata    176    176      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2289   2301   2520   13    8 : tunables    0    0
  0 : slabdata    177    177      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2316   2340   2520   13    8 : tunables    0    0
  0 : slabdata    180    180      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2324   2366   2520   13    8 : tunables    0    0
  0 : slabdata    182    182      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2356   2379   2520   13    8 : tunables    0    0
  0 : slabdata    183    183      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2450   2509   2520   13    8 : tunables    0    0
  0 : slabdata    193    193      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2450   2509   2520   13    8 : tunables    0    0
  0 : slabdata    193    193      0
# cat /proc/slabinfo | egrep "^kmalloc-2048"
kmalloc-2048        2450   2509   2520   13    8 : tunables    0    0
  0 : slabdata    193    193      0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web