[PATCH 1/2] tty: n_gsm: fix race condition in status line change on dead connections

[PATCH 1/2] tty: n_gsm: fix race condition in status line change on dead connections

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

gsm_cleanup_mux() cleans up the gsm by closing all DLCIs, stopping all
timers, removing the virtual tty devices and clearing the data queues.
This procedure, however, may cause subsequent changes of the virtual modem
status lines of a DLCI. More data is being added the outgoing data queue
and the deleted kick timer is restarted to handle this. At this point many
resources have already been removed by the cleanup procedure. Thus, a
kernel panic occurs.

Fix this by proving in gsm_modem_update() that the cleanup procedure has
not been started and the mux is still alive.

Note that writing to a virtual tty is already protected by checks against
the DLCI specific connection state.

Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1f3aba607cd5..0ee7531c9201 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -4108,6 +4108,8 @@ static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
 
 static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
 {
+	if (dlci->gsm->dead)
+		return -EL2HLT;
 	if (dlci->adaption == 2) {
 		/* Send convergence layer type 2 empty data frame. */
 		gsm_modem_upd_via_data(dlci, brk);
-- 
2.34.1

[PATCH 2/2] tty: n_gsm: add partial copyright Siemens Mobility GmbH

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

More than 1/3 of the n_gsm code has been contributed by us in the last
1.5 years, completing conformance with the standard and stabilizing the
driver:
- added UI (unnumbered information) frame support
- added PN (parameter negotiation) message handling and function support
- added optional keep-alive control link supervision via test messages
- added TIOCM_OUT1 and TIOCM_OUT2 to allow responder to operate as modem
- added TIOCMIWAIT support on virtual ttys
- added additional ioctls and parameters to configure the new functions
- added overall locking mechanism to avoid data race conditions
- added outgoing data flow to decouple physical from virtual tty handling
  for better performance and to avoid dead-locks
- fixed advanced option mode implementation
- fixed convergence layer type 2 implementation
- fixed handling of CLD (multiplexer close down) messages
- fixed broken muxer close down procedure
- and many more bug fixes

With this most of our initial RFC has been implemented. It gives the driver
a quality boost unseen in the decade before.

Add a partial copyright notice to the n_gsm files to highlight this
contribution.

Link: https://lore.kernel.org/all/20220225080758.2869-1-daniel.starke@siemens.com/
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c         | 1 +
 include/uapi/linux/gsmmux.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0ee7531c9201..fa882c7f4770 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2,6 +2,7 @@
 /*
  * n_gsm.c GSM 0710 tty multiplexor
  * Copyright (c) 2009/10 Intel Corporation
+ * Portions Copyright (c) 2022/23 Siemens Mobility GmbH
  *
  *	* THIS IS A DEVELOPMENT SNAPSHOT IT IS NOT A FINAL RELEASE *
  *
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index 4c878d84dbda..101ebd15954e 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* Portions Copyright (c) 2022/23 Siemens Mobility GmbH */
 #ifndef _LINUX_GSMMUX_H
 #define _LINUX_GSMMUX_H
 
-- 
2.34.1

Re: [PATCH 1/2] tty: n_gsm: fix race condition in status line change on dead connections

[Greg KH]

On Thu, Oct 26, 2023 at 07:58:43AM +0200, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> gsm_cleanup_mux() cleans up the gsm by closing all DLCIs, stopping all
> timers, removing the virtual tty devices and clearing the data queues.
> This procedure, however, may cause subsequent changes of the virtual modem
> status lines of a DLCI. More data is being added the outgoing data queue
> and the deleted kick timer is restarted to handle this. At this point many
> resources have already been removed by the cleanup procedure. Thus, a
> kernel panic occurs.
> 
> Fix this by proving in gsm_modem_update() that the cleanup procedure has
> not been started and the mux is still alive.
> 
> Note that writing to a virtual tty is already protected by checks against
> the DLCI specific connection state.
> 
> Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links")
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 1f3aba607cd5..0ee7531c9201 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -4108,6 +4108,8 @@ static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
>  
>  static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
>  {
> +	if (dlci->gsm->dead)
> +		return -EL2HLT;
>  	if (dlci->adaption == 2) {
>  		/* Send convergence layer type 2 empty data frame. */
>  		gsm_modem_upd_via_data(dlci, brk);
> -- 
> 2.34.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH 2/2] tty: n_gsm: add partial copyright Siemens Mobility GmbH

[Greg KH]

On Thu, Oct 26, 2023 at 07:58:44AM +0200, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> More than 1/3 of the n_gsm code has been contributed by us in the last
> 1.5 years, completing conformance with the standard and stabilizing the
> driver:
> - added UI (unnumbered information) frame support
> - added PN (parameter negotiation) message handling and function support
> - added optional keep-alive control link supervision via test messages
> - added TIOCM_OUT1 and TIOCM_OUT2 to allow responder to operate as modem
> - added TIOCMIWAIT support on virtual ttys
> - added additional ioctls and parameters to configure the new functions
> - added overall locking mechanism to avoid data race conditions
> - added outgoing data flow to decouple physical from virtual tty handling
>   for better performance and to avoid dead-locks
> - fixed advanced option mode implementation
> - fixed convergence layer type 2 implementation
> - fixed handling of CLD (multiplexer close down) messages
> - fixed broken muxer close down procedure
> - and many more bug fixes
> 
> With this most of our initial RFC has been implemented. It gives the driver
> a quality boost unseen in the decade before.
> 
> Add a partial copyright notice to the n_gsm files to highlight this
> contribution.
> 
> Link: https://lore.kernel.org/all/20220225080758.2869-1-daniel.starke@siemens.com/
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c         | 1 +
>  include/uapi/linux/gsmmux.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 0ee7531c9201..fa882c7f4770 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2,6 +2,7 @@
>  /*
>   * n_gsm.c GSM 0710 tty multiplexor
>   * Copyright (c) 2009/10 Intel Corporation
> + * Portions Copyright (c) 2022/23 Siemens Mobility GmbH

I have no objection to you adding your copyright, you all have done a
ton of great work here and it is correct to add.  But the "Portions"
line is odd, and isn't something we generally use.  Can you just resend
this with that word removed?

>   *
>   *	* THIS IS A DEVELOPMENT SNAPSHOT IT IS NOT A FINAL RELEASE *
>   *
> diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> index 4c878d84dbda..101ebd15954e 100644
> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -1,4 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Portions Copyright (c) 2022/23 Siemens Mobility GmbH */

Same here, just drop the "Portions"?

Odd that Intel didn't put their copyright here, but as we all know,
absence of that line does not mean that someone does not have copyright
on it, lines like these are lawyer cargo-cult-safety, so we play along
with them :)

thanks,

greg k-h

Re: [PATCH 1/2] tty: n_gsm: fix race condition in status line change on dead connections

[Greg KH]

On Thu, Oct 26, 2023 at 10:27:37AM +0200, Greg KH wrote:
> On Thu, Oct 26, 2023 at 07:58:43AM +0200, D. Starke wrote:
> > From: Daniel Starke <daniel.starke@siemens.com>
> > 
> > gsm_cleanup_mux() cleans up the gsm by closing all DLCIs, stopping all
> > timers, removing the virtual tty devices and clearing the data queues.
> > This procedure, however, may cause subsequent changes of the virtual modem
> > status lines of a DLCI. More data is being added the outgoing data queue
> > and the deleted kick timer is restarted to handle this. At this point many
> > resources have already been removed by the cleanup procedure. Thus, a
> > kernel panic occurs.
> > 
> > Fix this by proving in gsm_modem_update() that the cleanup procedure has
> > not been started and the mux is still alive.
> > 
> > Note that writing to a virtual tty is already protected by checks against
> > the DLCI specific connection state.
> > 
> > Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links")
> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >  drivers/tty/n_gsm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 1f3aba607cd5..0ee7531c9201 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -4108,6 +4108,8 @@ static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
> >  
> >  static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
> >  {
> > +	if (dlci->gsm->dead)
> > +		return -EL2HLT;
> >  	if (dlci->adaption == 2) {
> >  		/* Send convergence layer type 2 empty data frame. */
> >  		gsm_modem_upd_via_data(dlci, brk);
> > -- 
> > 2.34.1
> > 
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - You have marked a patch with a "Fixes:" tag for a commit that is in an
>   older released kernel, yet you do not have a cc: stable line in the
>   signed-off-by area at all, which means that the patch will not be
>   applied to any older kernel releases.  To properly fix this, please
>   follow the documented rules in the
>   Documentation/process/stable-kernel-rules.rst file for how to resolve
>   this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 

Note, I'll take this now, and add the tag by hand, just try to remember
it for the future.

thanks,

greg k-h

RE: [PATCH 2/2] tty: n_gsm: add partial copyright Siemens Mobility GmbH

[Starke, Daniel]

> > + * Portions Copyright (c) 2022/23 Siemens Mobility GmbH
> 
> I have no objection to you adding your copyright, you all have done a
> ton of great work here and it is correct to add.  But the "Portions"
> line is odd, and isn't something we generally use.  Can you just resend
> this with that word removed?
> 
> >   *
> >   *	* THIS IS A DEVELOPMENT SNAPSHOT IT IS NOT A FINAL RELEASE *
> >   *
> > diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> > index 4c878d84dbda..101ebd15954e 100644
> > --- a/include/uapi/linux/gsmmux.h
> > +++ b/include/uapi/linux/gsmmux.h
> > @@ -1,4 +1,5 @@
> >  /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* Portions Copyright (c) 2022/23 Siemens Mobility GmbH */
> 
> Same here, just drop the "Portions"?

This was something suggested by our IP department. But sure, I will resend
this patch without the word included.

Best regards,
Daniel Starke