[v3,2/4] tty: n_gsm: add keep alive support

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

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapters 5.4.6.3.4 and 5.1.8.1.3 describe the test
command which can be used to test the mux connection between both sides.

Currently, no algorithm is implemented to make use of this command. This
requires that each multiplexed upper layer protocol supervises the
underlying muxer connection to handle possible connection losses.

Introduce an ioctl commands and functions to optionally enable keep alive
handling via the test command as described in chapter 5.4.6.3.4. A single
incrementing octet is being used to distinguish between multiple retries.
Retry count and interval are taken from the general parameters N2 and T2.

Note that support for the test command is mandatory and already present in
the muxer implementation since the very first version.
Also note that the previous ioctl structure gsm_config cannot be extended
due to missing checks against zero of the field "unused".

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c         | 106 +++++++++++++++++++++++++++++++++++-
 include/uapi/linux/gsmmux.h |  10 ++++
 2 files changed, 114 insertions(+), 2 deletions(-)

v2 -> v3:
Split previous patch 1/3 into two commits as recommended in the review.
No other changes.

Link: https://lore.kernel.org/all/Y9vYxgGd6kC+ZIgR@kroah.com/
  

On Fri, Feb 03, 2023 at 03:50:21PM +0100, D. Starke wrote:
> +struct gsm_config_ext {
> +	__u32 keep_alive;	/* Control channel keep-alive in 1/100th of a
> +				 * second (0 to disable)
> +				 */
> +	__u32 reserved[7];	/* For future use */

You are not checking these fields, so this means any future use of them
will not be allowed (like the problem you have which required this new
structure.)

Please, always verify that the values here are 0, and if not, error out
with -EINVAL.  That's the only way you can properly reserve this for
future use.  The kernel documentation about "how to write an ioctl"
should describe all of this, right?

thanks,

greg k-h
  

> > +struct gsm_config_ext {
> > +	__u32 keep_alive;	/* Control channel keep-alive in 1/100th of a
> > +				 * second (0 to disable)
> > +				 */
> > +	__u32 reserved[7];	/* For future use */
> 
> You are not checking these fields, so this means any future use of them
> will not be allowed (like the problem you have which required this new
> structure.)
> 
> Please, always verify that the values here are 0, and if not, error out
> with -EINVAL.  That's the only way you can properly reserve this for
> future use.  The kernel documentation about "how to write an ioctl"
> should describe all of this, right?

gsmld_ioctl() calls for GSMIOC_SETCONF_EXT the function gsm_config_ext()
which returns -EINVAL if any of the reserved fields is non-zero. Should
this also happen for GSMIOC_GETCONF_EXT?

Best regards,
Daniel Starke
  

On Fri, Feb 03, 2023 at 03:50:21PM +0100, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapters 5.4.6.3.4 and 5.1.8.1.3 describe the test
> command which can be used to test the mux connection between both sides.
> 
> Currently, no algorithm is implemented to make use of this command. This
> requires that each multiplexed upper layer protocol supervises the
> underlying muxer connection to handle possible connection losses.
> 
> Introduce an ioctl commands and functions to optionally enable keep alive
> handling via the test command as described in chapter 5.4.6.3.4. A single
> incrementing octet is being used to distinguish between multiple retries.
> Retry count and interval are taken from the general parameters N2 and T2.
> 
> Note that support for the test command is mandatory and already present in
> the muxer implementation since the very first version.
> Also note that the previous ioctl structure gsm_config cannot be extended
> due to missing checks against zero of the field "unused".
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c         | 106 +++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/gsmmux.h |  10 ++++
>  2 files changed, 114 insertions(+), 2 deletions(-)
> 
> v2 -> v3:
> Split previous patch 1/3 into two commits as recommended in the review.
> No other changes.
> 
> Link: https://lore.kernel.org/all/Y9vYxgGd6kC+ZIgR@kroah.com/
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5783801d6524..d068df1cf2fd 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -318,6 +318,11 @@ struct gsm_mux {
>  	struct gsm_control *pending_cmd;/* Our current pending command */
>  	spinlock_t control_lock;	/* Protects the pending command */
>  
> +	/* Keep-alive */
> +	struct timer_list ka_timer;	/* Keep-alive response timer */
> +	u8 ka_num;			/* Keep-alive match pattern */
> +	int ka_retries;			/* Keep-alive retry counter */
> +
>  	/* Configuration */
>  	int adaption;		/* 1 or 2 supported */
>  	u8 ftype;		/* UI or UIH */
> @@ -325,6 +330,7 @@ struct gsm_mux {
>  	unsigned int t3;	/* Power wake-up timer in seconds. */
>  	int n2;			/* Retry count */
>  	u8 k;			/* Window size */
> +	u32 keep_alive;		/* Control channel keep-alive in ms */
>  
>  	/* Statistics (not currently exposed) */
>  	unsigned long bad_fcs;
> @@ -1897,11 +1903,13 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  						const u8 *data, int clen)
>  {
>  	struct gsm_control *ctrl;
> +	struct gsm_dlci *dlci;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&gsm->control_lock, flags);
>  
>  	ctrl = gsm->pending_cmd;
> +	dlci = gsm->dlci[0];
>  	command |= 1;
>  	/* Does the reply match our command */
>  	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
> @@ -1916,6 +1924,54 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  	/* Or did we receive the PN response to our PN command */
>  	} else if (command == CMD_PN) {
>  		gsm_control_negotiation(gsm, 0, data, clen);
> +	/* Or did we receive the TEST response to our TEST command */
> +	} else if (command == CMD_TEST && clen == 1 && *data == gsm->ka_num) {
> +		gsm->ka_retries = -1; /* trigger new keep-alive message */
> +		if (dlci && !dlci->dead)
> +			mod_timer(&gsm->ka_timer,
> +				  jiffies + gsm->keep_alive * HZ / 100);
> +	}
> +	spin_unlock_irqrestore(&gsm->control_lock, flags);
> +}
> +
> +/**
> + * gsm_control_keep_alive	-	check timeout or start keep-alive
> + * @t: timer contained in our gsm object
> + *
> + * Called off the keep-alive timer expiry signaling that our link
> + * partner is not responding anymore. Link will be closed.
> + * This is also called to startup our timer.
> + */
> +
> +static void gsm_control_keep_alive(struct timer_list *t)
> +{
> +	struct gsm_mux *gsm = from_timer(gsm, t, ka_timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&gsm->control_lock, flags);
> +	if (gsm->ka_num && gsm->ka_retries == 0) {
> +		/* Keep-alive expired -> close the link */
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s keep-alive timed out\n", __func__);
> +		spin_unlock_irqrestore(&gsm->control_lock, flags);
> +		if (gsm->dlci[0])
> +			gsm_dlci_begin_close(gsm->dlci[0]);
> +		return;
> +	} else if (gsm->keep_alive && gsm->dlci[0] && !gsm->dlci[0]->dead) {
> +		if (gsm->ka_retries > 0) {
> +			/* T2 expired for keep-alive -> resend */
> +			gsm->ka_retries--;
> +		} else {
> +			/* Start keep-alive timer */
> +			gsm->ka_num++;
> +			if (!gsm->ka_num)
> +				gsm->ka_num++;
> +			gsm->ka_retries = gsm->n2;
> +		}
> +		gsm_control_command(gsm, CMD_TEST, &gsm->ka_num,
> +				    sizeof(gsm->ka_num));
> +		mod_timer(&gsm->ka_timer,
> +			  jiffies + gsm->t2 * HZ / 100);
>  	}
>  	spin_unlock_irqrestore(&gsm->control_lock, flags);
>  }
> @@ -2061,8 +2117,10 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
>  		/* Ensure that gsmtty_open() can return. */
>  		tty_port_set_initialized(&dlci->port, false);
>  		wake_up_interruptible(&dlci->port.open_wait);
> -	} else
> +	} else {
> +		del_timer(&dlci->gsm->ka_timer);
>  		dlci->gsm->dead = true;
> +	}
>  	/* A DLCI 0 close is a MUX termination so we need to kick that
>  	   back to userspace somehow */
>  	gsm_dlci_data_kick(dlci);
> @@ -2078,6 +2136,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
>  
>  static void gsm_dlci_open(struct gsm_dlci *dlci)
>  {
> +	struct gsm_mux *gsm = dlci->gsm;
> +
>  	/* Note that SABM UA .. SABM UA first UA lost can mean that we go
>  	   open -> open */
>  	del_timer(&dlci->t1);
> @@ -2087,8 +2147,15 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
>  	if (debug & DBG_ERRORS)
>  		pr_debug("DLCI %d goes open.\n", dlci->addr);
>  	/* Send current modem state */
> -	if (dlci->addr)
> +	if (dlci->addr) {
>  		gsm_modem_update(dlci, 0);
> +	} else {
> +		/* Start keep-alive control */
> +		gsm->ka_num = 0;
> +		gsm->ka_retries = -1;
> +		mod_timer(&gsm->ka_timer,
> +			  jiffies + gsm->keep_alive * HZ / 100);
> +	}
>  	gsm_dlci_data_kick(dlci);
>  	wake_up(&dlci->gsm->event);
>  }
> @@ -2840,6 +2907,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
>  	/* Finish outstanding timers, making sure they are done */
>  	del_timer_sync(&gsm->kick_timer);
>  	del_timer_sync(&gsm->t2_timer);
> +	del_timer_sync(&gsm->ka_timer);
>  
>  	/* Finish writing to ldisc */
>  	flush_work(&gsm->tx_work);
> @@ -2987,6 +3055,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
>  	INIT_LIST_HEAD(&gsm->tx_data_list);
>  	timer_setup(&gsm->kick_timer, gsm_kick_timer, 0);
>  	timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
> +	timer_setup(&gsm->ka_timer, gsm_control_keep_alive, 0);
>  	INIT_WORK(&gsm->tx_work, gsmld_write_task);
>  	init_waitqueue_head(&gsm->event);
>  	spin_lock_init(&gsm->control_lock);
> @@ -3003,6 +3072,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
>  	gsm->mru = 64;	/* Default to encoding 1 so these should be 64 */
>  	gsm->mtu = 64;
>  	gsm->dead = true;	/* Avoid early tty opens */
> +	gsm->keep_alive = 0;	/* Disabled */
>  
>  	/* Store the instance to the mux array or abort if no space is
>  	 * available.
> @@ -3138,6 +3208,28 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  	return 0;
>  }
>  
> +static void gsm_copy_config_ext_values(struct gsm_mux *gsm,
> +				       struct gsm_config_ext *ce)
> +{
> +	memset(ce, 0, sizeof(*ce));
> +	ce->keep_alive = gsm->keep_alive;
> +}
> +
> +static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)
> +{
> +	unsigned int i;
> +
> +	gsm->keep_alive = ce->keep_alive;
> +	/*
> +	 * Check that userspace doesn't put stuff in here to prevent breakages
> +	 * in the future.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(ce->reserved); i++)
> +		if (ce->reserved[i])
> +			return -EINVAL;

Do the check before you save off the keep_alive variable?

Sorry I missed this check before.

thanks,

greg k-h
  

> > +static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)
> > +{
> > +	unsigned int i;
> > +
> > +	gsm->keep_alive = ce->keep_alive;
> > +	/*
> > +	 * Check that userspace doesn't put stuff in here to prevent breakages
> > +	 * in the future.
> > +	 */
> > +	for (i = 0; i < ARRAY_SIZE(ce->reserved); i++)
> > +		if (ce->reserved[i])
> > +			return -EINVAL;
> 
> Do the check before you save off the keep_alive variable?

Thank you for this hint!
I will change this and send a new version of this patch series.

Best regards,
Daniel Starke