[PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Gianluca Anzolin]

The following patches fix three regressions introduced with the
rfcomm tty_port conversion.

The first patch restores the expected behaviour of the rfcomm port when
it's created with the flag RFCOMM_RELEASE_ONHUP.

The second patch moves rfcomm_get_device() and is preparatory for the
third patch.

The third patch fixes two regressions:
1) when the tty is opened with O_NONBLOCK or CLOCAL is set (fixes
   wvdial)
2) when the rfcomm device is created with the flag RFCOMM_REUSE_DLC
   (fixes ModemManager)

The fourth patch removes rfcomm_dev_carrier_raised().

Changes from v2:
  * Fix the style of if statements in rfcomm_dev_activate().

Changes from v1:
  * Removed the device_move() fix which is now incorporated in a new patch.

Gianluca Anzolin (4):
  rfcomm: release the port when the last user closes the tty
  rfcomm: move rfcomm_get_device() before rfcomm_dev_activate()
  rfcomm: always wait for a bt connection on open()
  rfcomm: remove rfcomm_carrier_raised()

 net/bluetooth/rfcomm/tty.c | 103 +++++++++++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 37 deletions(-)

-- 
1.8.5.2

[PATCH v3 1/4] rfcomm: release the port when the last user closes the tty

[Gianluca Anzolin]

This patch fixes a userspace regression introduced by the commit
29cd718b.

If the rfcomm device was created with the flag RFCOMM_RELEASE_ONHUP the
user space expects that the tty_port is released as soon as the last
process closes the tty.

The current code attempts to release the port in the function
rfcomm_dev_state_change(). However it won't get a reference to the
relevant tty to send a HUP: at that point the tty is already destroyed
and therefore NULL.

This patch fixes the regression by taking over the tty refcount in the
tty install method(). This way the tty_port is automatically released as
soon as the tty is destroyed.

As a consequence the check for RFCOMM_RELEASE_ONHUP flag in the hangup()
method is now redundant. Instead we have to be careful with the reference
counting in the rfcomm_release_dev() function.

Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
Reported-by: Alexander Holler <holler@ahsoftware.de>
---
 net/bluetooth/rfcomm/tty.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..a535ef1 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,7 +437,8 @@ static int rfcomm_release_dev(void __user *arg)
 		tty_kref_put(tty);
 	}
 
-	if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
 		tty_port_put(&dev->port);
 
 	tty_port_put(&dev->port);
@@ -670,10 +671,20 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	/* install the tty_port */
 	err = tty_port_install(&dev->port, driver, tty);
-	if (err)
+	if (err) {
 		rfcomm_tty_cleanup(tty);
+		return err;
+	}
 
-	return err;
+	/* take over the tty_port reference if the port was created with the
+	 * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+	 * when the last process closes the tty. The behaviour is expected by
+	 * userspace.
+	 */
+	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+		tty_port_put(&dev->port);
+
+	return 0;
 }
 
 static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
@@ -1010,10 +1021,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 	BT_DBG("tty %p dev %p", tty, dev);
 
 	tty_port_hangup(&dev->port);
-
-	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
-	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
-		tty_port_put(&dev->port);
 }
 
 static int rfcomm_tty_tiocmget(struct tty_struct *tty)
-- 
1.8.5.2

[PATCH v3 2/4] rfcomm: move rfcomm_get_device() before rfcomm_dev_activate()

[Gianluca Anzolin]

This is a preparatory patch which moves the rfcomm_get_device()
definition before rfcomm_dev_activate() where it will be used.

Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
 net/bluetooth/rfcomm/tty.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index a535ef1..32ef9f9 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -103,6 +103,22 @@ static void rfcomm_dev_destruct(struct tty_port *port)
 	module_put(THIS_MODULE);
 }
 
+static struct device *rfcomm_get_device(struct rfcomm_dev *dev)
+{
+	struct hci_dev *hdev;
+	struct hci_conn *conn;
+
+	hdev = hci_get_route(&dev->dst, &dev->src);
+	if (!hdev)
+		return NULL;
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &dev->dst);
+
+	hci_dev_put(hdev);
+
+	return conn ? &conn->dev : NULL;
+}
+
 /* device-specific initialization: open the dlc */
 static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
 {
@@ -169,22 +185,6 @@ static struct rfcomm_dev *rfcomm_dev_get(int id)
 	return dev;
 }
 
-static struct device *rfcomm_get_device(struct rfcomm_dev *dev)
-{
-	struct hci_dev *hdev;
-	struct hci_conn *conn;
-
-	hdev = hci_get_route(&dev->dst, &dev->src);
-	if (!hdev)
-		return NULL;
-
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &dev->dst);
-
-	hci_dev_put(hdev);
-
-	return conn ? &conn->dev : NULL;
-}
-
 static ssize_t show_address(struct device *tty_dev, struct device_attribute *attr, char *buf)
 {
 	struct rfcomm_dev *dev = dev_get_drvdata(tty_dev);
-- 
1.8.5.2

[PATCH v3 3/4] rfcomm: always wait for a bt connection on open()

[Gianluca Anzolin]

This patch fixes two regressions introduced with the recent rfcomm tty
rework.

The current code uses the carrier_raised() method to wait for the
bluetooth connection when a process opens the tty.

However processes may open the port with the O_NONBLOCK flag or set the
CLOCAL termios flag: in these cases the open() syscall returns
immediately without waiting for the bluetooth connection to
complete.

This behaviour confuses userspace which expects an established bluetooth
connection.

The patch restores the old behaviour by waiting for the connection in
rfcomm_dev_activate() and removes carrier_raised() from the tty_port ops.

As a side effect the new code also fixes the case in which the rfcomm
tty device is created with the flag RFCOMM_REUSE_DLC: the old code
didn't call device_move() and ModemManager skipped the detection
probe. Now device_move() is always called inside rfcomm_dev_activate().

Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
Reported-by: Andrey Vihrov <andrey.vihrov@gmail.com>
Reported-by: Beson Chow <blc+bluez@mail.vanade.com>
---
 net/bluetooth/rfcomm/tty.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 32ef9f9..aeabade 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -58,6 +58,7 @@ struct rfcomm_dev {
 	uint			modem_status;
 
 	struct rfcomm_dlc	*dlc;
+	wait_queue_head_t       conn_wait;
 
 	struct device		*tty_dev;
 
@@ -123,8 +124,40 @@ static struct device *rfcomm_get_device(struct rfcomm_dev *dev)
 static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
 {
 	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+	DEFINE_WAIT(wait);
+	int err;
+
+	err = rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel);
+	if (err)
+		return err;
+
+	while (1) {
+		prepare_to_wait(&dev->conn_wait, &wait, TASK_INTERRUPTIBLE);
 
-	return rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel);
+		if (dev->dlc->state == BT_CLOSED) {
+			err = -dev->err;
+			break;
+		}
+
+		if (dev->dlc->state == BT_CONNECTED)
+			break;
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
+			break;
+		}
+
+		tty_unlock(tty);
+		schedule();
+		tty_lock(tty);
+	}
+	finish_wait(&dev->conn_wait, &wait);
+
+	if (!err)
+		device_move(dev->tty_dev, rfcomm_get_device(dev),
+			    DPM_ORDER_DEV_AFTER_PARENT);
+
+	return err;
 }
 
 /* we block the open until the dlc->state becomes BT_CONNECTED */
@@ -151,7 +184,6 @@ static const struct tty_port_operations rfcomm_port_ops = {
 	.destruct = rfcomm_dev_destruct,
 	.activate = rfcomm_dev_activate,
 	.shutdown = rfcomm_dev_shutdown,
-	.carrier_raised = rfcomm_dev_carrier_raised,
 };
 
 static struct rfcomm_dev *__rfcomm_dev_get(int id)
@@ -258,6 +290,7 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc)
 
 	tty_port_init(&dev->port);
 	dev->port.ops = &rfcomm_port_ops;
+	init_waitqueue_head(&dev->conn_wait);
 
 	skb_queue_head_init(&dev->pending);
 
@@ -576,12 +609,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 	BT_DBG("dlc %p dev %p err %d", dlc, dev, err);
 
 	dev->err = err;
-	if (dlc->state == BT_CONNECTED) {
-		device_move(dev->tty_dev, rfcomm_get_device(dev),
-			    DPM_ORDER_DEV_AFTER_PARENT);
+	wake_up_interruptible(&dev->conn_wait);
 
-		wake_up_interruptible(&dev->port.open_wait);
-	} else if (dlc->state == BT_CLOSED)
+	if (dlc->state == BT_CLOSED)
 		tty_port_tty_hangup(&dev->port, false);
 }
 
@@ -1103,7 +1133,7 @@ int __init rfcomm_init_ttys(void)
 	rfcomm_tty_driver->subtype	= SERIAL_TYPE_NORMAL;
 	rfcomm_tty_driver->flags	= TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
 	rfcomm_tty_driver->init_termios	= tty_std_termios;
-	rfcomm_tty_driver->init_termios.c_cflag	= B9600 | CS8 | CREAD | HUPCL;
+	rfcomm_tty_driver->init_termios.c_cflag	= B9600 | CS8 | CREAD | HUPCL | CLOCAL;
 	rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
 	tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
 
-- 
1.8.5.2

[PATCH v3 4/4] rfcomm: remove rfcomm_carrier_raised()

[Gianluca Anzolin]

Remove the rfcomm_carrier_raised() definition as that function isn't
used anymore.

Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
 net/bluetooth/rfcomm/tty.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index aeabade..f9c0980a 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -160,14 +160,6 @@ static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
 	return err;
 }
 
-/* we block the open until the dlc->state becomes BT_CONNECTED */
-static int rfcomm_dev_carrier_raised(struct tty_port *port)
-{
-	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
-
-	return (dev->dlc->state == BT_CONNECTED);
-}
-
 /* device-specific cleanup: close the dlc */
 static void rfcomm_dev_shutdown(struct tty_port *port)
 {
-- 
1.8.5.2

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Marcel Holtmann]

Hi Gianluca,

> The following patches fix three regressions introduced with the
> rfcomm tty_port conversion.
> 
> The first patch restores the expected behaviour of the rfcomm port when
> it's created with the flag RFCOMM_RELEASE_ONHUP.
> 
> The second patch moves rfcomm_get_device() and is preparatory for the
> third patch.
> 
> The third patch fixes two regressions:
> 1) when the tty is opened with O_NONBLOCK or CLOCAL is set (fixes
>   wvdial)
> 2) when the rfcomm device is created with the flag RFCOMM_REUSE_DLC
>   (fixes ModemManager)
> 
> The fourth patch removes rfcomm_dev_carrier_raised().
> 
> Changes from v2:
>  * Fix the style of if statements in rfcomm_dev_activate().
> 
> Changes from v1:
>  * Removed the device_move() fix which is now incorporated in a new patch.
> 
> Gianluca Anzolin (4):
>  rfcomm: release the port when the last user closes the tty
>  rfcomm: move rfcomm_get_device() before rfcomm_dev_activate()
>  rfcomm: always wait for a bt connection on open()
>  rfcomm: remove rfcomm_carrier_raised()
> 
> net/bluetooth/rfcomm/tty.c | 103 +++++++++++++++++++++++++++++----------------
> 1 file changed, 66 insertions(+), 37 deletions(-)

all 4 patches have been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Alexander Holler]

Am 06.01.2014 22:57, schrieb Marcel Holtmann:

> all 4 patches have been applied to bluetooth-next tree.

Maybe a bit late, but I've just seen they miss a Cc: 
stable@vger.kernel.org to automatically end up in 3.12 and 3.13 too.

Regards,

Alexander Holler

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Marcel Holtmann]

Hi Alexander,

>> all 4 patches have been applied to bluetooth-next tree.
> 
> Maybe a bit late, but I've just seen they miss a Cc: stable@vger.kernel.org to automatically end up in 3.12 and 3.13 too.

we can always promote them to stable. On Purpose I wanted them to cycle through bluetooth-next for a while to make sure they do not cause any other regressions.

Regards

Marcel

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Gianluca Anzolin]

On Mon, Jan 20, 2014 at 09:37:14AM -0800, Marcel Holtmann wrote:
> Hi Alexander,
> 
> >> all 4 patches have been applied to bluetooth-next tree.
> > 
> > Maybe a bit late, but I've just seen they miss a Cc: stable@vger.kernel.org to automatically end up in 3.12 and 3.13 too.
> 
> we can always promote them to stable. On Purpose I wanted them to cycle through bluetooth-next for a while to make sure they do not cause any other regressions.
> 
> Regards
> 
> Marcel
> 

Unfortunately it seems I overlooked the fact that rfcomm_dev_activate() is
called with the port->mutex held. So patches 2/3/4 cause a regression I missed
because I didn't turn on the appropriate debug options (circular locking
dependency, a bug report already appeared on this list).

I'm afraid this all stems from my partial knowledge of the tty_port code and
unfortunately I don't know how to solve the problem right now.

I think it's better to revert those patches for the moment.

Regards,

Gianluca

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Marcel Holtmann]

Hi Gianluca,

>>>> all 4 patches have been applied to bluetooth-next tree.
>>> 
>>> Maybe a bit late, but I've just seen they miss a Cc: stable@vger.kernel.org to automatically end up in 3.12 and 3.13 too.
>> 
>> we can always promote them to stable. On Purpose I wanted them to cycle through bluetooth-next for a while to make sure they do not cause any other regressions.
>> 
> 
> Unfortunately it seems I overlooked the fact that rfcomm_dev_activate() is
> called with the port->mutex held. So patches 2/3/4 cause a regression I missed
> because I didn't turn on the appropriate debug options (circular locking
> dependency, a bug report already appeared on this list).
> 
> I'm afraid this all stems from my partial knowledge of the tty_port code and
> unfortunately I don't know how to solve the problem right now.
> 
> I think it's better to revert those patches for the moment.

can we get this fixed instead. If I revert them, I have a different issue. So I am trading one bug for another one.

Regards

Marcel

Re: [PATCH v3 0/4] Regression fixes for rfcomm/tty.c

[Alexander Holler]

Am 28.01.2014 09:31, schrieb Gianluca Anzolin:

> Unfortunately it seems I overlooked the fact that rfcomm_dev_activate() is
> called with the port->mutex held. So patches 2/3/4 cause a regression I missed
> because I didn't turn on the appropriate debug options (circular locking
> dependency, a bug report already appeared on this list).
>
> I'm afraid this all stems from my partial knowledge of the tty_port code and
> unfortunately I don't know how to solve the problem right now.

Understandable, it's like a mine field. ;) Maybe it might make sense to 
add Alan Cox to Cc, I think he's active again and knows a lot about 
tty_port.

> I think it's better to revert those patches for the moment.

I prefer to still use those patches because without them, I have a more 
serious problem (at least for my use cases, which happily haven't run 
into that deadlock).

But thanks for notifying me/us about the possibility of a deadlock when 
using your patches.

Regards,

Alexander Holler