Skip to content

Commit

Permalink
Bluetooth: schedule SCO timeouts with delayed_work
Browse files Browse the repository at this point in the history
struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.

This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.

This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
       CPU0
       ----
  lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
  <Interrupt>
    lock(slock-AF_BLUETOOTH-BTPROTO_SCO);

To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.

As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.

Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.

Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
  • Loading branch information
desmondcheongzx authored and Vudentz committed Aug 10, 2021
1 parent 146af22 commit ba316be
Showing 1 changed file with 29 additions and 6 deletions.
35 changes: 29 additions & 6 deletions net/bluetooth/sco.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ struct sco_conn {
spinlock_t lock;
struct sock *sk;

struct delayed_work timeout_work;

unsigned int mtu;
};

Expand All @@ -74,9 +76,20 @@ struct sco_pinfo {
#define SCO_CONN_TIMEOUT (HZ * 40)
#define SCO_DISCONN_TIMEOUT (HZ * 2)

static void sco_sock_timeout(struct timer_list *t)
static void sco_sock_timeout(struct work_struct *work)
{
struct sock *sk = from_timer(sk, t, sk_timer);
struct sco_conn *conn = container_of(work, struct sco_conn,
timeout_work.work);
struct sock *sk;

sco_conn_lock(conn);
sk = conn->sk;
if (sk)
sock_hold(sk);
sco_conn_unlock(conn);

if (!sk)
return;

BT_DBG("sock %p state %d", sk, sk->sk_state);

Expand All @@ -91,14 +104,21 @@ static void sco_sock_timeout(struct timer_list *t)

static void sco_sock_set_timer(struct sock *sk, long timeout)
{
if (!sco_pi(sk)->conn)
return;

BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
schedule_delayed_work(&sco_pi(sk)->conn->timeout_work, timeout);
}

static void sco_sock_clear_timer(struct sock *sk)
{
if (!sco_pi(sk)->conn)
return;

BT_DBG("sock %p state %d", sk, sk->sk_state);
sk_stop_timer(sk, &sk->sk_timer);
cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
}

/* ---- SCO connections ---- */
Expand Down Expand Up @@ -179,6 +199,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
bh_unlock_sock(sk);
sco_sock_kill(sk);
sock_put(sk);

/* Ensure no more work items will run before freeing conn. */
cancel_delayed_work_sync(&conn->timeout_work);
}

hcon->sco_data = NULL;
Expand All @@ -193,6 +216,8 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
sco_pi(sk)->conn = conn;
conn->sk = sk;

INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);

if (parent)
bt_accept_enqueue(parent, sk, true);
}
Expand Down Expand Up @@ -500,8 +525,6 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,

sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;

timer_setup(&sk->sk_timer, sco_sock_timeout, 0);

bt_sock_link(&sco_sk_list, sk);
return sk;
}
Expand Down

0 comments on commit ba316be

Please sign in to comment.