Re: [syzbot] general protection fault in l2cap_chan_timeout (2)

[Luiz Augusto von Dentz]

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