Re: [syzbot] general protection fault in l2cap_chan_timeout (2)
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: 2021-06-01 20:48:47
Also in:
lkml, netdev
Subsystem:
bluetooth subsystem, the rest · Maintainers:
Marcel Holtmann, Luiz Augusto von Dentz, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2021-06-01 · Re: [syzbot] general protection fault in l2cap_chan_timeout (2) · Luiz Augusto von Dentz <luiz.dentz@gmail.com>
- 2021-05-31 · [syzbot] general protection fault in l2cap_chan_timeout (2) · syzbot <hidden>
Hi, On Tue, Jun 1, 2021 at 12:54 AM Hillf Danton [off-list ref] wrote:
On Mon, 31 May 2021 19:11:08 -0700 Luiz Augusto von Dentz wrote:quoted
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