RE: [PATCH 16/20] tty: n_gsm: fix invalid command/response bit check for UI/UIH frames

RE: [PATCH 16/20] tty: n_gsm: fix invalid command/response bit check for UI/UIH frames

[Starke, Daniel]

> 42:21AM -0700, 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. Chapter 5.4.3.1 states the CR bit in 
> > UI and UIH frames shall always be set 1 by the initiator and 0 by the responder.
> 
> This has nothing to do with the change you made here.
> 
> 
> > Currently, gsm_queue() has a pre-processor gated (excluded) check 
> > which treats all frames that conform to the standard as malformed frames.
> > Remove this optional code to avoid confusion and possible breaking 
> > changes in case that someone includes it.
> 
> Again, nothing to do with the code change.

Including this code (i.e. with #if 1) will treat every correct UI/UIH frame
as invalid, because the cr flag is always set to 1 for those frames
(as mentioned in chapter 5.4.3.1 of the standard). This is obviously wrong.

> > 
> > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> 
> This "fixes" nothing :(

What is the correct way to handle the removal of such dead and obviously
wrong code?

> > Cc: stable@vger.kernel.org
> 
> How is commenting out unused code a stable backport requirement?

True, it does not change the behavior but it fixes a commit which is also
present in the current stable release. I was unsure how to handle this
case. I will remove the backport remark.

> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >  drivers/tty/n_gsm.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > e9a7d9483c1f..f4ec48c0d6d7 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> >  	case UI|PF:
> >  	case UIH:
> >  	case UIH|PF:
> > -#if 0
> > -		if (cr)
> > -			goto invalid;
> > -#endif
> 
> All you are doing is cleaning up dead code.  Not a big deal, and not
> worth all the text above to confuse people :(

As mentioned above, this is not only dead but also wrong code. I tried to
elaborate the reason for it being wrong code in the text above.

Best regards,
Daniel Starke

Re: [PATCH 16/20] tty: n_gsm: fix invalid command/response bit check for UI/UIH frames

[Greg KH]

On Tue, Apr 19, 2022 at 07:10:43AM +0000, Starke, Daniel wrote:
> > 42:21AM -0700, 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. Chapter 5.4.3.1 states the CR bit in 
> > > UI and UIH frames shall always be set 1 by the initiator and 0 by the responder.
> > 
> > This has nothing to do with the change you made here.
> > 
> > 
> > > Currently, gsm_queue() has a pre-processor gated (excluded) check 
> > > which treats all frames that conform to the standard as malformed frames.
> > > Remove this optional code to avoid confusion and possible breaking 
> > > changes in case that someone includes it.
> > 
> > Again, nothing to do with the code change.
> 
> Including this code (i.e. with #if 1) will treat every correct UI/UIH frame
> as invalid, because the cr flag is always set to 1 for those frames
> (as mentioned in chapter 5.4.3.1 of the standard). This is obviously wrong.
> 
> > > 
> > > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> > 
> > This "fixes" nothing :(
> 
> What is the correct way to handle the removal of such dead and obviously
> wrong code?
> 
> > > Cc: stable@vger.kernel.org
> > 
> > How is commenting out unused code a stable backport requirement?
> 
> True, it does not change the behavior but it fixes a commit which is also
> present in the current stable release. I was unsure how to handle this
> case. I will remove the backport remark.
> 
> > > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > > ---
> > >  drivers/tty/n_gsm.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > > e9a7d9483c1f..f4ec48c0d6d7 100644
> > > --- a/drivers/tty/n_gsm.c
> > > +++ b/drivers/tty/n_gsm.c
> > > @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
> > >  	case UI|PF:
> > >  	case UIH:
> > >  	case UIH|PF:
> > > -#if 0
> > > -		if (cr)
> > > -			goto invalid;
> > > -#endif
> > 
> > All you are doing is cleaning up dead code.  Not a big deal, and not
> > worth all the text above to confuse people :(
> 
> As mentioned above, this is not only dead but also wrong code. I tried to
> elaborate the reason for it being wrong code in the text above.

That's fine, then just submit a patch that says:
	Remove commented out code as it is never used and if anyone
	accidentally turned it on, it would be broken.

We remove dead code like this all the time, it's not a "fix" as nothing
is broken as-is.

thanks,

greg k-h

[PATCH 01/20] tty: n_gsm: fix missing mux reset on config change at responder

[D. Starke]

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

Currently, only the initiator resets the mux protocol if the user requests
new parameters that are incompatible to those of the current connection.
The responder also needs to reset the multiplexer if the new parameter set
requires this. Otherwise, we end up with an inconsistent parameter set
between initiator and responder.
Revert the old behavior to inform the peer upon an incompatible parameter
set change from the user on the responder side by re-establishing the mux
protocol in such case.

Fixes: 509067bbd264 ("tty: n_gsm: Delete gsm_disconnect when config requester")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index fa92f727fdf8..3d28ecebd473 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2373,7 +2373,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	 * configuration
 	 */
 
-	if (gsm->initiator && (need_close || need_restart)) {
+	if (need_close || need_restart) {
 		int ret;
 
 		ret = gsm_disconnect(gsm);
-- 
2.25.1

[PATCH 16/20] tty: n_gsm: fix invalid command/response bit check for UI/UIH frames

[D. Starke]

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. Chapter 5.4.3.1 states the CR bit in UI and UIH
frames shall always be set 1 by the initiator and 0 by the responder.
Currently, gsm_queue() has a pre-processor gated (excluded) check which
treats all frames that conform to the standard as malformed frames.
Remove this optional code to avoid confusion and possible breaking changes
in case that someone includes it.

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 | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e9a7d9483c1f..f4ec48c0d6d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
 	case UI|PF:
 	case UIH:
 	case UIH|PF:
-#if 0
-		if (cr)
-			goto invalid;
-#endif
 		if (dlci == NULL || dlci->state != DLCI_OPEN) {
 			gsm_command(gsm, address, DM|PF);
 			return;
-- 
2.25.1

Re: [PATCH 16/20] tty: n_gsm: fix invalid command/response bit check for UI/UIH frames

[Greg KH]

On Thu, Apr 14, 2022 at 02:42:21AM -0700, 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. Chapter 5.4.3.1 states the CR bit in UI and UIH
> frames shall always be set 1 by the initiator and 0 by the responder.

This has nothing to do with the change you made here.


> Currently, gsm_queue() has a pre-processor gated (excluded) check which
> treats all frames that conform to the standard as malformed frames.
> Remove this optional code to avoid confusion and possible breaking changes
> in case that someone includes it.

Again, nothing to do with the code change.

> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")

This "fixes" nothing :(

> Cc: stable@vger.kernel.org

How is commenting out unused code a stable backport requirement?

> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index e9a7d9483c1f..f4ec48c0d6d7 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1896,10 +1896,6 @@ static void gsm_queue(struct gsm_mux *gsm)
>  	case UI|PF:
>  	case UIH:
>  	case UIH|PF:
> -#if 0
> -		if (cr)
> -			goto invalid;
> -#endif

All you are doing is cleaning up dead code.  Not a big deal, and not
worth all the text above to confuse people :(

thanks,

greg k-h