tty: n_gsm: Avoid sleeping during .write() whilst atomic

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

__might_sleep()
__might_resched() # Complains that IRQs are disabled

To fix this, let's ensure mutual exclusion by using a protected shared variable (busy) instead. We'll use the current locking mechanism to protect it, but ensure that the locks are released and IRQs re-enabled by the time we transit further down the call chain which may sleep.

Cc Daniel Starke <[email protected]>

Cc Fedor Pchelkin <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

Reported-by [email protected]

Signed-off-by Lee Jones <[email protected]>

---
 drivers/tty/n_gsm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/tty/n_gsm.c +2 -0

index 1f3aba607cd51..b83a97d58381f 100644
@@ -270,6 +270,7 @@ struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
+	bool busy;
 	unsigned int num;
 	struct kref ref;
 

@@ -3253,6 +3254,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	gsm->dead = true;	/* Avoid early tty opens */
 	gsm->wait_config = false; /* Disabled */
 	gsm->keep_alive = 0;	/* Disabled */
+	gsm->busy = false;
 
 	/* Store the instance to the mux array or abort if no space is
 	 * available.

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

Quoted 41 lines

__might_sleep()
__might_resched() # Complains that IRQs are disabled

To fix this, let's ensure mutual exclusion by using a protected shared variable (busy) instead. We'll use the current locking mechanism to protect it, but ensure that the locks are released and IRQs re-enabled by the time we transit further down the call chain which may sleep.

Cc Daniel Starke <[email protected]>

Cc Fedor Pchelkin <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

Reported-by [email protected]

Signed-off-by Lee Jones <[email protected]>

---
 drivers/tty/n_gsm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/tty/n_gsm.c +2 -0

index 1f3aba607cd51..b83a97d58381f 100644
@@ -270,6 +270,7 @@ struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
+	bool busy;
 	unsigned int num;
 	struct kref ref;
 

@@ -3253,6 +3254,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	gsm->dead = true;	/* Avoid early tty opens */
 	gsm->wait_config = false; /* Disabled */
 	gsm->keep_alive = 0;	/* Disabled */
+	gsm->busy = false;
 
 	/* Store the instance to the mux array or abort if no space is
 	 * available.

@@ -3718,11 +3720,21 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
 

drivers/tty/n_gsm.c static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, · line 3725

Quoted 5 lines

drivers/tty/n_gsm.c +3 -0

 	ret = -ENOBUFS;
 	spin_lock_irqsave(&gsm->tx_lock, flags);
+	if (gsm->busy) {
+		spin_unlock_irqrestore(&gsm->tx_lock, flags);
+		return -EBUSY;

So you just "busted" the re-entrant call chain here, are you sure this is ok for this protocl? Can it handle -EBUSY?

Daniel, any thoughts?

And Lee, you really don't have this hardware, right? So why are you dealing with bug reports for it? :)

Quoted 13 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

Quoted 3 lines

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

Quoted 2 lines

And Lee, you really don't have this hardware, right? So why are you dealing with bug reports for it? :)

'cos Syzkaller.

Quoted 19 lines · Lee Jones

Quoted 16 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

Quoted 3 lines

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

I would recommend that any distro/system that does not want to support this specific hardware protocol, just disable it for now (it's marked as experimental too), if they don't want to deal with the potential sleep-while-atomic issue.

Quoted 4 lines

Quoted 2 lines

And Lee, you really don't have this hardware, right? So why are you dealing with bug reports for it? :)

'cos Syzkaller.

Ah, yeah, fake report, no real issue here then :)

Quoted 24 lines · Greg Kroah-Hartman

Quoted 22 lines · Lee Jones

Quoted 9 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

Quoted 7 lines

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

Quoted 4 lines

I would recommend that any distro/system that does not want to support this specific hardware protocol, just disable it for now (it's marked as experimental too), if they don't want to deal with the potential sleep-while-atomic issue.

n_gsm is available on all the systems I have available.

The mention of
'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

And Lee, you really don't have this hardware, right? So why are you dealing with bug reports for it? :)

'cos Syzkaller.

Ah, yeah, fake report, no real issue here then :)

Ouch! The way I see it, the present issue with Syzkaller is that we do not have the resources to remedy all of the issues it flags. Passing off all reports as 'not real issues' is going to make engineers who decide to work on them feel undervalued and is likely have a detrimental effect overall. As an ambassador for young and new people trying to get into Kernel Engineering in general, is that really the outcome you're after?

Quoted 20 lines · Greg Kroah-Hartman

Quoted 19 lines · Lee Jones

Quoted 16 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

Quoted 8 lines

Quoted 3 lines

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

I would recommend that any distro/system that does not want to support this specific hardware protocol, just disable it for now (it's marked as experimental too), if they don't want to deal with the potential sleep-while-atomic issue.

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Quoted 31 lines · Lee Jones

Quoted 20 lines · Greg Kroah-Hartman

Quoted 19 lines · Lee Jones

Quoted 16 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

Quoted 8 lines

Quoted 3 lines

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

I would recommend that any distro/system that does not want to support this specific hardware protocol, just disable it for now (it's marked as experimental too), if they don't want to deal with the potential sleep-while-atomic issue.

Quoted 5 lines

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Also, I think this requires root to set this line discipline to the console, right? A normal user can't do that, or am I missing a code path here?

Is there a reproducer somewhere for this issue that runs as a normal user? I couldn't find one in the syzbot listings but I might have been not looking deep enough.

Quoted 38 lines · Greg Kroah-Hartman

Quoted 36 lines · Lee Jones

Quoted 20 lines · Greg Kroah-Hartman

Quoted 19 lines · Lee Jones

Quoted 16 lines · Greg Kroah-Hartman

Quoted 5 lines · Lee Jones

The important part of the call stack being:

gsmld_write() # Takes a lock and disables IRQs

    con_write()
      console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:

	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html

this is a specific modem protocol, why is con_write() being called?

Quoted 8 lines

Quoted 3 lines

What it's meant for and what random users can make it do are likely to be quite separate questions. This scenario is user driven and can be replicated simply by issuing a few syscalls (open, ioctl, write).

I would recommend that any distro/system that does not want to support this specific hardware protocol, just disable it for now (it's marked as experimental too), if they don't want to deal with the potential sleep-while-atomic issue.

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Quoted 3 lines

Also, I think this requires root to set this line discipline to the console, right? A normal user can't do that, or am I missing a code path here?

I haven't been testing long, but yes, early indications show that root is required.

Quoted 6 lines

Quoted 5 lines

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Quoted 3 lines

Also, I think this requires root to set this line discipline to the console, right? A normal user can't do that, or am I missing a code path here?

I haven't been testing long, but yes, early indications show that root is required.

Quoted 10 lines · Lee Jones

Quoted 9 lines

Quoted 5 lines

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Also, I think this requires root to set this line discipline to the console, right? A normal user can't do that, or am I missing a code path here?

Quoted 2 lines

I haven't been testing long, but yes, early indications show that root is required.

FWIW I concluded to the same yesterday, so I disputed the connected CVE-2023-31082. Waiting for mitre's ack/nack.

Quoted 14 lines · Jiri Slaby

Quoted 12 lines · Lee Jones

Quoted 9 lines

Quoted 5 lines

n_gsm is available on all the systems I have available. The mention of 'EXPERIMENTAL' in the module description appears to have zero effect on whether distros choose to make it available or not. If you're saying that we know this module is BROKEN however, then perhaps we should mark it as such.

Also, I think this requires root to set this line discipline to the console, right? A normal user can't do that, or am I missing a code path here?

I haven't been testing long, but yes, early indications show that root is required.

Quoted 2 lines

FWIW I concluded to the same yesterday, so I disputed the connected CVE-2023-31082. Waiting for mitre's ack/nack.

Trying to dispute obviously-wrong CVEs is a never-ending task.

Personally, it's fun to see who keeps popping up to attempt to resolve this issue showing what companies have their security policies dictated by a random government authority that can be modified by anyone :)

Daniel, any thoughts?

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.
There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Quoted 16 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.
There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Quoted 3 lines

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

So without actually looking into the code, can we just somehow disallow attaching this line discipline to a console (ban such a TIOCSETD) and be done with this issue? IOW disallow root to shoot their foot.

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

Quoted 4 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Quoted 3 lines

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 8 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Quoted 4 lines

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

What does the "better solution" look like?

Quoted 36 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 12 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 1 line

What does the "better solution" look like?

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Quoted 37 lines · Greg Kroah-Hartman

Quoted 36 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 12 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 5 lines

Quoted 1 line

What does the "better solution" look like?

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Does this solution break the recursion loop or affect performance?

Quoted 41 lines · Lee Jones

Quoted 39 lines · Greg Kroah-Hartman

Quoted 37 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 4 lines

Quoted 4 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Quoted 8 lines

Quoted 3 lines

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

What does the "better solution" look like?

Quoted 5 lines

Quoted 3 lines

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Does this solution break the recursion loop or affect performance?

This solution broke the recursion by returning an error, right?

The performance one was by using mutexes as in previous attempts.

Quoted 48 lines · Greg Kroah-Hartman

Quoted 46 lines · Lee Jones

Quoted 37 lines · Greg Kroah-Hartman

Quoted 36 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 12 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 5 lines

Quoted 1 line

What does the "better solution" look like?

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Does this solution break the recursion loop or affect performance?

Quoted 1 line

This solution broke the recursion by returning an error, right?

This is the part I was least sure about.

If this was considered valid and we were to go forward with a solution like this, what would a quality improvement look like? Should we have stayed in this function and waited for the previous occupant to leave before continuing through ->write()?

Quoted 50 lines · Lee Jones

Quoted 48 lines · Greg Kroah-Hartman

Quoted 46 lines · Lee Jones

Quoted 37 lines · Greg Kroah-Hartman

Quoted 36 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 12 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 5 lines

Quoted 1 line

What does the "better solution" look like?

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Does this solution break the recursion loop or affect performance?

Quoted 8 lines

Quoted 1 line

This solution broke the recursion by returning an error, right?

This is the part I was least sure about.

If this was considered valid and we were to go forward with a solution like this, what would a quality improvement look like? Should we have stayed in this function and waited for the previous occupant to leave before continuing through ->write()?

This isn't valid, as it obviously never shows up in real use.

The real solution should be to prevent binding a console to this line discipline as it can not handle the recursion that consoles require for the write path.

Then, if consoles are really needed, the code can be fixed up to handle such recursion. That's not a trivial thing to do, as can be seen by the crazy gyrations that the n_tty line discipline does in its write path...

Quoted 60 lines · Greg Kroah-Hartman

Quoted 50 lines · Lee Jones

Quoted 48 lines · Greg Kroah-Hartman

Quoted 46 lines · Lee Jones

Quoted 37 lines · Greg Kroah-Hartman

Quoted 36 lines · Lee Jones

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Daniel

Quoted 1 line

Daniel, any thoughts?

Our application of this protocol is only with specific modems to enable circuit switched operation (handling calls, selecting/querying networks, etc.) while doing packet switched communication (i.e. IP traffic over PPP). The protocol was developed for such use cases.

Quoted 5 lines

Quoted 5 lines

Regarding the issue itself:
There was already an attempt to fix all this by switching from spinlocks to mutexes resulting in ~20% performance loss. However, the patch was reverted as it did not handle the T1 timer leading into sleep during atomic within gsm_dlci_t1() on every mutex lock there.

That's correct. When I initially saw this report, my initial thought was to replace the spinlocks with mutexts, but having read the previous accepted attempt and it's subsequent reversion I started to think of other ways to solve this issue. This solution, unlike the last, does not involve adding sleep inducing locks into atomic contexts, nor should it negatively affect performance.

Quoted 12 lines

Quoted 7 lines

There was also a suggestion to fix this in do_con_write() as tty_operations::write() appears to be documented as "not allowed to sleep". The patch for this was rejected. It did not fix the issue within n_gsm.

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Link https://lore.kernel.org/all/[email protected]/ open here

Ok, I thought I remembered this, I'll just drop this patch from my review queue and wait for a better solution if it ever comes up as this isn't a real issue that people are seeing on actual systems, but just a syzbot report.

Quoted 5 lines

Quoted 1 line

What does the "better solution" look like?

One that actually fixes the root problem here (i.e. does not break the recursion loop, or cause a performance decrease for normal users, or prevent this from being bound to the console).

Does this solution break the recursion loop or affect performance?

Quoted 8 lines

Quoted 1 line

This solution broke the recursion by returning an error, right?

This is the part I was least sure about.

If this was considered valid and we were to go forward with a solution like this, what would a quality improvement look like? Should we have stayed in this function and waited for the previous occupant to leave before continuing through ->write()?

Quoted 5 lines

This isn't valid, as it obviously never shows up in real use.

The real solution should be to prevent binding a console to this line discipline as it can not handle the recursion that consoles require for the write path.

Would something like this tick that box?

drivers/tty/n_gsm.c +0 -0

index 1f3aba607cd51..5c1d2fcd5d9e2 100644

Quoted 10 lines

Would something like this tick that box?

drivers/tty/n_gsm.c index renamed from drivers/tty/n_gsm.c +0 -0

1f3aba607cd51..5c1d2fcd5d9e2 100644

drivers/tty/n_gsm.c +0 -0

@@ -3716,6 +3716,10 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
        if (!gsm)
                return -ENODEV;
 

drivers/tty/n_gsm.c static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, · line 3721

Quoted 3 lines

drivers/tty/n_gsm.c +3 -0

+       /* The GSM line discipline does not support binding to console */
+       if (strncmp(tty->name, "tty", 3))
+               return -EINVAL;

No, that's not going to work, some consoles do not start with "tty" :(

Quoted 10 lines

Quoted 10 lines

Would something like this tick that box?

drivers/tty/n_gsm.c index renamed from drivers/tty/n_gsm.c +0 -0

1f3aba607cd51..5c1d2fcd5d9e2 100644

drivers/tty/n_gsm.c +0 -0

@@ -3716,6 +3716,10 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
        if (!gsm)
                return -ENODEV;
 

Quoted 5 lines

drivers/tty/n_gsm.c static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, · line 3721

Quoted 3 lines

drivers/tty/n_gsm.c +3 -0

+       /* The GSM line discipline does not support binding to console */
+       if (strncmp(tty->name, "tty", 3))
+               return -EINVAL;

No, that's not going to work, some consoles do not start with "tty" :(

Also, I would recommend exiting as early as possible. E.g. in gsmld_open().

And please retain support for real serial devices (e.g. ttyS, ttyUSB, ttyACM, ...).

Quoted 10 lines · Daniel

Quoted 10 lines

Quoted 10 lines

Would something like this tick that box?

drivers/tty/n_gsm.c index renamed from drivers/tty/n_gsm.c +0 -0

1f3aba607cd51..5c1d2fcd5d9e2 100644

drivers/tty/n_gsm.c +0 -0

@@ -3716,6 +3716,10 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
        if (!gsm)
                return -ENODEV;
 

Quoted 5 lines

Quoted 5 lines

drivers/tty/n_gsm.c static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, · line 3721

Quoted 3 lines

drivers/tty/n_gsm.c +3 -0

+       /* The GSM line discipline does not support binding to console */
+       if (strncmp(tty->name, "tty", 3))
+               return -EINVAL;

No, that's not going to work, some consoles do not start with "tty" :(

Ah, you mean there are others that we need to consider?

I was just covering off con_write() from drivers/tty/vt/vt.c.

Does anyone have a counter proposal?

Quoted 20 lines · Lee Jones

Quoted 10 lines · Daniel

Quoted 10 lines

Quoted 10 lines

Would something like this tick that box?

drivers/tty/n_gsm.c index renamed from drivers/tty/n_gsm.c +0 -0

1f3aba607cd51..5c1d2fcd5d9e2 100644

drivers/tty/n_gsm.c +0 -0

@@ -3716,6 +3716,10 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
        if (!gsm)
                return -ENODEV;
 

Quoted 5 lines

Quoted 5 lines

drivers/tty/n_gsm.c static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, · line 3721

Quoted 3 lines

drivers/tty/n_gsm.c +3 -0

+       /* The GSM line discipline does not support binding to console */
+       if (strncmp(tty->name, "tty", 3))
+               return -EINVAL;

No, that's not going to work, some consoles do not start with "tty" :(

Ah, you mean there are others that we need to consider?

Quoted 3 lines

I was just covering off con_write() from drivers/tty/vt/vt.c.

Does anyone have a counter proposal?

consoles are dynamically assigned, the device knows if it is a console or not, so there is a way to determine this at runtime. It's not a device naming thing at all.

Quoted 21 lines · Greg Kroah-Hartman

Quoted 20 lines · Lee Jones

Quoted 10 lines · Daniel

Quoted 10 lines

Quoted 10 lines

Would something like this tick that box?

drivers/tty/n_gsm.c index renamed from drivers/tty/n_gsm.c +0 -0

1f3aba607cd51..5c1d2fcd5d9e2 100644

drivers/tty/n_gsm.c +0 -0

@@ -3716,6 +3716,10 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
        if (!gsm)
                return -ENODEV;
 

Quoted 5 lines

Quoted 5 lines

drivers/tty/n_gsm.c static int gsmld_open(struct tty_struct *tty) · line 3634

Quoted 3 lines

drivers/tty/n_gsm.c +3 -0

+       /* The GSM line discipline does not support binding to console */
+       if (strncmp(tty->name, "tty", 3))
+               return -EINVAL;

No, that's not going to work, some consoles do not start with "tty" :(

Ah, you mean there are others that we need to consider?

Quoted 7 lines

Quoted 3 lines

I was just covering off con_write() from drivers/tty/vt/vt.c.

Does anyone have a counter proposal?

consoles are dynamically assigned, the device knows if it is a console or not, so there is a way to determine this at runtime. It's not a device naming thing at all.

It's not a device naming thing, but it is a ... :)

Okay, here's another uninformed stab in the dark:

drivers/tty/n_gsm.c +4 -0

index 1f3aba607cd51..ddf00f6a4141d 100644
@@ -3629,6 +3629,10 @@ static int gsmld_open(struct tty_struct *tty)
        if (tty->ops->write == NULL)
                return -EINVAL;
 
+       /* The GSM line discipline does not support binding to console */
+       if (tty->driver->type == TTY_DRIVER_TYPE_CONSOLE)
+               return -EINVAL;
+
        /* Attach our ldisc data */
        gsm = gsm_alloc_mux();
        if (gsm == NULL)

This seems to allow for the real serial devices (TTY_DRIVER_TYPE_SERIAL) suggested by Daniel.