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@xxxxxxxxxxxxxx>
---
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);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
