[PATCH] TTY: n_gsm, fix false positive WARN_ON

[PATCH] TTY: n_gsm, fix false positive WARN_ON

[Jiri Slaby]

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

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

[xinhui]

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. */
>

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

[Jiri Slaby]

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

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

[xinhui]

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

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

[Dmitry Vyukov]

On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> 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.


This is still not merged and fires regularly for me. Can we please merge it?

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

[Greg Kroah-Hartman]

On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> > 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.
> 
> 
> This is still not merged and fires regularly for me. Can we please merge it?

merge what?  I don't see any patch here or in my queue for this :(

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

[Dmitry Vyukov]

On Tue, Mar 1, 2016 at 6:01 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
>> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
>> > 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.
>>
>>
>> This is still not merged and fires regularly for me. Can we please merge it?
>
> merge what?  I don't see any patch here or in my queue for this :(


"[PATCH] TTY: n_gsm, fix false positive WARN_ON" from Jiri Slaby:
https://lkml.org/lkml/2015/11/24/600

FWIW I have it in my tree for 3 months. Warnings have gone. No issues noticed.

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

[Greg Kroah-Hartman]

On Tue, Mar 01, 2016 at 10:03:30AM +0100, Dmitry Vyukov wrote:
> On Tue, Mar 1, 2016 at 6:01 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
> >> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> >> > 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.
> >>
> >>
> >> This is still not merged and fires regularly for me. Can we please merge it?
> >
> > merge what?  I don't see any patch here or in my queue for this :(
> 
> 
> "[PATCH] TTY: n_gsm, fix false positive WARN_ON" from Jiri Slaby:
> https://lkml.org/lkml/2015/11/24/600
> 
> FWIW I have it in my tree for 3 months. Warnings have gone. No issues noticed.

I don't see it in my queue, nor in Linus's tree, so someone needs to
resend it if they want it merged into the kernel tree...

{hint}

[PATCH] TTY: n_gsm, fix false positive WARN_ON

[Jiri Slaby]

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: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
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 c01620780f5b..365dfd8bc42b 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.7.4