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

Groups > linux.kernel > #1276288 > unrolled thread

WARNING in gsm_cleanup_mux

Back to article view | Back to linux.kernel

Contents

  WARNING in gsm_cleanup_mux Dmitry Vyukov <dvyukov@google.com> - 2015-11-24 11:40 +0100
    [PATCH] TTY: n_gsm, fix false positive WARN_ON Jiri Slaby <jslaby@suse.cz> - 2015-11-24 18:00 +0100
      Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON xinhui <xinhui@linux.vnet.ibm.com> - 2015-11-25 07:40 +0100
        Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON Jiri Slaby <jslaby@suse.cz> - 2015-11-25 11:00 +0100
          Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON xinhui <xinhui@linux.vnet.ibm.com> - 2015-11-25 11:40 +0100

#1276288 — WARNING in gsm_cleanup_mux

Hello,

The following program trigger WARNING in gsm_cleanup_mux (run in a
loop, there seems to be a race):

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

long r2 = -1;

void *thr1(void *a)
{
        r2 = syscall(SYS_open, 0x200004b1ul, 0x100ul, 0x0ul, 0, 0, 0);
        return 0;
}

void *thr2(void *a)
{
        *(uint32_t*)0x200055a1 = 0x15;
        syscall(SYS_ioctl, r2, 0x5423ul, 0x200055a1ul, 0, 0, 0);
        return 0;
}

int main()
{
        syscall(SYS_mmap, 0x20000000ul, 0x100000ul, 0x3ul, 0x32ul,
0xfffffffffffffffful, 0x0ul);
        memcpy((void*)0x200004b1, "\x2f\x64\x65\x76\x2f\x70\x74\x6d\x78", 9);
        r2 = syscall(SYS_open, 0x200004b1ul, 0x100ul, 0x0ul, 0, 0, 0);
        *(uint32_t*)0x200055a1 = 0x15;
        syscall(SYS_ioctl, r2, 0x5423ul, 0x200055a1ul, 0, 0, 0);
        pthread_t th1;
        pthread_create(&th1, 0, thr1, 0);
        pthread_t th2;
        pthread_create(&th2, 0, thr2, 0);
        usleep(10000);
        return 0;
}


------------[ cut here ]------------
WARNING: CPU: 2 PID: 24238 at drivers/tty/n_gsm.c:2048
gsm_cleanup_mux+0x166/0x6b0()
Modules linked in:
CPU: 2 PID: 24238 Comm: a.out Tainted: G    B   W       4.4.0-rc1+ #81
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff8800621279d0 ffffffff825d3336 0000000000000000
 ffff880061e50000 ffffffff84f12ce0 ffff880062127a10 ffffffff81247889
 ffffffff828d0456 ffffffff84f12ce0 0000000000000800 0000000000000004
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff825d3336>] dump_stack+0x68/0x92 lib/dump_stack.c:50
 [<ffffffff81247889>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:457
 [<ffffffff81247ab9>] warn_slowpath_null+0x29/0x30 kernel/panic.c:490
 [<ffffffff828d0456>] gsm_cleanup_mux+0x166/0x6b0 drivers/tty/n_gsm.c:2048
 [<ffffffff828d4d87>] gsmld_open+0x5b7/0x7a0 drivers/tty/n_gsm.c:2386
 [<ffffffff828b9078>] tty_ldisc_open.isra.2+0x78/0xd0
drivers/tty/tty_ldisc.c:447
 [<ffffffff828b973a>] tty_set_ldisc+0x1ca/0xa70 drivers/tty/tty_ldisc.c:567
 [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
 [<ffffffff828a14ea>] tty_ioctl+0xb2a/0x2140 drivers/tty/tty_io.c:2883
 [<     inline     >] vfs_ioctl fs/ioctl.c:43
 [<ffffffff8169bcc1>] do_vfs_ioctl+0x681/0xe40 fs/ioctl.c:607
 [<     inline     >] SYSC_ioctl fs/ioctl.c:622
 [<ffffffff8169c50f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:613
 [<ffffffff84bf0c36>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---[ end trace 179dec89fcb66e7c ]---


Here is strace output of the program:

execve("./a.out", ["./a.out"], [/* 14 vars */]) = 0
uname({sys="Linux", node="dvyukov-z840.muc.corp.google.com", ...}) = 0
brk(0)                                  = 0xef6000
brk(0xef71c0)                           = 0xef71c0
arch_prctl(ARCH_SET_FS, 0xef6880)       = 0
set_tid_address(0xef6b50)               = 24314
set_robust_list(0xef6b60, 0x18)         = 0
futex(0x7ffe5155b000, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME,
1, NULL, ef6880) = -1 EAGAIN (Resource temporarily unavailable)
rt_sigaction(SIGRTMIN, {0x405a10, [], SA_RESTORER|SA_SIGINFO,
0x4060d0}, NULL, 8) = 0
rt_sigaction(SIGRT_1, {0x405aa0, [],
SA_RESTORER|SA_RESTART|SA_SIGINFO, 0x4060d0}, NULL, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
getrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0
readlink("/proc/self/exe", "/root/a.out", 4096) = 11
brk(0xf181c0)                           = 0xf181c0
brk(0xf19000)                           = 0xf19000
access("/etc/ld.so.nohwcap", F_OK)      = -1 ENOENT (No such file or directory)
mmap(0x20000000, 1048576, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
open("/dev/ptmx", O_RDONLY|O_NOCTTY)    = 3
ioctl(3, TIOCSETD, [21])                = 0
mmap(NULL, 8392704, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x7fd129b91000
mprotect(0x7fd129b91000, 4096, PROT_NONE) = 0
clone(Process 24315 attached
child_stack=0x7fd12a390e70,
flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fd12a3919d0, tls=0x7fd12a391700,
child_tidptr=0x7fd12a3919d0) = 24315
[pid 24314] mmap(NULL, 8392704, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x7fd129390000
[pid 24315] set_robust_list(0x7fd12a3919e0, 0x18 <unfinished ...>
[pid 24314] mprotect(0x7fd129390000, 4096, PROT_NONE <unfinished ...>
[pid 24315] <... set_robust_list resumed> ) = 0
[pid 24314] <... mprotect resumed> )    = 0
[pid 24315] open("/dev/ptmx", O_RDONLY|O_NOCTTY <unfinished ...>
[pid 24314] clone(Process 24316 attached
child_stack=0x7fd129b8fe70,
flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fd129b909d0, tls=0x7fd129b90700,
child_tidptr=0x7fd129b909d0) = 24316
[pid 24314] nanosleep({0, 10000000},  <unfinished ...>
[pid 24315] <... open resumed> )        = 4
[pid 24315] madvise(0x7fd129b91000, 8368128, MADV_DONTNEED) = 0
[pid 24315] _exit(0)                    = ?
Process 24315 detached
[pid 24316] set_robust_list(0x7fd129b909e0, 0x18) = 0
[pid 24316] ioctl(4, TIOCSETD <unfinished ...>
[pid 24314] <... nanosleep resumed> NULL) = 0
[pid 24314] exit_group(0)               = ?


On commit 8005c49d9aea74d382f474ce11afbbc7d7130bec.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1276649 — [PATCH] TTY: n_gsm, fix false positive WARN_ON

Dmitry reported, that the current cleanup code in n_gsm can trigger a
warning:
WARNING: CPU: 2 PID: 24238 at drivers/tty/n_gsm.c:2048 gsm_cleanup_mux+0x166/0x6b0()
...
Call Trace:
...
 [<ffffffff81247ab9>] warn_slowpath_null+0x29/0x30 kernel/panic.c:490
 [<ffffffff828d0456>] gsm_cleanup_mux+0x166/0x6b0 drivers/tty/n_gsm.c:2048
 [<ffffffff828d4d87>] gsmld_open+0x5b7/0x7a0 drivers/tty/n_gsm.c:2386
 [<ffffffff828b9078>] tty_ldisc_open.isra.2+0x78/0xd0 drivers/tty/tty_ldisc.c:447
 [<ffffffff828b973a>] tty_set_ldisc+0x1ca/0xa70 drivers/tty/tty_ldisc.c:567
 [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
 [<ffffffff828a14ea>] tty_ioctl+0xb2a/0x2140 drivers/tty/tty_io.c:2883
...

But this is a legal path when open fails to find a space in the
gsm_mux array and tries to clean up. So make it a standard test
instead of a warning.

Reported-by: "Dmitry Vyukov" <dvyukov@google.com>
Cc: Alan Cox <alan@linux.intel.com>
Link: http://lkml.kernel.org/r/CACT4Y+bHQbAB68VFi7Romcs-Z9ZW3kQRvcq+BvHH1oa5NcAdLA@mail.gmail.com
Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/n_gsm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c3fe026d3168..9aff37186246 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2045,7 +2045,9 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 		}
 	}
 	spin_unlock(&gsm_mux_lock);
-	WARN_ON(i == MAX_MUX);
+	/* open failed before registering => nothing to do */
+	if (i == MAX_MUX)
+		return;
 
 	/* In theory disconnecting DLCI 0 is sufficient but for some
 	   modems this is apparently not the case. */
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1277056 — Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON

hi, Jiri
	This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()"). When gsm driver failed to activate one mux,there is memory leak. So I call this ->cleanup() to do the cleanup work. Seems I did not consider all cases.

I have one confusion. As there is field gsm->num to store the index of gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find this mux?

In error handle path, for example, the call trace in this patch, as we failed to activate it and the
gsm->num is invalid(and the value is 0). we can just modify the codes like below:

if(gsm_mux[gsm->num] == gsm)
....other work
else
	return;

I think it would work, and the logic is correct. Or I just miss something important?

thanks
xinhui

On 2015/11/25 00:54, Jiri Slaby wrote:
> Dmitry reported, that the current cleanup code in n_gsm can trigger a
> warning:
> WARNING: CPU: 2 PID: 24238 at drivers/tty/n_gsm.c:2048 gsm_cleanup_mux+0x166/0x6b0()
> ...
> Call Trace:
> ...
>   [<ffffffff81247ab9>] warn_slowpath_null+0x29/0x30 kernel/panic.c:490
>   [<ffffffff828d0456>] gsm_cleanup_mux+0x166/0x6b0 drivers/tty/n_gsm.c:2048
>   [<ffffffff828d4d87>] gsmld_open+0x5b7/0x7a0 drivers/tty/n_gsm.c:2386
>   [<ffffffff828b9078>] tty_ldisc_open.isra.2+0x78/0xd0 drivers/tty/tty_ldisc.c:447
>   [<ffffffff828b973a>] tty_set_ldisc+0x1ca/0xa70 drivers/tty/tty_ldisc.c:567
>   [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
>   [<ffffffff828a14ea>] tty_ioctl+0xb2a/0x2140 drivers/tty/tty_io.c:2883
> ...
>
> But this is a legal path when open fails to find a space in the
> gsm_mux array and tries to clean up. So make it a standard test
> instead of a warning.
>
> Reported-by: "Dmitry Vyukov" <dvyukov@google.com>
> Cc: Alan Cox <alan@linux.intel.com>
> Link: http://lkml.kernel.org/r/CACT4Y+bHQbAB68VFi7Romcs-Z9ZW3kQRvcq+BvHH1oa5NcAdLA@mail.gmail.com
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>   drivers/tty/n_gsm.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c3fe026d3168..9aff37186246 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2045,7 +2045,9 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
>   		}
>   	}
>   	spin_unlock(&gsm_mux_lock);
> -	WARN_ON(i == MAX_MUX);
> +	/* open failed before registering => nothing to do */
> +	if (i == MAX_MUX)
> +		return;
>
>   	/* In theory disconnecting DLCI 0 is sufficient but for some
>   	   modems this is apparently not the case. */
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1277147 — Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON

Hi,

On 11/25/2015, 07:32 AM, xinhui wrote:
>     This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
> memory leak in gsmld_open()").

Oh, yes, I messed up the "Fixes" line then. It should write:
Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")

> I have one confusion. As there is field gsm->num to store the index of
> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
> this mux?
> 
> In error handle path, for example, the call trace in this patch, as we
> failed to activate it and the
> gsm->num is invalid(and the value is 0). we can just modify the codes
> like below:
> 
> if(gsm_mux[gsm->num] == gsm)
> ....other work
> else
>     return;
> 
> I think it would work, and the logic is correct. Or I just miss
> something important?

Yup, it looks like a cleanup. Could you prepare a separate patch for that?

Something like this:
        /* open failed before registering => nothing to do */
        if (gsm_mux[gsm->num] != gsm)
              return;
        spin_lock(&gsm_mux_lock);
        gsm_mux[gsm->num] = NULL;
        spin_unlock(&gsm_mux_lock);

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1277176 — Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON

hi, Jiri

On 2015/11/25 17:56, Jiri Slaby wrote:
> Hi,
>
> On 11/25/2015, 07:32 AM, xinhui wrote:
>>      This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
>> memory leak in gsmld_open()").
>
> Oh, yes, I messed up the "Fixes" line then. It should write:
> Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
>
that's Okay. :)

>> I have one confusion. As there is field gsm->num to store the index of
>> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
>> this mux?
>>
>> In error handle path, for example, the call trace in this patch, as we
>> failed to activate it and the
>> gsm->num is invalid(and the value is 0). we can just modify the codes
>> like below:
>>
>> if(gsm_mux[gsm->num] == gsm)
>> ....other work
>> else
>>      return;
>>
>> I think it would work, and the logic is correct. Or I just miss
>> something important?
>
> Yup, it looks like a cleanup. Could you prepare a separate patch for that?
>
yes, I will do that :)

> Something like this:
>          /* open failed before registering => nothing to do */
>          if (gsm_mux[gsm->num] != gsm)
>                return;
>          spin_lock(&gsm_mux_lock);
>          gsm_mux[gsm->num] = NULL;
>          spin_unlock(&gsm_mux_lock);
>
looks pretty good, thanks.

> thanks,
>
thanks
xinhui

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web