Hi,
On Tue, Jun 1, 2021 at 12:54 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Mon, 31 May 2021 19:11:08 -0700 Luiz Augusto von Dentz wrote:
> >
> >Shouldn't we actually cancel the timeout work if the connection is
> >freed here? At least I don't see a valid reason to have a l2cap_chan
> >without l2cap_conn.
>
> A far neater approach at the cost of making l2cap_conn_put() blocking and
> nobody currently seems to care about it.
I wonder what is going on here, there doesn't seem to be any code path
where the chan_timer is not cleared since the code path should be:
l2cap_conn_del -> l2cap_chan_del -> __clear_chan_timer -> cancel_delayed_work
chan->conn = NULL
Perhaps the problem is that cancel_delayed_work does not actually
prevent l2cap_chan_timeout to run if that is already pending, so maybe
something like this would work:
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9ebb85df4db4..f6e423111dfc 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -414,17 +414,23 @@ static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
chan_timer.work);
- struct l2cap_conn *conn = chan->conn;
+ struct l2cap_conn *conn;
int reason;
BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
- mutex_lock(&conn->chan_lock);
/* __set_chan_timer() calls l2cap_chan_hold(chan) while scheduling
* this work. No need to call l2cap_chan_hold(chan) here again.
*/
l2cap_chan_lock(chan);
+ conn = chan->conn;
+ if (!conn)
+ /* l2cap_conn_del might have run */
+ goto unlock;
+
+ mutex_lock(&conn->chan_lock);
+
if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
reason = ECONNREFUSED;
else if (chan->state == BT_CONNECT &&
@@ -437,10 +443,11 @@ static void l2cap_chan_timeout(struct work_struct *work)
chan->ops->close(chan);
+ mutex_unlock(&conn->chan_lock);
+
+unlock:
l2cap_chan_unlock(chan);
l2cap_chan_put(chan);
-
- mutex_unlock(&conn->chan_lock);
}
--
Luiz Augusto von Dentz
