[PATCH 1/8] Take proper tty references in net/bluetooth/rfcomm/tty.c

[PATCH 1/8] Take proper tty references in net/bluetooth/rfcomm/tty.c

[Gianluca Anzolin]

In net/bluetooth/rfcomm/tty.c the struct tty_struct is used without taking
references. This leads to panics because the structure is freed under our feet
while being used.

Fix it by taking references properly, using the tty_port_* helpers when
possible.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index b6e44ad..6c3efb5 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -333,10 +333,11 @@ static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
 static void rfcomm_wfree(struct sk_buff *skb)
 {
 	struct rfcomm_dev *dev = (void *) skb->sk;
-	struct tty_struct *tty = dev->port.tty;
+
 	atomic_sub(skb->truesize, &dev->wmem_alloc);
-	if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags) && tty)
-		tty_wakeup(tty);
+	if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags))
+		tty_port_tty_wakeup(&dev->port);
+
 	tty_port_put(&dev->port);
 }
 
@@ -410,6 +411,7 @@ static int rfcomm_release_dev(void __user *arg)
 {
 	struct rfcomm_dev_req req;
 	struct rfcomm_dev *dev;
+	struct tty_struct *tty;
 
 	if (copy_from_user(&req, arg, sizeof(req)))
 		return -EFAULT;
@@ -429,8 +431,11 @@ static int rfcomm_release_dev(void __user *arg)
 		rfcomm_dlc_close(dev->dlc, 0);
 
 	/* Shut down TTY synchronously before freeing rfcomm_dev */
-	if (dev->port.tty)
-		tty_vhangup(dev->port.tty);
+	tty = tty_port_tty_get(&dev->port);
+	if (tty) {
+		tty_vhangup(tty);
+		tty_kref_put(tty);
+	}
 
 	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
 		rfcomm_dev_del(dev);
@@ -563,6 +568,7 @@ static void rfcomm_dev_data_ready(struct rfcomm_dlc *dlc, struct sk_buff *skb)
 static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 {
 	struct rfcomm_dev *dev = dlc->owner;
+	struct tty_struct *tty;
 	if (!dev)
 		return;
 
@@ -572,7 +578,8 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 	wake_up_interruptible(&dev->wait);
 
 	if (dlc->state == BT_CLOSED) {
-		if (!dev->port.tty) {
+		tty = tty_port_tty_get(&dev->port);
+		if (!tty) {
 			if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
 				/* Drop DLC lock here to avoid deadlock
 				 * 1. rfcomm_dev_get will take rfcomm_dev_lock
@@ -591,8 +598,10 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 				tty_port_put(&dev->port);
 				rfcomm_dlc_lock(dlc);
 			}
-		} else
-			tty_hangup(dev->port.tty);
+		} else {
+			tty_hangup(tty);
+			tty_kref_put(tty);
+		}
 	}
 }
 
@@ -604,10 +613,8 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
 
 	BT_DBG("dlc %p dev %p v24_sig 0x%02x", dlc, dev, v24_sig);
 
-	if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV)) {
-		if (dev->port.tty && !C_CLOCAL(dev->port.tty))
-			tty_hangup(dev->port.tty);
-	}
+	if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV))
+		tty_port_tty_hangup(&dev->port, true);
 
 	dev->modem_status =
 		((v24_sig & RFCOMM_V24_RTC) ? (TIOCM_DSR | TIOCM_DTR) : 0) |
@@ -674,7 +681,7 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 
 	rfcomm_dlc_lock(dlc);
 	tty->driver_data = dev;
-	dev->port.tty = tty;
+	tty_port_tty_set(&dev->port, tty);
 	rfcomm_dlc_unlock(dlc);
 	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
@@ -742,7 +749,7 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 
 		rfcomm_dlc_lock(dev->dlc);
 		tty->driver_data = NULL;
-		dev->port.tty = NULL;
+		tty_port_tty_set(&dev->port, NULL);
 		rfcomm_dlc_unlock(dev->dlc);
 
 		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
-- 
1.8.3.2

[PATCH 2/8] Move functions before the definition of rfcomm_port_ops

[Gianluca Anzolin]

Move rfcomm_tty_copy_pending and rfcomm_get_device before the definition of
rfcomm_port_ops.

These functions will be called later from the .activate and .shutdown methods of
the tty_port.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 6c3efb5..ff60171 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -112,6 +112,43 @@ 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;
+}
+
+static void rfcomm_tty_copy_pending(struct rfcomm_dev *dev)
+{
+	struct sk_buff *skb;
+	int inserted = 0;
+
+	BT_DBG("dev %p", dev);
+
+	rfcomm_dlc_lock(dev->dlc);
+
+	while ((skb = skb_dequeue(&dev->pending))) {
+		inserted += tty_insert_flip_string(&dev->port, skb->data,
+				skb->len);
+		kfree_skb(skb);
+	}
+
+	rfcomm_dlc_unlock(dev->dlc);
+
+	if (inserted > 0)
+		tty_flip_buffer_push(&dev->port);
+}
+
 static const struct tty_port_operations rfcomm_port_ops = {
 	.destruct = rfcomm_dev_destruct,
 };
@@ -147,22 +184,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);
@@ -624,27 +645,6 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
 }
 
 /* ---- TTY functions ---- */
-static void rfcomm_tty_copy_pending(struct rfcomm_dev *dev)
-{
-	struct sk_buff *skb;
-	int inserted = 0;
-
-	BT_DBG("dev %p", dev);
-
-	rfcomm_dlc_lock(dev->dlc);
-
-	while ((skb = skb_dequeue(&dev->pending))) {
-		inserted += tty_insert_flip_string(&dev->port, skb->data,
-				skb->len);
-		kfree_skb(skb);
-	}
-
-	rfcomm_dlc_unlock(dev->dlc);
-
-	if (inserted > 0)
-		tty_flip_buffer_push(&dev->port);
-}
-
 static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 {
 	DECLARE_WAITQUEUE(wait, current);
-- 
1.8.3.2

[PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Gianluca Anzolin]

Move the device initialization from rfcomm_tty_open to the .activate function of
tty_port.

At the same time move also the device shutdown from rfcomm_tty_close to the
.shutdown function of tty_port.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index ff60171..9b0b064 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -149,8 +149,80 @@ static void rfcomm_tty_copy_pending(struct rfcomm_dev *dev)
 		tty_flip_buffer_push(&dev->port);
 }
 
+/*
+ * Do the device-specific initialization by opening the DLC and waiting for
+ * a connection.
+ */
+static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+	struct rfcomm_dlc *dlc = dev->dlc;
+	int err;
+
+	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
+	if (err < 0)
+		goto error_no_dlc;
+
+	/* Wait for DLC to connect */
+	add_wait_queue(&dev->wait, &wait);
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (dlc->state == BT_CLOSED) {
+			err = -dev->err;
+			break;
+		}
+
+		if (dlc->state == BT_CONNECTED)
+			break;
+
+		if (signal_pending(current)) {
+			err = -EINTR;
+			break;
+		}
+
+		tty_unlock(tty);
+		schedule();
+		tty_lock(tty);
+	}
+	set_current_state(TASK_RUNNING);
+	remove_wait_queue(&dev->wait, &wait);
+
+	if (err < 0)
+		goto error_no_connection;
+
+	device_move(dev->tty_dev, rfcomm_get_device(dev),
+	            DPM_ORDER_DEV_AFTER_PARENT);
+
+	rfcomm_tty_copy_pending(dev);
+	rfcomm_dlc_unthrottle(dlc);
+	return 0;
+
+error_no_connection:
+	rfcomm_dlc_close(dlc, err);
+error_no_dlc:
+	return err;
+}
+
+/*
+ * Undo the device initialization by closing the DLC
+ */
+static void rfcomm_dev_shutdown(struct tty_port *port)
+{
+	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+
+	if (dev->tty_dev->parent)
+		device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
+
+	/* Close DLC */
+	rfcomm_dlc_close(dev->dlc, 0);
+}
+
 static const struct tty_port_operations rfcomm_port_ops = {
 	.destruct = rfcomm_dev_destruct,
+	.activate = rfcomm_dev_activate,
+	.shutdown = rfcomm_dev_shutdown,
 };
 
 static struct rfcomm_dev *__rfcomm_dev_get(int id)
@@ -647,11 +719,10 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
 /* ---- TTY functions ---- */
 static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 {
-	DECLARE_WAITQUEUE(wait, current);
 	struct rfcomm_dev *dev;
 	struct rfcomm_dlc *dlc;
 	unsigned long flags;
-	int err, id;
+	int id;
 
 	id = tty->index;
 
@@ -685,44 +756,7 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 	rfcomm_dlc_unlock(dlc);
 	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
-	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
-	if (err < 0)
-		return err;
-
-	/* Wait for DLC to connect */
-	add_wait_queue(&dev->wait, &wait);
-	while (1) {
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		if (dlc->state == BT_CLOSED) {
-			err = -dev->err;
-			break;
-		}
-
-		if (dlc->state == BT_CONNECTED)
-			break;
-
-		if (signal_pending(current)) {
-			err = -EINTR;
-			break;
-		}
-
-		tty_unlock(tty);
-		schedule();
-		tty_lock(tty);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(&dev->wait, &wait);
-
-	if (err == 0)
-		device_move(dev->tty_dev, rfcomm_get_device(dev),
-			    DPM_ORDER_DEV_AFTER_PARENT);
-
-	rfcomm_tty_copy_pending(dev);
-
-	rfcomm_dlc_unthrottle(dev->dlc);
-
-	return err;
+	return 0;
 }
 
 static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
@@ -739,12 +773,8 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 	spin_lock_irqsave(&dev->port.lock, flags);
 	if (!--dev->port.count) {
 		spin_unlock_irqrestore(&dev->port.lock, flags);
-		if (dev->tty_dev->parent)
-			device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
-
-		/* Close DLC and dettach TTY */
-		rfcomm_dlc_close(dev->dlc, 0);
 
+		/* detach the TTY */
 		clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
 		rfcomm_dlc_lock(dev->dlc);
-- 
1.8.3.2

[PATCH 4/8] Move tty initialization and cleanup out of open/close

[Gianluca Anzolin]

Move the tty initialization from rfcomm_tty_open to rfcomm_tty_install and do
the same for the cleanup moving the code from rfcomm_tty_close to
rfcomm_tty_cleanup.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 9b0b064..c1dd55d 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -717,10 +717,55 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
 }
 
 /* ---- TTY functions ---- */
+/*
+ * here we acquire the tty_port reference since it's here the tty is first used
+ * by setting the termios. We also populate the driver_data field and install
+ * the tty_port.
+ */
+static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+        struct rfcomm_dev *dev;
+        int err;
+
+        dev = rfcomm_dev_get(tty->index);
+        if (!dev)
+	        return -ENODEV;
+
+        /* Attach TTY */
+        rfcomm_dlc_lock(dev->dlc);
+        tty->driver_data = dev;
+        rfcomm_dlc_unlock(dev->dlc);
+        set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+
+        err = tty_port_install(&dev->port, driver, tty);
+        if (err < 0) {
+                clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+                tty_port_put(&dev->port);
+        }
+
+        return err;
+}
+
+/*
+ * here we do the reverse, by clearing the tty fields and releasing
+ * the reference to tty_port.
+ */
+static void rfcomm_tty_cleanup(struct tty_struct *tty)
+{
+	struct rfcomm_dev *dev = tty->driver_data;
+
+        /* Remove driver data */
+        clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+        rfcomm_dlc_lock(dev->dlc);
+        tty->driver_data = NULL;
+        rfcomm_dlc_unlock(dev->dlc);
+
+        tty_port_put(&dev->port);
+}
+
 static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 {
-	struct rfcomm_dev *dev;
-	struct rfcomm_dlc *dlc;
+	struct rfcomm_dev *dev = tty->driver_data;
 	unsigned long flags;
 	int id;
 
@@ -728,14 +773,6 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 
 	BT_DBG("tty %p id %d", tty, id);
 
-	/* We don't leak this refcount. For reasons which are not entirely
-	   clear, the TTY layer will call our ->close() method even if the
-	   open fails. We decrease the refcount there, and decreasing it
-	   here too would cause breakage. */
-	dev = rfcomm_dev_get(id);
-	if (!dev)
-		return -ENODEV;
-
 	BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
 	       dev->channel, dev->port.count);
 
@@ -746,16 +783,6 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 	}
 	spin_unlock_irqrestore(&dev->port.lock, flags);
 
-	dlc = dev->dlc;
-
-	/* Attach TTY and open DLC */
-
-	rfcomm_dlc_lock(dlc);
-	tty->driver_data = dev;
-	tty_port_tty_set(&dev->port, tty);
-	rfcomm_dlc_unlock(dlc);
-	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
-
 	return 0;
 }
 
@@ -764,9 +791,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 	struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
 	unsigned long flags;
 
-	if (!dev)
-		return;
-
 	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
 						dev->port.count);
 
@@ -774,14 +798,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 	if (!--dev->port.count) {
 		spin_unlock_irqrestore(&dev->port.lock, flags);
 
-		/* detach the TTY */
-		clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
-
-		rfcomm_dlc_lock(dev->dlc);
-		tty->driver_data = NULL;
-		tty_port_tty_set(&dev->port, NULL);
-		rfcomm_dlc_unlock(dev->dlc);
-
 		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
 			spin_lock(&rfcomm_dev_lock);
 			list_del_init(&dev->list);
@@ -791,8 +807,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 		}
 	} else
 		spin_unlock_irqrestore(&dev->port.lock, flags);
-
-	tty_port_put(&dev->port);
 }
 
 static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -1165,6 +1179,8 @@ static const struct tty_operations rfcomm_ops = {
 	.wait_until_sent	= rfcomm_tty_wait_until_sent,
 	.tiocmget		= rfcomm_tty_tiocmget,
 	.tiocmset		= rfcomm_tty_tiocmset,
+	.install                = rfcomm_tty_install,
+	.cleanup                = rfcomm_tty_cleanup,
 };
 
 int __init rfcomm_init_ttys(void)
-- 
1.8.3.2

[PATCH 5/8] Use the tty_port_* functions in tty_open/tty_close/tty_hangup

[Gianluca Anzolin]

Use the tty_port_open tty_port_close and tty_port_hangup functions that manage
properly the port.count and the tty_port operations.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index c1dd55d..c8ef06d 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -766,47 +766,23 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
 static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
 {
 	struct rfcomm_dev *dev = tty->driver_data;
-	unsigned long flags;
-	int id;
-
-	id = tty->index;
 
-	BT_DBG("tty %p id %d", tty, id);
+	BT_DBG("tty %p id %d", tty, tty->index);
 
 	BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
 	       dev->channel, dev->port.count);
 
-	spin_lock_irqsave(&dev->port.lock, flags);
-	if (++dev->port.count > 1) {
-		spin_unlock_irqrestore(&dev->port.lock, flags);
-		return 0;
-	}
-	spin_unlock_irqrestore(&dev->port.lock, flags);
-
-	return 0;
+	return tty_port_open(&dev->port, tty, filp);
 }
 
 static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 {
 	struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
-	unsigned long flags;
 
 	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
 						dev->port.count);
 
-	spin_lock_irqsave(&dev->port.lock, flags);
-	if (!--dev->port.count) {
-		spin_unlock_irqrestore(&dev->port.lock, flags);
-
-		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
-			spin_lock(&rfcomm_dev_lock);
-			list_del_init(&dev->list);
-			spin_unlock(&rfcomm_dev_lock);
-
-			tty_port_put(&dev->port);
-		}
-	} else
-		spin_unlock_irqrestore(&dev->port.lock, flags);
+	tty_port_close(&dev->port, tty, filp);
 }
 
 static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -1106,11 +1082,10 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 
 	BT_DBG("tty %p dev %p", tty, dev);
 
-	if (!dev)
-		return;
-
 	rfcomm_tty_flush_buffer(tty);
 
+	tty_port_hangup(&dev->port);
+
 	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
 		if (rfcomm_dev_get(dev->id) == NULL)
 			return;
-- 
1.8.3.2

[PATCH 6/8] Fix the reference counting of tty_port

[Gianluca Anzolin]

The tty_port can be released in two places: in rfcomm_tty_hangup when the flag
RFCOMM_RELEASE_ONHUP is set and there is a HUP and in rfcomm_release_dev.

There we set the flag RFCOMM_TTY_RELEASED so that no other function can get a
reference of the tty_port.

The destructor is changed to remove the device from the list.

Remove also rfcomm_dev_del which isn't used anymore.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index c8ef06d..0d61d65 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -77,11 +77,7 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig);
 /* ---- Device functions ---- */
 
 /*
- * The reason this isn't actually a race, as you no doubt have a little voice
- * screaming at you in your head, is that the refcount should never actually
- * reach zero unless the device has already been taken off the list, in
- * rfcomm_dev_del(). And if that's not true, we'll hit the BUG() in
- * rfcomm_dev_destruct() anyway.
+ * the port destructor is called when the tty_port refcount reaches zero
  */
 static void rfcomm_dev_destruct(struct tty_port *port)
 {
@@ -90,10 +86,10 @@ static void rfcomm_dev_destruct(struct tty_port *port)
 
 	BT_DBG("dev %p dlc %p", dev, dlc);
 
-	/* Refcount should only hit zero when called from rfcomm_dev_del()
-	   which will have taken us off the list. Everything else are
-	   refcounting bugs. */
-	BUG_ON(!list_empty(&dev->list));
+	/* remove the dev from the list */
+	spin_lock(&rfcomm_dev_lock);
+	list_del_init(&dev->list);
+	spin_unlock(&rfcomm_dev_lock);
 
 	rfcomm_dlc_lock(dlc);
 	/* Detach DLC if it's owned by this dev */
@@ -394,27 +390,6 @@ free:
 	return err;
 }
 
-static void rfcomm_dev_del(struct rfcomm_dev *dev)
-{
-	unsigned long flags;
-	BT_DBG("dev %p", dev);
-
-	BUG_ON(test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags));
-
-	spin_lock_irqsave(&dev->port.lock, flags);
-	if (dev->port.count > 0) {
-		spin_unlock_irqrestore(&dev->port.lock, flags);
-		return;
-	}
-	spin_unlock_irqrestore(&dev->port.lock, flags);
-
-	spin_lock(&rfcomm_dev_lock);
-	list_del_init(&dev->list);
-	spin_unlock(&rfcomm_dev_lock);
-
-	tty_port_put(&dev->port);
-}
-
 /* ---- Send buffer ---- */
 static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
 {
@@ -530,8 +505,10 @@ static int rfcomm_release_dev(void __user *arg)
 		tty_kref_put(tty);
 	}
 
-	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
-		rfcomm_dev_del(dev);
+	/* release the TTY if not already done in hangup */
+	if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+		tty_port_put(&dev->port);
+
 	tty_port_put(&dev->port);
 	return 0;
 }
@@ -662,6 +639,7 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 {
 	struct rfcomm_dev *dev = dlc->owner;
 	struct tty_struct *tty;
+
 	if (!dev)
 		return;
 
@@ -687,7 +665,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 					return;
 				}
 
-				rfcomm_dev_del(dev);
+				set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
+				tty_port_put(&dev->port);
+
 				tty_port_put(&dev->port);
 				rfcomm_dlc_lock(dlc);
 			}
@@ -754,9 +734,9 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
 {
 	struct rfcomm_dev *dev = tty->driver_data;
 
-        /* Remove driver data */
-        clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
-        rfcomm_dlc_lock(dev->dlc);
+	/* Remove driver data */
+	clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+	rfcomm_dlc_lock(dev->dlc);
         tty->driver_data = NULL;
         rfcomm_dlc_unlock(dev->dlc);
 
@@ -1087,9 +1067,7 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 	tty_port_hangup(&dev->port);
 
 	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
-		if (rfcomm_dev_get(dev->id) == NULL)
-			return;
-		rfcomm_dev_del(dev);
+		set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
 		tty_port_put(&dev->port);
 	}
 }
-- 
1.8.3.2

[PATCH 7/8] Avoid a circular dependency between dev and dev->dlc

[Gianluca Anzolin]

There is a circular dependency between dev and dlc: dev takes a reference to the
dlc in rfcomm_create_dev. The dlc->tx_queue has a list of skbs with references
to dev itself.

That list is purged only when the dlc is destructed, but that cannot happen
because dev acquired a reference to it and won't release it until it's
destructed.

Since we now purge the skbs in the cleanup method we can remove the call to
rfcomm_tty_flush_buffer in rfcomm_tty_hangup.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 0d61d65..7bc603a 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -740,6 +740,9 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
         tty->driver_data = NULL;
         rfcomm_dlc_unlock(dev->dlc);
 
+        /* avoid dev and dlc->tx_queue circular dependency  */
+        skb_queue_purge(&dev->dlc->tx_queue);
+
         tty_port_put(&dev->port);
 }
 
@@ -1062,8 +1065,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 
 	BT_DBG("tty %p dev %p", tty, dev);
 
-	rfcomm_tty_flush_buffer(tty);
-
 	tty_port_hangup(&dev->port);
 
 	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
-- 
1.8.3.2

[PATCH 8/8] Add module_put in rfcomm_dev_add error path

[Gianluca Anzolin]

rfcomm_dev_add doesn't call module_put() in its error path when it took a
reference.

Fix by always calling __module_get() and module_put() in the error path.

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

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 7bc603a..40288c3 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -357,11 +357,11 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc)
 
 	rfcomm_dlc_unlock(dlc);
 
+out:
 	/* It's safe to call __module_get() here because socket already
 	   holds reference to this module. */
 	__module_get(THIS_MODULE);
 
-out:
 	spin_unlock(&rfcomm_dev_lock);
 
 	if (err < 0)
@@ -386,6 +386,7 @@ out:
 	return dev->id;
 
 free:
+	module_put(THIS_MODULE);
 	kfree(dev);
 	return err;
 }
-- 
1.8.3.2

Re: [PATCH 1/8] Take proper tty references in net/bluetooth/rfcomm/tty.c

[Peter Hurley]

[+cc Greg KH, Jiri Slaby ]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> In net/bluetooth/rfcomm/tty.c the struct tty_struct is used without taking
> references. This leads to panics because the structure is freed under our feet
> while being used.

If you have a stack trace of a panic which is due to use-after-free
of an rfcomm tty, it should go here. Otherwise, the above sentence should instead
read sometime like, "This may lead to a use-after-free of the rfcomm tty."

Everything else looks ok.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

>
> Fix it by taking references properly, using the tty_port_* helpers when
> possible.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>   net/bluetooth/rfcomm/tty.c | 35 +++++++++++++++++++++--------------
>   1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index b6e44ad..6c3efb5 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -333,10 +333,11 @@ static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
>   static void rfcomm_wfree(struct sk_buff *skb)
>   {
>   	struct rfcomm_dev *dev = (void *) skb->sk;
> -	struct tty_struct *tty = dev->port.tty;
> +
>   	atomic_sub(skb->truesize, &dev->wmem_alloc);
> -	if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags) && tty)
> -		tty_wakeup(tty);
> +	if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags))
> +		tty_port_tty_wakeup(&dev->port);
> +
>   	tty_port_put(&dev->port);
>   }
>
> @@ -410,6 +411,7 @@ static int rfcomm_release_dev(void __user *arg)
>   {
>   	struct rfcomm_dev_req req;
>   	struct rfcomm_dev *dev;
> +	struct tty_struct *tty;
>
>   	if (copy_from_user(&req, arg, sizeof(req)))
>   		return -EFAULT;
> @@ -429,8 +431,11 @@ static int rfcomm_release_dev(void __user *arg)
>   		rfcomm_dlc_close(dev->dlc, 0);
>
>   	/* Shut down TTY synchronously before freeing rfcomm_dev */
> -	if (dev->port.tty)
> -		tty_vhangup(dev->port.tty);
> +	tty = tty_port_tty_get(&dev->port);
> +	if (tty) {
> +		tty_vhangup(tty);
> +		tty_kref_put(tty);
> +	}
>
>   	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
>   		rfcomm_dev_del(dev);
> @@ -563,6 +568,7 @@ static void rfcomm_dev_data_ready(struct rfcomm_dlc *dlc, struct sk_buff *skb)
>   static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>   {
>   	struct rfcomm_dev *dev = dlc->owner;
> +	struct tty_struct *tty;
>   	if (!dev)
>   		return;
>
> @@ -572,7 +578,8 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>   	wake_up_interruptible(&dev->wait);
>
>   	if (dlc->state == BT_CLOSED) {
> -		if (!dev->port.tty) {
> +		tty = tty_port_tty_get(&dev->port);
> +		if (!tty) {
>   			if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
>   				/* Drop DLC lock here to avoid deadlock
>   				 * 1. rfcomm_dev_get will take rfcomm_dev_lock
> @@ -591,8 +598,10 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>   				tty_port_put(&dev->port);
>   				rfcomm_dlc_lock(dlc);
>   			}
> -		} else
> -			tty_hangup(dev->port.tty);
> +		} else {
> +			tty_hangup(tty);
> +			tty_kref_put(tty);
> +		}
>   	}
>   }
>
> @@ -604,10 +613,8 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
>
>   	BT_DBG("dlc %p dev %p v24_sig 0x%02x", dlc, dev, v24_sig);
>
> -	if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV)) {
> -		if (dev->port.tty && !C_CLOCAL(dev->port.tty))
> -			tty_hangup(dev->port.tty);
> -	}
> +	if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV))
> +		tty_port_tty_hangup(&dev->port, true);
>
>   	dev->modem_status =
>   		((v24_sig & RFCOMM_V24_RTC) ? (TIOCM_DSR | TIOCM_DTR) : 0) |
> @@ -674,7 +681,7 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>
>   	rfcomm_dlc_lock(dlc);
>   	tty->driver_data = dev;
> -	dev->port.tty = tty;
> +	tty_port_tty_set(&dev->port, tty);
>   	rfcomm_dlc_unlock(dlc);
>   	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
>
> @@ -742,7 +749,7 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>
>   		rfcomm_dlc_lock(dev->dlc);
>   		tty->driver_data = NULL;
> -		dev->port.tty = NULL;
> +		tty_port_tty_set(&dev->port, NULL);
>   		rfcomm_dlc_unlock(dev->dlc);
>
>   		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
>

Re: [PATCH 2/8] Move functions before the definition of rfcomm_port_ops

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> Move rfcomm_tty_copy_pending and rfcomm_get_device before the definition of
> rfcomm_port_ops.
>
> These functions will be called later from the .activate and .shutdown methods of
> the tty_port.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>

Ok.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH 4/8] Move tty initialization and cleanup out of open/close

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> Move the tty initialization from rfcomm_tty_open to rfcomm_tty_install and do
> the same for the cleanup moving the code from rfcomm_tty_close to
> rfcomm_tty_cleanup.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>

This should be patch 3/8.

Thus changes to rfcomm_tty_open()/rfcomm_tty_close() can rely on these
operations occurring. (see my comments in current patch 3/8)

> ---
>   net/bluetooth/rfcomm/tty.c | 82 +++++++++++++++++++++++++++-------------------
>   1 file changed, 49 insertions(+), 33 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index 9b0b064..c1dd55d 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -717,10 +717,55 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
>   }
>
>   /* ---- TTY functions ---- */
> +/*
> + * here we acquire the tty_port reference since it's here the tty is first used
> + * by setting the termios. We also populate the driver_data field and install
> + * the tty_port.
> + */
> +static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> +        struct rfcomm_dev *dev;
> +        int err;
> +
> +        dev = rfcomm_dev_get(tty->index);
> +        if (!dev)
> +	        return -ENODEV;
> +
> +        /* Attach TTY */
> +        rfcomm_dlc_lock(dev->dlc);
> +        tty->driver_data = dev;
> +        rfcomm_dlc_unlock(dev->dlc);
> +        set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> +
> +        err = tty_port_install(&dev->port, driver, tty);
> +        if (err < 0) {
> +                clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> +                tty_port_put(&dev->port);
> +        }
> +
> +        return err;
> +}
> +
> +/*
> + * here we do the reverse, by clearing the tty fields and releasing
> + * the reference to tty_port.
> + */
> +static void rfcomm_tty_cleanup(struct tty_struct *tty)
> +{
> +	struct rfcomm_dev *dev = tty->driver_data;
> +
> +        /* Remove driver data */
> +        clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> +        rfcomm_dlc_lock(dev->dlc);
> +        tty->driver_data = NULL;
> +        rfcomm_dlc_unlock(dev->dlc);
> +
> +        tty_port_put(&dev->port);
> +}
> +
>   static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>   {
> -	struct rfcomm_dev *dev;
> -	struct rfcomm_dlc *dlc;
> +	struct rfcomm_dev *dev = tty->driver_data;
>   	unsigned long flags;
>   	int id;
>
> @@ -728,14 +773,6 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>
>   	BT_DBG("tty %p id %d", tty, id);
>
> -	/* We don't leak this refcount. For reasons which are not entirely
> -	   clear, the TTY layer will call our ->close() method even if the
> -	   open fails. We decrease the refcount there, and decreasing it
> -	   here too would cause breakage. */
> -	dev = rfcomm_dev_get(id);
> -	if (!dev)
> -		return -ENODEV;
> -
>   	BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
>   	       dev->channel, dev->port.count);
>
> @@ -746,16 +783,6 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>   	}
>   	spin_unlock_irqrestore(&dev->port.lock, flags);
>
> -	dlc = dev->dlc;
> -
> -	/* Attach TTY and open DLC */
> -
> -	rfcomm_dlc_lock(dlc);
> -	tty->driver_data = dev;
> -	tty_port_tty_set(&dev->port, tty);
> -	rfcomm_dlc_unlock(dlc);
> -	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> -
>   	return 0;
>   }
>
> @@ -764,9 +791,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>   	struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
>   	unsigned long flags;
>
> -	if (!dev)
> -		return;
> -
>   	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
>   						dev->port.count);
>
> @@ -774,14 +798,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>   	if (!--dev->port.count) {
>   		spin_unlock_irqrestore(&dev->port.lock, flags);
>
> -		/* detach the TTY */
> -		clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> -
> -		rfcomm_dlc_lock(dev->dlc);
> -		tty->driver_data = NULL;
> -		tty_port_tty_set(&dev->port, NULL);
> -		rfcomm_dlc_unlock(dev->dlc);
> -
>   		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
>   			spin_lock(&rfcomm_dev_lock);
>   			list_del_init(&dev->list);
> @@ -791,8 +807,6 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>   		}
>   	} else
>   		spin_unlock_irqrestore(&dev->port.lock, flags);
> -
> -	tty_port_put(&dev->port);
>   }
>
>   static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
> @@ -1165,6 +1179,8 @@ static const struct tty_operations rfcomm_ops = {
>   	.wait_until_sent	= rfcomm_tty_wait_until_sent,
>   	.tiocmget		= rfcomm_tty_tiocmget,
>   	.tiocmset		= rfcomm_tty_tiocmset,
> +	.install                = rfcomm_tty_install,
> +	.cleanup                = rfcomm_tty_cleanup,
>   };
>
>   int __init rfcomm_init_ttys(void)
>

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> Move the device initialization from rfcomm_tty_open to the .activate function of
> tty_port.
>
> At the same time move also the device shutdown from rfcomm_tty_close to the
> .shutdown function of tty_port.

This patch should be merged with patch 5/8 to maintain bisectability.
Otherwise, the .activate() and .shutdown() methods will not be called.


> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>   net/bluetooth/rfcomm/tty.c | 120 ++++++++++++++++++++++++++++-----------------
>   1 file changed, 75 insertions(+), 45 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index ff60171..9b0b064 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -149,8 +149,80 @@ static void rfcomm_tty_copy_pending(struct rfcomm_dev *dev)
>   		tty_flip_buffer_push(&dev->port);
>   }
>
> +/*
> + * Do the device-specific initialization by opening the DLC and waiting for
> + * a connection.
> + */
> +static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
> +	struct rfcomm_dlc *dlc = dev->dlc;
> +	int err;
> +
> +	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
> +	if (err < 0)
> +		goto error_no_dlc;
> +
> +	/* Wait for DLC to connect */
> +	add_wait_queue(&dev->wait, &wait);
> +	while (1) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +


> +		if (dlc->state == BT_CLOSED) {
> +			err = -dev->err;
> +			break;
> +		}
> +
> +		if (dlc->state == BT_CONNECTED)
> +			break;

Please consider moving these dlc->state tests into a
.carrier_raised() port method (this is what the gsm
driver does). Then this wait loop could go away.


> +
> +		if (signal_pending(current)) {
> +			err = -EINTR;
> +			break;
> +		}
> +
> +		tty_unlock(tty);
> +		schedule();
> +		tty_lock(tty);
> +	}
> +	set_current_state(TASK_RUNNING);
> +	remove_wait_queue(&dev->wait, &wait);
> +
> +	if (err < 0)
> +		goto error_no_connection;
> +
> +	device_move(dev->tty_dev, rfcomm_get_device(dev),
> +	            DPM_ORDER_DEV_AFTER_PARENT);
> +
> +	rfcomm_tty_copy_pending(dev);
> +	rfcomm_dlc_unthrottle(dlc);
> +	return 0;
> +
> +error_no_connection:
> +	rfcomm_dlc_close(dlc, err);
> +error_no_dlc:
> +	return err;
> +}
> +
> +/*
> + * Undo the device initialization by closing the DLC
> + */
> +static void rfcomm_dev_shutdown(struct tty_port *port)
> +{
> +	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
> +
> +	if (dev->tty_dev->parent)
> +		device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
> +
> +	/* Close DLC */
> +	rfcomm_dlc_close(dev->dlc, 0);
> +}
> +
>   static const struct tty_port_operations rfcomm_port_ops = {
>   	.destruct = rfcomm_dev_destruct,
> +	.activate = rfcomm_dev_activate,
> +	.shutdown = rfcomm_dev_shutdown,
>   };
>
>   static struct rfcomm_dev *__rfcomm_dev_get(int id)
> @@ -647,11 +719,10 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
>   /* ---- TTY functions ---- */
>   static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>   {
> -	DECLARE_WAITQUEUE(wait, current);
>   	struct rfcomm_dev *dev;
>   	struct rfcomm_dlc *dlc;
>   	unsigned long flags;
> -	int err, id;
> +	int id;
>
>   	id = tty->index;
>
> @@ -685,44 +756,7 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>   	rfcomm_dlc_unlock(dlc);
>   	set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
>
> -	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
> -	if (err < 0)
> -		return err;
> -
> -	/* Wait for DLC to connect */
> -	add_wait_queue(&dev->wait, &wait);
> -	while (1) {
> -		set_current_state(TASK_INTERRUPTIBLE);
> -
> -		if (dlc->state == BT_CLOSED) {
> -			err = -dev->err;
> -			break;
> -		}
> -
> -		if (dlc->state == BT_CONNECTED)
> -			break;
> -
> -		if (signal_pending(current)) {
> -			err = -EINTR;
> -			break;
> -		}
> -
> -		tty_unlock(tty);
> -		schedule();
> -		tty_lock(tty);
> -	}
> -	set_current_state(TASK_RUNNING);
> -	remove_wait_queue(&dev->wait, &wait);
> -
> -	if (err == 0)
> -		device_move(dev->tty_dev, rfcomm_get_device(dev),
> -			    DPM_ORDER_DEV_AFTER_PARENT);
> -
> -	rfcomm_tty_copy_pending(dev);
> -
> -	rfcomm_dlc_unthrottle(dev->dlc);
> -
> -	return err;
> +	return 0;
>   }
>
>   static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
> @@ -739,12 +773,8 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>   	spin_lock_irqsave(&dev->port.lock, flags);
>   	if (!--dev->port.count) {
>   		spin_unlock_irqrestore(&dev->port.lock, flags);
> -		if (dev->tty_dev->parent)
> -			device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
> -
> -		/* Close DLC and dettach TTY */
> -		rfcomm_dlc_close(dev->dlc, 0);
>
> +		/* detach the TTY */
>   		clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
>
>   		rfcomm_dlc_lock(dev->dlc);
>

Re: [PATCH 5/8] Use the tty_port_* functions in tty_open/tty_close/tty_hangup

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> Use the tty_port_open tty_port_close and tty_port_hangup functions that manage
> properly the port.count and the tty_port operations.

Please merge this patch with 'Move device initialization and shutdown...".
See comments below.

> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>   net/bluetooth/rfcomm/tty.c | 35 +++++------------------------------
>   1 file changed, 5 insertions(+), 30 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index c1dd55d..c8ef06d 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -766,47 +766,23 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
>   static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>   {
>   	struct rfcomm_dev *dev = tty->driver_data;
> -	unsigned long flags;
> -	int id;
> -
> -	id = tty->index;
>
> -	BT_DBG("tty %p id %d", tty, id);
> +	BT_DBG("tty %p id %d", tty, tty->index);
>
>   	BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
>   	       dev->channel, dev->port.count);
>
> -	spin_lock_irqsave(&dev->port.lock, flags);
> -	if (++dev->port.count > 1) {
> -		spin_unlock_irqrestore(&dev->port.lock, flags);
> -		return 0;
> -	}
> -	spin_unlock_irqrestore(&dev->port.lock, flags);
> -
> -	return 0;
> +	return tty_port_open(&dev->port, tty, filp);

As noted in my review of patch 3/8, this needs to be in the patch which
introduces the .activate() port method to maintain bisectability.

>   }
>
>   static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>   {
>   	struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
> -	unsigned long flags;
>
>   	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
>   						dev->port.count);
>
> -	spin_lock_irqsave(&dev->port.lock, flags);
> -	if (!--dev->port.count) {
> -		spin_unlock_irqrestore(&dev->port.lock, flags);
> -
> -		if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
> -			spin_lock(&rfcomm_dev_lock);
> -			list_del_init(&dev->list);
> -			spin_unlock(&rfcomm_dev_lock);

Removal from the rfcomm_dev_list should be in a separate patch
with the list node removal in the rfcomm dev destructor from
patch 6/8.

> -
> -			tty_port_put(&dev->port);
> -		}
> -	} else
> -		spin_unlock_irqrestore(&dev->port.lock, flags);
> +	tty_port_close(&dev->port, tty, filp);

As with the tty_port_open(), this hunk needs to be in the patch which
introduces the .shutdown() port method to maintain bisectability.

>   }
>
>   static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
> @@ -1106,11 +1082,10 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
>
>   	BT_DBG("tty %p dev %p", tty, dev);
>
> -	if (!dev)
> -		return;
> -
>   	rfcomm_tty_flush_buffer(tty);
>
> +	tty_port_hangup(&dev->port);
> +

Same as above.

>   	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
>   		if (rfcomm_dev_get(dev->id) == NULL)
>   			return;
>

Re: [PATCH 5/8] Use the tty_port_* functions in tty_open/tty_close/tty_hangup

[Gianluca Anzolin]

On Tue, Jul 16, 2013 at 04:51:37PM -0400, Peter Hurley wrote:
> On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> >Use the tty_port_open tty_port_close and tty_port_hangup functions that manage
> >properly the port.count and the tty_port operations.
> 
> Please merge this patch with 'Move device initialization and shutdown...".
> See comments below.

Thank you for your review, I'll rework the patches following your
instructions.

After I sent them I noticed also there were whitespace issues, for the next
iteration I will be more careful.

Gianluca

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> The tty_port can be released in two places: in rfcomm_tty_hangup when the flag
> RFCOMM_RELEASE_ONHUP is set and there is a HUP and in rfcomm_release_dev.
>
> There we set the flag RFCOMM_TTY_RELEASED so that no other function can get a
> reference of the tty_port.
>


> The destructor is changed to remove the device from the list

Such a simple and elegant solution -- good work.

I think these changes related to rfcomm_dev_list should be in a
separate, earlier patch though.

>
> Remove also rfcomm_dev_del which isn't used anymore.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>   net/bluetooth/rfcomm/tty.c | 56 ++++++++++++++--------------------------------
>   1 file changed, 17 insertions(+), 39 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index c8ef06d..0d61d65 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -77,11 +77,7 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig);
>   /* ---- Device functions ---- */
>
>   /*
> - * The reason this isn't actually a race, as you no doubt have a little voice
> - * screaming at you in your head, is that the refcount should never actually
> - * reach zero unless the device has already been taken off the list, in
> - * rfcomm_dev_del(). And if that's not true, we'll hit the BUG() in
> - * rfcomm_dev_destruct() anyway.
> + * the port destructor is called when the tty_port refcount reaches zero
>    */
>   static void rfcomm_dev_destruct(struct tty_port *port)
>   {
> @@ -90,10 +86,10 @@ static void rfcomm_dev_destruct(struct tty_port *port)
>
>   	BT_DBG("dev %p dlc %p", dev, dlc);
>
> -	/* Refcount should only hit zero when called from rfcomm_dev_del()
> -	   which will have taken us off the list. Everything else are
> -	   refcounting bugs. */
> -	BUG_ON(!list_empty(&dev->list));
> +	/* remove the dev from the list */
> +	spin_lock(&rfcomm_dev_lock);
> +	list_del_init(&dev->list);
> +	spin_unlock(&rfcomm_dev_lock);
>
>   	rfcomm_dlc_lock(dlc);
>   	/* Detach DLC if it's owned by this dev */
> @@ -394,27 +390,6 @@ free:
>   	return err;
>   }
>
> -static void rfcomm_dev_del(struct rfcomm_dev *dev)
> -{
> -	unsigned long flags;
> -	BT_DBG("dev %p", dev);
> -
> -	BUG_ON(test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags));
> -
> -	spin_lock_irqsave(&dev->port.lock, flags);
> -	if (dev->port.count > 0) {
> -		spin_unlock_irqrestore(&dev->port.lock, flags);
> -		return;
> -	}
> -	spin_unlock_irqrestore(&dev->port.lock, flags);
> -
> -	spin_lock(&rfcomm_dev_lock);
> -	list_del_init(&dev->list);
> -	spin_unlock(&rfcomm_dev_lock);
> -
> -	tty_port_put(&dev->port);
> -}
> -
>   /* ---- Send buffer ---- */
>   static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
>   {
> @@ -530,8 +505,10 @@ static int rfcomm_release_dev(void __user *arg)
>   		tty_kref_put(tty);
>   	}
>
> -	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
> -		rfcomm_dev_del(dev);
> +	/* release the TTY if not already done in hangup */
> +	if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
> +		tty_port_put(&dev->port);
> +
>   	tty_port_put(&dev->port);
>   	return 0;
>   }
> @@ -662,6 +639,7 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>   {
>   	struct rfcomm_dev *dev = dlc->owner;
>   	struct tty_struct *tty;
> +
>   	if (!dev)
>   		return;
>
> @@ -687,7 +665,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>   					return;
>   				}
>
> -				rfcomm_dev_del(dev);
> +				set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
> +				tty_port_put(&dev->port);
> +
>   				tty_port_put(&dev->port);
>   				rfcomm_dlc_lock(dlc);
>   			}

While this is functionally correct, it ignores the larger issue in
rfcomm_dev_state_change(); namely, what prevents the rfcomm_dev from being
destructed immediately after

	struct rfcomm_dev *dev = dlc->owner;

If the answer to that question is the dlc lock, then the whole function is
_broken_.

No amount of reference counting will prevent the rfcomm_dev destructor
from completing once the dlc lock is dropped. (Presumably the dlc is not
subject to destruction once the lock is dropped. Is this true?)

This means:
1. Holding the dlc lock from the caller is pointless and should be dropped.
2. Some other solution is required to either preserve rfcomm_dev lifetime
    or determine that destruction is already in progress.


> @@ -754,9 +734,9 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
>   {
>   	struct rfcomm_dev *dev = tty->driver_data;
>
> -        /* Remove driver data */
> -        clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> -        rfcomm_dlc_lock(dev->dlc);
> +	/* Remove driver data */
> +	clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
> +	rfcomm_dlc_lock(dev->dlc);
>           tty->driver_data = NULL;
>           rfcomm_dlc_unlock(dev->dlc);
>
> @@ -1087,9 +1067,7 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
>   	tty_port_hangup(&dev->port);
>
>   	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
> -		if (rfcomm_dev_get(dev->id) == NULL)
> -			return;
> -		rfcomm_dev_del(dev);
> +		set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
>   		tty_port_put(&dev->port);
>   	}
>   }
>

Re: [PATCH 8/8] Add module_put in rfcomm_dev_add error path

[Peter Hurley]

On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> rfcomm_dev_add doesn't call module_put() in its error path when it took a
> reference.

Good catch.

> Fix by always calling __module_get() and module_put() in the error path.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>   net/bluetooth/rfcomm/tty.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index 7bc603a..40288c3 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -357,11 +357,11 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc)
>
>   	rfcomm_dlc_unlock(dlc);
>
> +out:
>   	/* It's safe to call __module_get() here because socket already
>   	   holds reference to this module. */
>   	__module_get(THIS_MODULE);
>
> -out:
>   	spin_unlock(&rfcomm_dev_lock);
>
>   	if (err < 0)

Is it possible to reverse the order-of-operations here:

	spin_unlock(&rfcomm_dev_lock);
	__module_get(THIS_MODULE);

Then the out: label could be moved to the function exit as noted below.

The rfcomm_dev_lock use here is what creates the mess in rfcomm_dev_state_change().
Anyone know why the rfcomm_dev_lock critical section is so ridiculously large
here? Does it really protect the module count??


> @@ -386,6 +386,7 @@ out:
>   	return dev->id;
>
>   free:
> +	module_put(THIS_MODULE);

     out:
	spin_unlock(&rfcomm_dev_lock);

>   	kfree(dev);
>   	return err;
>   }
>

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Gianluca Anzolin]

On Wed, Jul 17, 2013 at 10:02:31AM -0400, Peter Hurley wrote:
> >
> >@@ -687,7 +665,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
> >  					return;
> >  				}
> >
> >-				rfcomm_dev_del(dev);
> >+				set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
> >+				tty_port_put(&dev->port);
> >+
> >  				tty_port_put(&dev->port);
> >  				rfcomm_dlc_lock(dlc);
> >  			}
> 
> While this is functionally correct, it ignores the larger issue in
> rfcomm_dev_state_change(); namely, what prevents the rfcomm_dev from being
> destructed immediately after
> 
> 	struct rfcomm_dev *dev = dlc->owner;
> 
> If the answer to that question is the dlc lock, then the whole function is
> _broken_.
> 
> No amount of reference counting will prevent the rfcomm_dev destructor
> from completing once the dlc lock is dropped. (Presumably the dlc is not
> subject to destruction once the lock is dropped. Is this true?)
> 
> This means:
> 1. Holding the dlc lock from the caller is pointless and should be dropped.
> 2. Some other solution is required to either preserve rfcomm_dev lifetime
>    or determine that destruction is already in progress.

I'm afraid I lied in the commit message: there are three places where the
tty_port may be released and the code above is the third one.

I wrote that message because at first I wanted to remove that code path but
then noticed I shouldn't.

Maybe we can simply save the dev->id before releasing the lock and then feed
that integer to rfcomm_dev_get? After all if the destruction is in progress
rfcomm_dev_get(id) == NULL and we return immediately. Otherwise we release the
tty_port.

Gianluca

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Peter Hurley]

On 07/17/2013 01:05 PM, Gianluca Anzolin wrote:
> On Wed, Jul 17, 2013 at 10:02:31AM -0400, Peter Hurley wrote:
>>>
>>> @@ -687,7 +665,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>>>   					return;
>>>   				}
>>>
>>> -				rfcomm_dev_del(dev);
>>> +				set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
>>> +				tty_port_put(&dev->port);
>>> +
>>>   				tty_port_put(&dev->port);
>>>   				rfcomm_dlc_lock(dlc);
>>>   			}
>>
>> While this is functionally correct, it ignores the larger issue in
>> rfcomm_dev_state_change(); namely, what prevents the rfcomm_dev from being
>> destructed immediately after
>>
>> 	struct rfcomm_dev *dev = dlc->owner;
>>
>> If the answer to that question is the dlc lock, then the whole function is
>> _broken_.
>>
>> No amount of reference counting will prevent the rfcomm_dev destructor
>> from completing once the dlc lock is dropped. (Presumably the dlc is not
>> subject to destruction once the lock is dropped. Is this true?)
>>
>> This means:
>> 1. Holding the dlc lock from the caller is pointless and should be dropped.
>> 2. Some other solution is required to either preserve rfcomm_dev lifetime
>>     or determine that destruction is already in progress.
>
> I'm afraid I lied in the commit message: there are three places where the
> tty_port may be released and the code above is the third one.
>
> I wrote that message because at first I wanted to remove that code path but
> then noticed I shouldn't.
>
> Maybe we can simply save the dev->id before releasing the lock and then feed
> that integer to rfcomm_dev_get? After all if the destruction is in progress
> rfcomm_dev_get(id) == NULL and we return immediately. Otherwise we release the
> tty_port.

I had forgotten about your earlier change to remove the rfcomm_dev node
from the list as the first op of the destructor. That does ensure
rfcomm_dev_get(id) will not return a rfcomm_dev which is destructing.

That said, preventing rfcomm_dev destruction by holding the dlc lock
is poor design (not that I'm suggesting you should be required to fix it though)
and something that at least needs documenting.

Regarding acquiring a snapshot of dev->id is fine, provided that the id
cannot be reallocated in between dropping the dlc lock and subsequently
scanning the rfcomm_dev_list for that id.

Regards,
Peter Hurley

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Peter Hurley]

On 07/17/2013 02:10 PM, Peter Hurley wrote:
> That said, preventing rfcomm_dev destruction by holding the dlc lock
> is poor design (not that I'm suggesting you should be required to fix it though)
> and something that at least needs documenting.
>
> Regarding acquiring a snapshot of dev->id is fine, provided that the id
> cannot be reallocated in between dropping the dlc lock and subsequently
> scanning the rfcomm_dev_list for that id.

Or at least a FIXME comment that the id could potentially be reallocated
between dropping the dlc lock and the subsequent rfcomm_dev_get().

Regards,
Peter Hurley

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Gianluca Anzolin]

On Thu, Jul 18, 2013 at 08:45:43AM -0400, Peter Hurley wrote:
> On 07/17/2013 02:10 PM, Peter Hurley wrote:
> >That said, preventing rfcomm_dev destruction by holding the dlc lock
> >is poor design (not that I'm suggesting you should be required to fix it though)
> >and something that at least needs documenting.
> >
> >Regarding acquiring a snapshot of dev->id is fine, provided that the id
> >cannot be reallocated in between dropping the dlc lock and subsequently
> >scanning the rfcomm_dev_list for that id.
> 
> Or at least a FIXME comment that the id could potentially be reallocated
> between dropping the dlc lock and the subsequent rfcomm_dev_get().
> 
> Regards,
> Peter Hurley

I must admit I don't know how to solve the issue you outlined. I cannot also
understand why that code exists in first place: why should we release the
device when RFCOMM_RELEASE_ONHUP is set but we didn't get a HUP?

Gianluca

Re: [PATCH 6/8] Fix the reference counting of tty_port

[Peter Hurley]

On 07/18/2013 10:13 AM, Gianluca Anzolin wrote:
> On Thu, Jul 18, 2013 at 08:45:43AM -0400, Peter Hurley wrote:
>> On 07/17/2013 02:10 PM, Peter Hurley wrote:
>>> That said, preventing rfcomm_dev destruction by holding the dlc lock
>>> is poor design (not that I'm suggesting you should be required to fix it though)
>>> and something that at least needs documenting.
>>>
>>> Regarding acquiring a snapshot of dev->id is fine, provided that the id
>>> cannot be reallocated in between dropping the dlc lock and subsequently
>>> scanning the rfcomm_dev_list for that id.
>>
>> Or at least a FIXME comment that the id could potentially be reallocated
>> between dropping the dlc lock and the subsequent rfcomm_dev_get().
>>
>> Regards,
>> Peter Hurley
>
> I must admit I don't know how to solve the issue you outlined. I cannot also
> understand why that code exists in first place: why should we release the
> device when RFCOMM_RELEASE_ONHUP is set but we didn't get a HUP?

Essentially a HUP did occur: the underlying device is gone/disconnected.
The rfcomm_dev_state_change(BT_CLOSED) is the notification that this has
happened. This event is similar to a usb disconnect or pci remove.

As far as why a user-space flag (RFCOMM_RELEASE_ONHUP) controls this
behavior, I have no idea. It pre-dates the original commit in
current mainline.

But regardless, rfcomm_dev teardown must be a supported behavior of lower-layer
device disconnects.

ISTM the central design flaw is the cross-linkage of dlc <-> rfcomm_dev.
Cross-linked structures are trivial to establish and *very* difficult
to dismantle. A solution I've used before is RCU from one direction and
spinlock from the other.

For this particular application though, it may be simpler to figure out
how to either reorder or separate the locks in rfcomm_dev_add(). If the
rfcomm_dev_lock can be dropped before acquiring the dlc lock, then
rfcomm_dev_state_change() could hold the dlc lock during rfcomm_dev
teardown. Unfortunately, it seems like that solution might allow
a not-completely-initialized rfcomm_dev to be found on the rfcomm_dev_list.

Maybe a better solution would be to completely initialize the rfcomm_dev
and dlc, and then just before registering the tty device, do the id
lookup and link in the rfcomm_dev into the rfcomm_dev_list last. The
main issue with this approach is that some means of preventing
rfcomm_dev_state_change() from acquiring a partially constructed
rfcomm_dev would need to exist. I don't see any serialization coming
from the lower-layer drivers, so the dlc->owner linkage would have
to be delayed until the rfcomm_dev was constructed and attached to
the rfcomm_dev_list. Or something like that :)

FWIW, your existing patches are a huge step forward for this code
so feel free to proceed with a v2 patchset that leaves this
problem unaddressed.

Regards,
Peter Hurley

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Gianluca Anzolin]

On Tue, Jul 16, 2013 at 04:48:22PM -0400, Peter Hurley wrote:
> >+static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
> >+{
> >+	DECLARE_WAITQUEUE(wait, current);
> >+	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
> >+	struct rfcomm_dlc *dlc = dev->dlc;
> >+	int err;
> >+
> >+	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
> >+	if (err < 0)
> >+		goto error_no_dlc;
> >+
> >+	/* Wait for DLC to connect */
> >+	add_wait_queue(&dev->wait, &wait);
> >+	while (1) {
> >+		set_current_state(TASK_INTERRUPTIBLE);
> >+
> 
> 
> >+		if (dlc->state == BT_CLOSED) {
> >+			err = -dev->err;
> >+			break;
> >+		}
> >+
> >+		if (dlc->state == BT_CONNECTED)
> >+			break;
> 
> Please consider moving these dlc->state tests into a
> .carrier_raised() port method (this is what the gsm
> driver does). Then this wait loop could go away.
> 

I have a question about this: how do I signal an error condition with
carrier_raised? In case of success I should also call some device_move,
rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in
carrier_raised directly?

Thanks,

Gianluca

> >+		goto error_no_connection;
> >+
> >+	device_move(dev->tty_dev, rfcomm_get_device(dev),
> >+	            DPM_ORDER_DEV_AFTER_PARENT);
> >+
> >+	rfcomm_tty_copy_pending(dev);
> >+	rfcomm_dlc_unthrottle(dlc);
> >+	return 0;
> >+
> >+error_no_connection:
> >+	rfcomm_dlc_close(dlc, err);
> >+error_no_dlc:
> >+	return err;
> >+}

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Peter Hurley]

On 07/20/2013 03:10 AM, Gianluca Anzolin wrote:
> On Tue, Jul 16, 2013 at 04:48:22PM -0400, Peter Hurley wrote:
>>> +static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
>>> +{
>>> +	DECLARE_WAITQUEUE(wait, current);
>>> +	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
>>> +	struct rfcomm_dlc *dlc = dev->dlc;
>>> +	int err;
>>> +
>>> +	err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
>>> +	if (err < 0)
>>> +		goto error_no_dlc;
>>> +
>>> +	/* Wait for DLC to connect */
>>> +	add_wait_queue(&dev->wait, &wait);
>>> +	while (1) {
>>> +		set_current_state(TASK_INTERRUPTIBLE);
>>> +
>>
>>
>>> +		if (dlc->state == BT_CLOSED) {
>>> +			err = -dev->err;
>>> +			break;
>>> +		}
>>> +
>>> +		if (dlc->state == BT_CONNECTED)
>>> +			break;
>>
>> Please consider moving these dlc->state tests into a
>> .carrier_raised() port method (this is what the gsm
>> driver does). Then this wait loop could go away.
>>
>
> I have a question about this: how do I signal an error condition with
> carrier_raised?

Sorry Gianluca, I should have been more specific here.

There's no need to test for dlc->state == BT_CLOSED in carrier_raised().
At the point where port->carrier_raised is called, the tty will have been
linked with the file descriptor, so if the dlc->state goes to BT_CLOSED,
then rfcomm_dev_state_change() will call
   tty_hangup() -> driver hangup() -> tty_port_hangup() -> tty_port_shutdown()
This call chain will
   1. set the file_ops to hung_up_tty_fops which will cause tty_hung_up_p() to
      return true
   2. clear ASYNCB_INITIALIZED in port->flags
   3. wakeup port->open_wait

So an open() parked in the schedule loop of tty_port_block_til_ready()
will wake and exit the loop with either -EAGAIN or -ERESTARTSYS.

rfcomm_dev_state_change() should only do a wakeup on port->open_wait when
dlc->state == BT_CONNECTED.

> In case of success I should also call some device_move,
> rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in
> carrier_raised directly?

I wouldn't. That would be a nasty hack and a potential problem if a
signal occurred.

The device_move() isn't dependent on success, and can stay in .activate().

The rfcomm_tty_copy_pending() looks to me like a hack for a lack
of flow control when receiving data for the rfcomm_dev.
[Note how rfcomm_dev_data_ready() doesn't bother to test the return
value from tty_insert_flip_string().]

For simplicity, you could leave
	rfcomm_tty_copy_pending(dev);
	rfcomm_dlc_unthrottle(dlc);
in rfcomm_tty_open(), eg.:

	err = tty_port_open(&dev->port, tty, filp);
	if (err)
		return err;
	/*
	 * FIXME: rfcomm should use proper flow control for
	 * received data. This hack will be unnecessary and can
	 * be removed when that's implemented.
	 */
	rfcomm_tty_copy_pending(dev);
	rfcomm_dlc_unthrottle(dlc);
	return 0;

Regards,
Peter Hurley

>>> +		goto error_no_connection;
>>> +
>>> +	device_move(dev->tty_dev, rfcomm_get_device(dev),
>>> +	            DPM_ORDER_DEV_AFTER_PARENT);
>>> +
>>> +	rfcomm_tty_copy_pending(dev);
>>> +	rfcomm_dlc_unthrottle(dlc);
>>> +	return 0;
>>> +
>>> +error_no_connection:
>>> +	rfcomm_dlc_close(dlc, err);
>>> +error_no_dlc:
>>> +	return err;
>>> +}

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Gianluca Anzolin]

On Sat, Jul 20, 2013 at 10:11:50AM -0400, Peter Hurley wrote:
> Sorry Gianluca, I should have been more specific here.
> 
> There's no need to test for dlc->state == BT_CLOSED in carrier_raised().
> At the point where port->carrier_raised is called, the tty will have been
> linked with the file descriptor, so if the dlc->state goes to BT_CLOSED,
> then rfcomm_dev_state_change() will call
>   tty_hangup() -> driver hangup() -> tty_port_hangup() -> tty_port_shutdown()
> This call chain will
>   1. set the file_ops to hung_up_tty_fops which will cause tty_hung_up_p() to
>      return true
>   2. clear ASYNCB_INITIALIZED in port->flags
>   3. wakeup port->open_wait
> 
> So an open() parked in the schedule loop of tty_port_block_til_ready()
> will wake and exit the loop with either -EAGAIN or -ERESTARTSYS.
> 
> rfcomm_dev_state_change() should only do a wakeup on port->open_wait when
> dlc->state == BT_CONNECTED.
> 
> >In case of success I should also call some device_move,
> >rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in
> >carrier_raised directly?
> 
> I wouldn't. That would be a nasty hack and a potential problem if a
> signal occurred.
> 
> The device_move() isn't dependent on success, and can stay in .activate().

I changed the code and it's cleaner than before, very nice. However the
device_move() is really dependent on success: the parent device is there only
when the connection has been successfully established.

So I have to call that function after the carrier is raised, or right before.
Since you already told me that calling it in the .carrier_raised method is
unwise the only place left is the state_change callback of the dlc.

Conversely in .shutdown the check for the device parent == NULL takes care of
the scenario in which the aforementioned device_move() is never called.

Another unrelated question: I'm working on the rfcomm_dev_add function to avoid
the two nested locks. When the patch is ready should I send it separately or
can I include it with the other patches?

Thanks,
Gianluca

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Peter Hurley]

On 07/21/2013 04:08 AM, Gianluca Anzolin wrote:
> On Sat, Jul 20, 2013 at 10:11:50AM -0400, Peter Hurley wrote:
>> Sorry Gianluca, I should have been more specific here.
>>
>> There's no need to test for dlc->state == BT_CLOSED in carrier_raised().
>> At the point where port->carrier_raised is called, the tty will have been
>> linked with the file descriptor, so if the dlc->state goes to BT_CLOSED,
>> then rfcomm_dev_state_change() will call
>>    tty_hangup() -> driver hangup() -> tty_port_hangup() -> tty_port_shutdown()
>> This call chain will
>>    1. set the file_ops to hung_up_tty_fops which will cause tty_hung_up_p() to
>>       return true
>>    2. clear ASYNCB_INITIALIZED in port->flags
>>    3. wakeup port->open_wait
>>
>> So an open() parked in the schedule loop of tty_port_block_til_ready()
>> will wake and exit the loop with either -EAGAIN or -ERESTARTSYS.
>>
>> rfcomm_dev_state_change() should only do a wakeup on port->open_wait when
>> dlc->state == BT_CONNECTED.
>>
>>> In case of success I should also call some device_move,
>>> rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in
>>> carrier_raised directly?
>>
>> I wouldn't. That would be a nasty hack and a potential problem if a
>> signal occurred.
>>
>> The device_move() isn't dependent on success, and can stay in .activate().
>
> I changed the code and it's cleaner than before, very nice. However the
> device_move() is really dependent on success: the parent device is there only
> when the connection has been successfully established.

Oh, right.

> So I have to call that function after the carrier is raised, or right before.
> Since you already told me that calling it in the .carrier_raised method is
> unwise the only place left is the state_change callback of the dlc.

That seems fine. [ Another place would be in rfcomm_tty_open() just after
tty_port_open() returns success -- the tty is still locked here so it won't
race with .close/.hangup() ]

I do wonder why the tty device is re-parented to the host controller device.
It's obviously not for subsystem teardown. Maybe one of the bluetooth
maintainers could clarify this? Are there userspace components waiting for
this uevent?

> Conversely in .shutdown the check for the device parent == NULL takes care of
> the scenario in which the aforementioned device_move() is never called.
>
> Another unrelated question: I'm working on the rfcomm_dev_add function to avoid
> the two nested locks. When the patch is ready should I send it separately or
> can I include it with the other patches?

Your preference.

Just a reminder: the dlc lock will still need to be dropped after bumping the
rfcomm_dev ref count via rfcomm_dev_get(), because when you subsequently drop
both references, rfcomm_dev destruction will attempt to gain the dlc lock,
resulting in deadlock. [Or do it as a work item]

Regards,
Peter Hurley

Re: [PATCH 3/8] Move device initialization and shutdown to tty_port_operations

[Gianluca Anzolin]

On Sun, Jul 21, 2013 at 01:04:03PM -0400, Peter Hurley wrote:
> On 07/21/2013 04:08 AM, Gianluca Anzolin wrote:
> >On Sat, Jul 20, 2013 at 10:11:50AM -0400, Peter Hurley wrote:
> >>Sorry Gianluca, I should have been more specific here.
> >>
> >>There's no need to test for dlc->state == BT_CLOSED in carrier_raised().
> >>At the point where port->carrier_raised is called, the tty will have been
> >>linked with the file descriptor, so if the dlc->state goes to BT_CLOSED,
> >>then rfcomm_dev_state_change() will call
> >>   tty_hangup() -> driver hangup() -> tty_port_hangup() -> tty_port_shutdown()
> >>This call chain will
> >>   1. set the file_ops to hung_up_tty_fops which will cause tty_hung_up_p() to
> >>      return true
> >>   2. clear ASYNCB_INITIALIZED in port->flags
> >>   3. wakeup port->open_wait
> >>
> >>So an open() parked in the schedule loop of tty_port_block_til_ready()
> >>will wake and exit the loop with either -EAGAIN or -ERESTARTSYS.
> >>
> >>rfcomm_dev_state_change() should only do a wakeup on port->open_wait when
> >>dlc->state == BT_CONNECTED.
> >>
> >>>In case of success I should also call some device_move,
> >>>rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in
> >>>carrier_raised directly?
> >>
> >>I wouldn't. That would be a nasty hack and a potential problem if a
> >>signal occurred.
> >>
> >>The device_move() isn't dependent on success, and can stay in .activate().
> >
> >I changed the code and it's cleaner than before, very nice. However the
> >device_move() is really dependent on success: the parent device is there only
> >when the connection has been successfully established.
> 
> Oh, right.
> 
> >So I have to call that function after the carrier is raised, or right before.
> >Since you already told me that calling it in the .carrier_raised method is
> >unwise the only place left is the state_change callback of the dlc.
> 
> That seems fine. [ Another place would be in rfcomm_tty_open() just after
> tty_port_open() returns success -- the tty is still locked here so it won't
> race with .close/.hangup() ]

I will do it in state_change because open could be called multiple times by
several user space programs.

> 
> I do wonder why the tty device is re-parented to the host controller device.
> It's obviously not for subsystem teardown. Maybe one of the bluetooth
> maintainers could clarify this? Are there userspace components waiting for
> this uevent?
> 
> >Conversely in .shutdown the check for the device parent == NULL takes care of
> >the scenario in which the aforementioned device_move() is never called.
> >
> >Another unrelated question: I'm working on the rfcomm_dev_add function to avoid
> >the two nested locks. When the patch is ready should I send it separately or
> >can I include it with the other patches?
> 
> Your preference.
> 
> Just a reminder: the dlc lock will still need to be dropped after bumping the
> rfcomm_dev ref count via rfcomm_dev_get(), because when you subsequently drop
> both references, rfcomm_dev destruction will attempt to gain the dlc lock,
> resulting in deadlock. [Or do it as a work item]

Yes I noticed that and before the last tty_port_put I will unlock the dlc.

> 
> Regards,
> Peter Hurley
> 

Thanks,

Gianluca