[PATCH] Prevent deadlock in n_gsm driver

[PATCH] Prevent deadlock in n_gsm driver

[Dirkjan Bussink]

This change fixes a deadlock when the multiplexer is closed while there
are still client side ports open.

When the multiplexer is closed and there are active tty's it tries to
close them with tty_vhangup. This has a problem though, because
tty_vhangup needs the tty_lock. This patch changes it to unlock the
tty_lock before attempting the hangup and relocks afterwards. The
additional call to tty_port_tty_set is needed because otherwise the
port stays active because of the reference counter.

This change also exposed another problem that other code paths don't
expect that the multiplexer could have been closed. This patch also adds
checks for these cases in the gsmtty_ class of function that could be
called.

The documentation explicitly states that "first close all virtual ports
before closing the physical port" but we've found this to not always
reality in our field situations. The GPRS / UTMS modem sometimes crashes
and needs a power cycle in that case which means cleanly shutting down
everything is not always possible. This change makes it much more robust
for our situation where at least the system is recoverable with this patch
and doesn't hang in a deadlock situation inside the kernel.

The patch is against the long term support kernel (3.4.27) and should
apply cleanly to more recent branches. Tested with a Telit GE864-QUADV2
and Telit HE910 modem.

Signed-off-by: Dirkjan Bussink <dirkjan.bussink@nedap.com>
---
 drivers/tty/n_gsm.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 90dff82..d9bde03 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1692,6 +1692,8 @@ static inline void dlci_put(struct gsm_dlci *dlci)
 	kref_put(&dlci->ref, gsm_dlci_free);
 }
 
+static void gsm_destroy_network(struct gsm_dlci *dlci);
+
 /**
  *	gsm_dlci_release		-	release DLCI
  *	@dlci: DLCI to destroy
@@ -1705,9 +1707,19 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
 {
 	struct tty_struct *tty = tty_port_tty_get(&dlci->port);
 	if (tty) {
+		mutex_lock(&dlci->mutex);
+		gsm_destroy_network(dlci);
+		mutex_unlock(&dlci->mutex);
+
+		/* tty_vhangup needs the tty_lock, so unlock and
+		   relock after doing the hangup. */
+		tty_unlock();
 		tty_vhangup(tty);
+		tty_lock();
+		tty_port_tty_set(&dlci->port, NULL);
 		tty_kref_put(tty);
 	}
+	dlci->state = DLCI_CLOSED;
 	dlci_put(dlci);
 }
 
@@ -2933,6 +2945,8 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 
 	if (dlci == NULL)
 		return;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	mutex_lock(&dlci->mutex);
 	gsm_destroy_network(dlci);
 	mutex_unlock(&dlci->mutex);
@@ -2951,6 +2965,8 @@ out:
 static void gsmtty_hangup(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	tty_port_hangup(&dlci->port);
 	gsm_dlci_begin_close(dlci);
 }
@@ -2958,9 +2974,12 @@ static void gsmtty_hangup(struct tty_struct *tty)
 static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 								    int len)
 {
+	int sent;
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	/* Stuff the bytes into the fifo queue */
-	int sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
+	sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
 	/* Need to kick the channel */
 	gsm_dlci_data_kick(dlci);
 	return sent;
@@ -2969,18 +2988,24 @@ static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 static int gsmtty_write_room(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	return TX_SIZE - kfifo_len(dlci->fifo);
 }
 
 static int gsmtty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	return kfifo_len(dlci->fifo);
 }
 
 static void gsmtty_flush_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	/* Caution needed: If we implement reliable transport classes
 	   then the data being transmitted can't simply be junked once
 	   it has first hit the stack. Until then we can just blow it
@@ -2999,6 +3024,8 @@ static void gsmtty_wait_until_sent(struct tty_struct *tty, int timeout)
 static int gsmtty_tiocmget(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	return dlci->modem_rx;
 }
 
@@ -3008,6 +3035,8 @@ static int gsmtty_tiocmset(struct tty_struct *tty,
 	struct gsm_dlci *dlci = tty->driver_data;
 	unsigned int modem_tx = dlci->modem_tx;
 
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	modem_tx &= ~clear;
 	modem_tx |= set;
 
@@ -3026,6 +3055,8 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 	struct gsm_netconfig nc;
 	int index;
 
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 	switch (cmd) {
 	case GSMIOC_ENABLE_NET:
 		if (copy_from_user(&nc, (void __user *)arg, sizeof(nc)))
@@ -3052,6 +3083,9 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 
 static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
+	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	/* For the moment its fixed. In actual fact the speed information
 	   for the virtual channel can be propogated in both directions by
 	   the RPN control message. This however rapidly gets nasty as we
@@ -3063,6 +3097,8 @@ static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 static void gsmtty_throttle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	if (tty->termios->c_cflag & CRTSCTS)
 		dlci->modem_tx &= ~TIOCM_DTR;
 	dlci->throttled = 1;
@@ -3073,6 +3109,8 @@ static void gsmtty_throttle(struct tty_struct *tty)
 static void gsmtty_unthrottle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state != DLCI_OPEN)
+		return;
 	if (tty->termios->c_cflag & CRTSCTS)
 		dlci->modem_tx |= TIOCM_DTR;
 	dlci->throttled = 0;
@@ -3084,6 +3122,8 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	int encode = 0;	/* Off */
+	if (dlci->state != DLCI_OPEN)
+		return -EINVAL;
 
 	if (state == -1)	/* "On indefinitely" - we can't encode this
 				    properly */
-- 
1.7.2.5

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Greg Kroah-Hartman]

On Tue, Jan 29, 2013 at 09:49:52AM +0100, Dirkjan Bussink wrote:
> This change fixes a deadlock when the multiplexer is closed while there
> are still client side ports open.
> 
> When the multiplexer is closed and there are active tty's it tries to
> close them with tty_vhangup. This has a problem though, because
> tty_vhangup needs the tty_lock. This patch changes it to unlock the
> tty_lock before attempting the hangup and relocks afterwards. The
> additional call to tty_port_tty_set is needed because otherwise the
> port stays active because of the reference counter.
> 
> This change also exposed another problem that other code paths don't
> expect that the multiplexer could have been closed. This patch also adds
> checks for these cases in the gsmtty_ class of function that could be
> called.
> 
> The documentation explicitly states that "first close all virtual ports
> before closing the physical port" but we've found this to not always
> reality in our field situations. The GPRS / UTMS modem sometimes crashes
> and needs a power cycle in that case which means cleanly shutting down
> everything is not always possible. This change makes it much more robust
> for our situation where at least the system is recoverable with this patch
> and doesn't hang in a deadlock situation inside the kernel.
> 
> The patch is against the long term support kernel (3.4.27) and should
> apply cleanly to more recent branches. Tested with a Telit GE864-QUADV2
> and Telit HE910 modem.

It doesn't apply cleanly to the linux-next tree, can you refresh this
and resend it so that I can apply it?  If you want it backported to the
stable kernel releases, we can only do that after it gets into Linus's
tree, see the file, Documentation/stable_kernel_rules.txt for details
about this.

thanks,

greg k-h

[PATCH] tty: Prevent deadlock in n_gsm driver

[Dirkjan Bussink]

This change fixes a deadlock when the multiplexer is closed while there
are still client side ports open.

When the multiplexer is closed and there are active tty's it tries to
close them with tty_vhangup. This has a problem though, because
tty_vhangup needs the tty_lock. This patch changes it to unlock the
tty_lock before attempting the hangup and relocks afterwards. The
additional call to tty_port_tty_set is needed because otherwise the
port stays active because of the reference counter.

This change also exposed another problem that other code paths don't
expect that the multiplexer could have been closed. This patch also adds
checks for these cases in the gsmtty_ class of function that could be
called.

The documentation explicitly states that "first close all virtual ports
before closing the physical port" but we've found this to not always
reality in our field situations. The GPRS / UTMS modem sometimes crashes
and needs a power cycle in that case which means cleanly shutting down
everything is not always possible. This change makes it much more robust
for our situation where at least the system is recoverable with this patch
and doesn't hang in a deadlock situation inside the kernel.

The patch is against the long term support kernel (3.4.27) and should
apply cleanly to more recent branches. Tested with a Telit GE864-QUADV2
and Telit HE910 modem.

Signed-off-by: Dirkjan Bussink <dirkjan.bussink@nedap.com>
---
 drivers/tty/n_gsm.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e0f80ce..e2b57fa 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1689,6 +1689,8 @@ static inline void dlci_put(struct gsm_dlci *dlci)
 	tty_port_put(&dlci->port);
 }

+static void gsm_destroy_network(struct gsm_dlci *dlci);
+
 /**
  *	gsm_dlci_release		-	release DLCI
  *	@dlci: DLCI to destroy
@@ -1702,9 +1704,19 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
 {
 	struct tty_struct *tty = tty_port_tty_get(&dlci->port);
 	if (tty) {
+		mutex_lock(&dlci->mutex);
+		gsm_destroy_network(dlci);
+		mutex_unlock(&dlci->mutex);
+
+		/* tty_vhangup needs the tty_lock, so unlock and
+		   relock after doing the hangup. */
+		tty_unlock();
 		tty_vhangup(tty);
+		tty_lock();
+		tty_port_tty_set(&dlci->port, NULL);
 		tty_kref_put(tty);
 	}
+	dlci->state = DLCI_CLOSED;
 	dlci_put(dlci);
 }

@@ -2947,6 +2959,8 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)

 	if (dlci == NULL)
 		return;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	mutex_lock(&dlci->mutex);
 	gsm_destroy_network(dlci);
 	mutex_unlock(&dlci->mutex);
@@ -2965,6 +2979,8 @@ out:
 static void gsmtty_hangup(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	tty_port_hangup(&dlci->port);
 	gsm_dlci_begin_close(dlci);
 }
@@ -2972,9 +2988,12 @@ static void gsmtty_hangup(struct tty_struct *tty)
 static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 								    int len)
 {
+	int sent;
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	/* Stuff the bytes into the fifo queue */
-	int sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
+	sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
 	/* Need to kick the channel */
 	gsm_dlci_data_kick(dlci);
 	return sent;
@@ -2983,18 +3002,24 @@ static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 static int gsmtty_write_room(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return TX_SIZE - kfifo_len(dlci->fifo);
 }

 static int gsmtty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return kfifo_len(dlci->fifo);
 }

 static void gsmtty_flush_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	/* Caution needed: If we implement reliable transport classes
 	   then the data being transmitted can't simply be junked once
 	   it has first hit the stack. Until then we can just blow it
@@ -3013,6 +3038,8 @@ static void gsmtty_wait_until_sent(struct tty_struct *tty, int timeout)
 static int gsmtty_tiocmget(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return dlci->modem_rx;
 }

@@ -3022,6 +3049,8 @@ static int gsmtty_tiocmset(struct tty_struct *tty,
 	struct gsm_dlci *dlci = tty->driver_data;
 	unsigned int modem_tx = dlci->modem_tx;

+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	modem_tx &= ~clear;
 	modem_tx |= set;

@@ -3040,6 +3069,8 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 	struct gsm_netconfig nc;
 	int index;

+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	switch (cmd) {
 	case GSMIOC_ENABLE_NET:
 		if (copy_from_user(&nc, (void __user *)arg, sizeof(nc)))
@@ -3066,6 +3097,9 @@ static int gsmtty_ioctl(struct tty_struct *tty,

 static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
+	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	/* For the moment its fixed. In actual fact the speed information
 	   for the virtual channel can be propogated in both directions by
 	   the RPN control message. This however rapidly gets nasty as we
@@ -3077,6 +3111,8 @@ static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 static void gsmtty_throttle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	if (tty->termios.c_cflag & CRTSCTS)
 		dlci->modem_tx &= ~TIOCM_DTR;
 	dlci->throttled = 1;
@@ -3087,6 +3123,8 @@ static void gsmtty_throttle(struct tty_struct *tty)
 static void gsmtty_unthrottle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	if (tty->termios.c_cflag & CRTSCTS)
 		dlci->modem_tx |= TIOCM_DTR;
 	dlci->throttled = 0;
@@ -3098,6 +3136,8 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	int encode = 0;	/* Off */
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;

 	if (state == -1)	/* "On indefinitely" - we can't encode this
 				    properly */
--
1.7.2.5

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Greg Kroah-Hartman]

On Wed, Jan 30, 2013 at 10:51:59AM +0100, Dirkjan Bussink wrote:
> This change fixes a deadlock when the multiplexer is closed while there
> are still client side ports open.
> 
> When the multiplexer is closed and there are active tty's it tries to
> close them with tty_vhangup. This has a problem though, because
> tty_vhangup needs the tty_lock. This patch changes it to unlock the
> tty_lock before attempting the hangup and relocks afterwards. The
> additional call to tty_port_tty_set is needed because otherwise the
> port stays active because of the reference counter.
> 
> This change also exposed another problem that other code paths don't
> expect that the multiplexer could have been closed. This patch also adds
> checks for these cases in the gsmtty_ class of function that could be
> called.
> 
> The documentation explicitly states that "first close all virtual ports
> before closing the physical port" but we've found this to not always
> reality in our field situations. The GPRS / UTMS modem sometimes crashes
> and needs a power cycle in that case which means cleanly shutting down
> everything is not always possible. This change makes it much more robust
> for our situation where at least the system is recoverable with this patch
> and doesn't hang in a deadlock situation inside the kernel.
> 
> The patch is against the long term support kernel (3.4.27) and should
> apply cleanly to more recent branches. Tested with a Telit GE864-QUADV2
> and Telit HE910 modem.
> 
> Signed-off-by: Dirkjan Bussink <dirkjan.bussink@nedap.com>
> Cc: stable <stable@vger.kernel.org>

This patch breaks the build in the tty-next tree:

drivers/tty/n_gsm.c: In function ‘gsm_dlci_release’:
drivers/tty/n_gsm.c:1713:3: error: too few arguments to function ‘tty_unlock’
In file included from drivers/tty/n_gsm.c:44:0:
include/linux/tty.h:596:24: note: declared here
drivers/tty/n_gsm.c:1715:3: error: too few arguments to function ‘tty_lock’
In file included from drivers/tty/n_gsm.c:44:0:
include/linux/tty.h:595:24: note: declared here

Can you fix this up (get it to build on the linux-next tree) and resend
it?

thanks,

greg k-h

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Dirkjan Bussink]

On 30 Jan 2013, at 11:10, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> This patch breaks the build in the tty-next tree:
> 
> drivers/tty/n_gsm.c: In function ‘gsm_dlci_release’:
> drivers/tty/n_gsm.c:1713:3: error: too few arguments to function ‘tty_unlock’
> In file included from drivers/tty/n_gsm.c:44:0:
> include/linux/tty.h:596:24: note: declared here
> drivers/tty/n_gsm.c:1715:3: error: too few arguments to function ‘tty_lock’
> In file included from drivers/tty/n_gsm.c:44:0:
> include/linux/tty.h:595:24: note: declared here
> 
> Can you fix this up (get it to build on the linux-next tree) and resend
> it?

I've resent it with the proper changes. I also tried to ensure that the patch
wasn't mangled this time, so that should be good as well now. Please let me
know if this is still problematic, I've used mutt and -H to send the patch
created with git format-patch. 

-- 
Regards,

Dirkjan Bussink

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Dirkjan Bussink]

On 30 Jan 2013, at 06:12, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> It doesn't apply cleanly to the linux-next tree, can you refresh this
> and resend it so that I can apply it?  If you want it backported to the
> stable kernel releases, we can only do that after it gets into Linus's
> tree, see the file, Documentation/stable_kernel_rules.txt for details
> about this.

I've reapplied the patch to linux-next and submitted it just now. Is there
anything additionally that needs to be done to merge this back to stable
after this patch is applied? 

I also changed the additional check conditions by changing them into

if (dlci->state == DLCI_CLOSED)

During testing I've found that there can be a case where gsmtty_ioctl was called
while it was still in the state DLCI_OPENING and this is not a problematic case.
Therefore I changed the check from the original patch to the one shown in the
new one.

-- 
Regards,

Dirkjan Bussink

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Greg Kroah-Hartman]

On Wed, Jan 30, 2013 at 10:55:00AM +0100, Dirkjan Bussink wrote:
> 
> On 30 Jan 2013, at 06:12, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > It doesn't apply cleanly to the linux-next tree, can you refresh this
> > and resend it so that I can apply it?  If you want it backported to the
> > stable kernel releases, we can only do that after it gets into Linus's
> > tree, see the file, Documentation/stable_kernel_rules.txt for details
> > about this.
> 
> I've reapplied the patch to linux-next and submitted it just now. Is there
> anything additionally that needs to be done to merge this back to stable
> after this patch is applied? 

You need to add a "cc: stable@vger.kernel.org" line to the
signed-off-by: area.  I'll do it to the patch when I apply it.

I found the problem, your email client stripped out the trailing spaces
of the patch, which kept it from being applied as it wasn't a valid
patch.  I've edited it by hand this time, but next time please fix your
email client.

thanks,

greg k-h

[PATCH] tty: Prevent deadlock in n_gsm driver

[Dirkjan Bussink]

This change fixes a deadlock when the multiplexer is closed while there
are still client side ports open.

When the multiplexer is closed and there are active tty's it tries to
close them with tty_vhangup. This has a problem though, because
tty_vhangup needs the tty_lock. This patch changes it to unlock the
tty_lock before attempting the hangup and relocks afterwards. The
additional call to tty_port_tty_set is needed because otherwise the
port stays active because of the reference counter.

This change also exposed another problem that other code paths don't
expect that the multiplexer could have been closed. This patch also adds
checks for these cases in the gsmtty_ class of function that could be
called.

The documentation explicitly states that "first close all virtual ports
before closing the physical port" but we've found this to not always
reality in our field situations. The GPRS / UTMS modem sometimes crashes
and needs a power cycle in that case which means cleanly shutting down
everything is not always possible. This change makes it much more robust
for our situation where at least the system is recoverable with this patch
and doesn't hang in a deadlock situation inside the kernel.

The patch is against the long term support kernel (3.4.27) and should
apply cleanly to more recent branches. Tested with a Telit GE864-QUADV2
and Telit HE910 modem.

Signed-off-by: Dirkjan Bussink <dirkjan.bussink@nedap.com>

Cc: stable <stable@vger.kernel.org>
---
 drivers/tty/n_gsm.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e0f80ce..4a43ef5d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1689,6 +1689,8 @@ static inline void dlci_put(struct gsm_dlci *dlci)
 	tty_port_put(&dlci->port);
 }
 
+static void gsm_destroy_network(struct gsm_dlci *dlci);
+
 /**
  *	gsm_dlci_release		-	release DLCI
  *	@dlci: DLCI to destroy
@@ -1702,9 +1704,19 @@ static void gsm_dlci_release(struct gsm_dlci *dlci)
 {
 	struct tty_struct *tty = tty_port_tty_get(&dlci->port);
 	if (tty) {
+		mutex_lock(&dlci->mutex);
+		gsm_destroy_network(dlci);
+		mutex_unlock(&dlci->mutex);
+
+		/* tty_vhangup needs the tty_lock, so unlock and
+		   relock after doing the hangup. */
+		tty_unlock(tty);
 		tty_vhangup(tty);
+		tty_lock(tty);
+		tty_port_tty_set(&dlci->port, NULL);
 		tty_kref_put(tty);
 	}
+	dlci->state = DLCI_CLOSED;
 	dlci_put(dlci);
 }
 
@@ -2947,6 +2959,8 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 
 	if (dlci == NULL)
 		return;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	mutex_lock(&dlci->mutex);
 	gsm_destroy_network(dlci);
 	mutex_unlock(&dlci->mutex);
@@ -2965,6 +2979,8 @@ out:
 static void gsmtty_hangup(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	tty_port_hangup(&dlci->port);
 	gsm_dlci_begin_close(dlci);
 }
@@ -2972,9 +2988,12 @@ static void gsmtty_hangup(struct tty_struct *tty)
 static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 								    int len)
 {
+	int sent;
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	/* Stuff the bytes into the fifo queue */
-	int sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
+	sent = kfifo_in_locked(dlci->fifo, buf, len, &dlci->lock);
 	/* Need to kick the channel */
 	gsm_dlci_data_kick(dlci);
 	return sent;
@@ -2983,18 +3002,24 @@ static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 static int gsmtty_write_room(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return TX_SIZE - kfifo_len(dlci->fifo);
 }
 
 static int gsmtty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return kfifo_len(dlci->fifo);
 }
 
 static void gsmtty_flush_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	/* Caution needed: If we implement reliable transport classes
 	   then the data being transmitted can't simply be junked once
 	   it has first hit the stack. Until then we can just blow it
@@ -3013,6 +3038,8 @@ static void gsmtty_wait_until_sent(struct tty_struct *tty, int timeout)
 static int gsmtty_tiocmget(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	return dlci->modem_rx;
 }
 
@@ -3022,6 +3049,8 @@ static int gsmtty_tiocmset(struct tty_struct *tty,
 	struct gsm_dlci *dlci = tty->driver_data;
 	unsigned int modem_tx = dlci->modem_tx;
 
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	modem_tx &= ~clear;
 	modem_tx |= set;
 
@@ -3040,6 +3069,8 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 	struct gsm_netconfig nc;
 	int index;
 
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 	switch (cmd) {
 	case GSMIOC_ENABLE_NET:
 		if (copy_from_user(&nc, (void __user *)arg, sizeof(nc)))
@@ -3066,6 +3097,9 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 
 static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
+	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	/* For the moment its fixed. In actual fact the speed information
 	   for the virtual channel can be propogated in both directions by
 	   the RPN control message. This however rapidly gets nasty as we
@@ -3077,6 +3111,8 @@ static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 static void gsmtty_throttle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	if (tty->termios.c_cflag & CRTSCTS)
 		dlci->modem_tx &= ~TIOCM_DTR;
 	dlci->throttled = 1;
@@ -3087,6 +3123,8 @@ static void gsmtty_throttle(struct tty_struct *tty)
 static void gsmtty_unthrottle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+	if (dlci->state == DLCI_CLOSED)
+		return;
 	if (tty->termios.c_cflag & CRTSCTS)
 		dlci->modem_tx |= TIOCM_DTR;
 	dlci->throttled = 0;
@@ -3098,6 +3136,8 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	int encode = 0;	/* Off */
+	if (dlci->state == DLCI_CLOSED)
+		return -EINVAL;
 
 	if (state == -1)	/* "On indefinitely" - we can't encode this
 				    properly */
-- 
1.7.2.5

Re: [PATCH] tty: Prevent deadlock in n_gsm driver

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

I'm looking at commit 4d9b109060f690f5c835130ff54165ae157b3087 ('tty:
Prevent deadlock in n_gsm driver') which was cc'd to stable.  This drops
the tty lock in gsm_dlci_release().

I could apply it to 3.2.y with some adjustment, but I checked the
locking context of this function and I think there's another bug to fix.
Tracing back the callers in 3.2.y I found:

gsm_dlci_release
<- gsm_cleanup_mux
   <- gsmld_detach_gsm
      <- gsmld_close
         <- tty_ldisc_close
            <- tty_set_ldisc [with tty_lock]
            <- tty_ldisc_reinit
               <- tty_ldisc_hangup [with tty_lock]
   <- gsmld_config
      <- gsmld_ioctl
         <- tty_ioctl [without tty lock]

and I think the call chains are much the same in mainline.  So
gsmld_config() presumably needs to acquire the tty lock.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]