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

[PATCH v2 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 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 | 100 ++++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 37 deletions(-)

-- 
1.8.5.2

[PATCH v2 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 v2 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 v2 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 | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 32ef9f9..65c0699 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,37 @@ 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);
+
+		if (dev->dlc->state == BT_CLOSED) {
+			err = -dev->err;
+			break;
+		} else if (dev->dlc->state == BT_CONNECTED)
+			break;
+		else if (signal_pending(current)) {
+			err = -ERESTARTSYS;
+			break;
+		}
+
+		tty_unlock(tty);
+		schedule();
+		tty_lock(tty);
+	}
+	finish_wait(&dev->conn_wait, &wait);
 
-	return rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel);
+	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 +181,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 +287,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 +606,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 +1130,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

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

[Marcel Holtmann]

Hi Gianluca,

> 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 | 43 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index 32ef9f9..65c0699 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,37 @@ 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);
> +
> +		if (dev->dlc->state == BT_CLOSED) {
> +			err = -dev->err;
> +			break;
> +		} else if (dev->dlc->state == BT_CONNECTED)
> +			break;
> +		else if (signal_pending(current)) {
> +			err = -ERESTARTSYS;
> +			break;
> +		}

don’t do an if-else-else if statement here. Just break.

	if (dev->dlc->state == BT_CLOSED) {
		..
		break;	
	}

	if (dev->dlc->state == BT_CONNECTED)
		break;

	if (signal_pending(..)) {
		..
		break;
	}

> +
> +		tty_unlock(tty);
> +		schedule();
> +		tty_lock(tty);
> +	}
> +	finish_wait(&dev->conn_wait, &wait);
> 
> -	return rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel);
> +	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 +181,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 +287,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 +606,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 +1130,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;

Is adding CLOCAL by default intentional?

> 	rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
> 	tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);

Regards

Marcel

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

[Gianluca Anzolin]

On Mon, Jan 06, 2014 at 11:17:06AM -0800, Marcel Holtmann wrote:
> don’t do an if-else-else if statement here. Just break.
> 
> 	if (dev->dlc->state == BT_CLOSED) {
> 		..
> 		break;	
> 	}
> 
> 	if (dev->dlc->state == BT_CONNECTED)
> 		break;
> 
> 	if (signal_pending(..)) {
> 		..
> 		break;
> 	}

I'll change the code.

> > -	rfcomm_tty_driver->init_termios.c_cflag	= B9600 | CS8 | CREAD | HUPCL;
> > +	rfcomm_tty_driver->init_termios.c_cflag	= B9600 | CS8 | CREAD | HUPCL | CLOCAL;
> 
> Is adding CLOCAL by default intentional?

Yes, I removed it before because I relied on the carrier_raised() tty_port
method. But it turned out it wasn't a good idea because some code (wvdial
for example) set that flag on exit, leaving a non-working tty for subsequent
open() calls.

Now I restored the original flags, even if CLOCAL is actually ignored by the
code.

I will shortly send a third iteration with the fixed if statements.

Thanks,
Gianluca

> > 	rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
> > 	tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
> 
> Regards
> 
> Marcel
> 

[PATCH v2 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 65c0699..6d96954 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -157,14 +157,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