linux-kernel - RE: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled links

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <DB9PR10MB5881A13C22F91D7F5834E71EE0D49@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
Date:   Mon, 23 May 2022 12:35:20 +0000
From:   "Starke, Daniel" <daniel.starke@...mens.com>
To:     Jiri Slaby <jirislaby@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled
 links

> > From: Daniel Starke <daniel.starke@...mens.com>
> > 
> > The current implementation does not handle the situation that no data 
> > is in the internal queue and needs to be sent out while the user tty 
> > fifo is full.
> > Add a timer that moves more data from user tty down to the internal 
> > queue which is then serialized on the ldisc. This timer is triggered 
> > if no data was moved from a user tty to the internal queue within 10 * T1.
> > 
> > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Daniel Starke <daniel.starke@...mens.com>
> > ---
> >   drivers/tty/n_gsm.c | 43 +++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 35 insertions(+), 8 deletions(-)
> > 
> > See patch 6 regarding changes since to v1.
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > 0a9924445968..3a4a2394d970 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> ...
> > @@ -833,6 +834,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
> >   	list_add_tail(&msg->list, &gsm->tx_list);
> >   	gsm->tx_bytes += msg->len;
> >   	gsm_data_kick(gsm, dlci);
> > +	mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100);
> 
> The formula deserves an explanation. And why 10 * X / 100, and not X / 10?

T1 is defined as 1/100th of a second (see chapter 5.7.1 of the standard).
Therefore, it is gsm->t1 * HZ / 100. I chose 10x T1 as this case should
usually not occur and only acts as a final countermeasure against a
stalled link. Or are there any other suggestions for a proper kick timer
value?

> > @@ -1062,9 +1058,9 @@ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci,
> >    *	renegotiate DLCI priorities with optional stuff. Needs optimising.
> >    */
> >   
> > -static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
> > +static int gsm_dlci_data_sweep(struct gsm_mux *gsm)
> >   {
> > -	int len;
> > +	int len, ret = 0;
> 
> Why is ret signed?

Many obviously only unsigned values are signed in the code of the original
author. I simply aligned my code to this to believe that int is preferred.
But I can change it to unsigned int if this is preferred here?

Best regards,
Daniel Starke