RE: [PATCH v2 1/9] tty: n_gsm: fix user open not possible at responder until initiator open

RE: [PATCH v2 1/9] tty: n_gsm: fix user open not possible at responder until initiator open

[Starke, Daniel]

> > +/**
> > + *	gsm_dlci_wait_open	-	wait for channel open procedure
> > + *	@dlci: DLCI to open
> > + *
> > + *	Wait for a DLCI opening from the other side. Asynchronously wait until
> > + *	we get a SABM and set off timers and the responses.
> > + */
> > +static void gsm_dlci_wait_open(struct gsm_dlci *dlci) {
> > +	switch (dlci->state) {
> > +	case DLCI_CLOSED:
> > +	case DLCI_CLOSING:
> > +		dlci->state = DLCI_OPENING;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> 
> The documentation for this function is odd, you are not waiting for
> anything.  You are just changing the state.  This makes no sense as-is,
> sorry.

Thank you for your comment. I have prepared a new version. I will send it
after the other 8 patches have been commented.

Best regards,
Daniel Starke

Re: [PATCH v2 1/9] tty: n_gsm: fix user open not possible at responder until initiator open

[Greg KH]

On Mon, May 23, 2022 at 08:45:06AM +0000, Starke, Daniel wrote:
> > > +/**
> > > + *	gsm_dlci_wait_open	-	wait for channel open procedure
> > > + *	@dlci: DLCI to open
> > > + *
> > > + *	Wait for a DLCI opening from the other side. Asynchronously wait until
> > > + *	we get a SABM and set off timers and the responses.
> > > + */
> > > +static void gsm_dlci_wait_open(struct gsm_dlci *dlci) {
> > > +	switch (dlci->state) {
> > > +	case DLCI_CLOSED:
> > > +	case DLCI_CLOSING:
> > > +		dlci->state = DLCI_OPENING;
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +}
> > 
> > The documentation for this function is odd, you are not waiting for
> > anything.  You are just changing the state.  This makes no sense as-is,
> > sorry.
> 
> Thank you for your comment. I have prepared a new version. I will send it
> after the other 8 patches have been commented.

No, please fix up and resend the series, it is long gone from my review
queue.

thanks,

greg k-h

Re: [PATCH v2 1/9] tty: n_gsm: fix user open not possible at responder until initiator open

[Greg KH]

On Thu, May 19, 2022 at 09:07:49AM +0200, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> After setting up the control channel on both sides the responder side may
> want to open a virtual tty to listen on until the initiator starts an
> application on a user channel. The current implementation allows the
> open() but no other operation, like termios. These fail with EINVAL.
> The responder sided application has no means to detect an open by the
> initiator sided application this way. And the initiator sided applications
> usually expect the responder sided application to listen on the user
> channel upon open.
> Set the user channel into half-open state on responder side once a user
> application opens the virtual tty to allow IO operations on it.
> Furthermore, keep the user channel constipated until the initiator side
> opens it to give the responder sided application the chance to detect the
> new connection and to avoid data loss if the responder sided application
> starts sending before the user channel is open.
> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> This commit was not changed as there have been no comments on it in v1.
> 
> Link: https://lore.kernel.org/all/20220506144725.1946-1-daniel.starke@siemens.com/
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index fd8b86dde525..08fea3e7674d 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1493,6 +1493,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
>  	if (debug & 8)
>  		pr_debug("DLCI %d goes closed.\n", dlci->addr);
>  	dlci->state = DLCI_CLOSED;
> +	/* Prevent us from sending data before the link is up again */
> +	dlci->constipated = true;
>  	if (dlci->addr != 0) {
>  		tty_port_tty_hangup(&dlci->port, false);
>  		spin_lock_irqsave(&dlci->lock, flags);
> @@ -1522,6 +1524,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
>  	del_timer(&dlci->t1);
>  	/* This will let a tty open continue */
>  	dlci->state = DLCI_OPEN;
> +	dlci->constipated = false;
>  	if (debug & 8)
>  		pr_debug("DLCI %d goes open.\n", dlci->addr);
>  	/* Send current modem state */
> @@ -1602,6 +1605,25 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>  	mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
>  }
>  
> +/**
> + *	gsm_dlci_wait_open	-	wait for channel open procedure
> + *	@dlci: DLCI to open
> + *
> + *	Wait for a DLCI opening from the other side. Asynchronously wait until
> + *	we get a SABM and set off timers and the responses.
> + */
> +static void gsm_dlci_wait_open(struct gsm_dlci *dlci)
> +{
> +	switch (dlci->state) {
> +	case DLCI_CLOSED:
> +	case DLCI_CLOSING:
> +		dlci->state = DLCI_OPENING;
> +		break;
> +	default:
> +		break;
> +	}
> +}

The documentation for this function is odd, you are not waiting for
anything.  You are just changing the state.  This makes no sense as-is,
sorry.

greg k-h

[PATCH v2 1/9] tty: n_gsm: fix user open not possible at responder until initiator open

[D. Starke]

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

After setting up the control channel on both sides the responder side may
want to open a virtual tty to listen on until the initiator starts an
application on a user channel. The current implementation allows the
open() but no other operation, like termios. These fail with EINVAL.
The responder sided application has no means to detect an open by the
initiator sided application this way. And the initiator sided applications
usually expect the responder sided application to listen on the user
channel upon open.
Set the user channel into half-open state on responder side once a user
application opens the virtual tty to allow IO operations on it.
Furthermore, keep the user channel constipated until the initiator side
opens it to give the responder sided application the chance to detect the
new connection and to avoid data loss if the responder sided application
starts sending before the user channel is open.

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

This commit was not changed as there have been no comments on it in v1.

Link: https://lore.kernel.org/all/20220506144725.1946-1-daniel.starke@siemens.com/

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index fd8b86dde525..08fea3e7674d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1493,6 +1493,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 	if (debug & 8)
 		pr_debug("DLCI %d goes closed.\n", dlci->addr);
 	dlci->state = DLCI_CLOSED;
+	/* Prevent us from sending data before the link is up again */
+	dlci->constipated = true;
 	if (dlci->addr != 0) {
 		tty_port_tty_hangup(&dlci->port, false);
 		spin_lock_irqsave(&dlci->lock, flags);
@@ -1522,6 +1524,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	del_timer(&dlci->t1);
 	/* This will let a tty open continue */
 	dlci->state = DLCI_OPEN;
+	dlci->constipated = false;
 	if (debug & 8)
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
 	/* Send current modem state */
@@ -1602,6 +1605,25 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 	mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
 }
 
+/**
+ *	gsm_dlci_wait_open	-	wait for channel open procedure
+ *	@dlci: DLCI to open
+ *
+ *	Wait for a DLCI opening from the other side. Asynchronously wait until
+ *	we get a SABM and set off timers and the responses.
+ */
+static void gsm_dlci_wait_open(struct gsm_dlci *dlci)
+{
+	switch (dlci->state) {
+	case DLCI_CLOSED:
+	case DLCI_CLOSING:
+		dlci->state = DLCI_OPENING;
+		break;
+	default:
+		break;
+	}
+}
+
 /**
  *	gsm_dlci_begin_close	-	start channel open procedure
  *	@dlci: DLCI to open
@@ -1745,10 +1767,13 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 	dlci->addr = addr;
 	dlci->adaption = gsm->adaption;
 	dlci->state = DLCI_CLOSED;
-	if (addr)
+	if (addr) {
 		dlci->data = gsm_dlci_data;
-	else
+		/* Prevent us from sending data before the link is up */
+		dlci->constipated = true;
+	} else {
 		dlci->data = gsm_dlci_command;
+	}
 	gsm->dlci[addr] = dlci;
 	return dlci;
 }
@@ -3163,6 +3188,8 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 	/* Start sending off SABM messages */
 	if (gsm->initiator)
 		gsm_dlci_begin_open(dlci);
+	else
+		gsm_dlci_wait_open(dlci);
 	/* And wait for virtual carrier */
 	return tty_port_block_til_ready(port, tty, filp);
 }
-- 
2.34.1