diff mbox series

[v4] Bluetooth: schedule SCO timeouts with delayed_work

Message ID 20210728071721.411669-1-desmondcheongzx@gmail.com
State New
Headers show
Series [v4] Bluetooth: schedule SCO timeouts with delayed_work | expand

Commit Message

Desmond Cheong Zhi Xi July 28, 2021, 7:17 a.m. UTC
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, sco_conn_ready, rfcomm_connect_ind, and
bt_accept_enqueue also 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.

Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---

Hi,

As suggested, this patch addresses the inconsistent lock state while
avoiding having to deal with local_bh_disable.

Now that sco_sock_timeout is no longer run in IRQ context, it might
be the case that bh_lock_sock is no longer needed to sync between
SOFTIRQ and user contexts, so we can switch to lock_sock.

I'm not too certain about this, or if there's any benefit to using
lock_sock instead, so I've left that out of this patch.

v3 -> v4:
- Switch to using delayed_work to schedule SCO sock timeouts instead
of using local_bh_disable. As suggested by Luiz Augusto von Dentz.

v2 -> v3:
- Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von
Dentz.
- Simplify local bh disabling in SCO by using local_bh_disable/enable
inside sco_chan_del since local_bh_disable/enable pairs are reentrant.

v1 -> v2:
- Instead of pulling out the clean-up code out from sco_chan_del and
using it directly in sco_conn_del, disable local softirqs for relevant
sections.
- Disable local softirqs more thoroughly for instances of
bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.
Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made
with local softirqs disabled as well.

Best wishes,
Desmond

 net/bluetooth/sco.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

Comments

bluez.test.bot@gmail.com July 28, 2021, 8:01 a.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=522485

---Test result---

Test Summary:
CheckPatch                    PASS      0.46 seconds
GitLint                       FAIL      0.10 seconds
BuildKernel                   PASS      508.96 seconds
TestRunner: Setup             PASS      334.21 seconds
TestRunner: l2cap-tester      PASS      2.55 seconds
TestRunner: bnep-tester       PASS      1.88 seconds
TestRunner: mgmt-tester       PASS      30.81 seconds
TestRunner: rfcomm-tester     PASS      2.03 seconds
TestRunner: sco-tester        PASS      2.00 seconds
TestRunner: smp-tester        FAIL      2.08 seconds
TestRunner: userchan-tester   PASS      1.92 seconds

Details
##############################
Test: CheckPatch - PASS - 0.46 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - FAIL - 0.10 seconds
Run gitlint with rule in .gitlint
Bluetooth: schedule SCO timeouts with delayed_work
24: B1 Line exceeds max length (87>80): "Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]"


##############################
Test: BuildKernel - PASS - 508.96 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 334.21 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.55 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.88 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 30.81 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.03 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.00 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.08 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.020 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.92 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth
Desmond Cheong Zhi Xi July 29, 2021, 4:28 a.m. UTC | #2
Hi Luiz,

On 29/7/21 7:07 am, Luiz Augusto von Dentz wrote:
> Hi Desmond,

> 

> On Wed, Jul 28, 2021 at 12:17 AM Desmond Cheong Zhi Xi

> <desmondcheongzx@gmail.com> wrote:

>>

>> 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, sco_conn_ready, rfcomm_connect_ind, and

>> bt_accept_enqueue also 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.

>>

>> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]

>> Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com

>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

>> ---

>>

>> Hi,

>>

>> As suggested, this patch addresses the inconsistent lock state while

>> avoiding having to deal with local_bh_disable.

>>

>> Now that sco_sock_timeout is no longer run in IRQ context, it might

>> be the case that bh_lock_sock is no longer needed to sync between

>> SOFTIRQ and user contexts, so we can switch to lock_sock.

>>

>> I'm not too certain about this, or if there's any benefit to using

>> lock_sock instead, so I've left that out of this patch.

>>

>> v3 -> v4:

>> - Switch to using delayed_work to schedule SCO sock timeouts instead

>> of using local_bh_disable. As suggested by Luiz Augusto von Dentz.

>>

>> v2 -> v3:

>> - Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von

>> Dentz.

>> - Simplify local bh disabling in SCO by using local_bh_disable/enable

>> inside sco_chan_del since local_bh_disable/enable pairs are reentrant.

>>

>> v1 -> v2:

>> - Instead of pulling out the clean-up code out from sco_chan_del and

>> using it directly in sco_conn_del, disable local softirqs for relevant

>> sections.

>> - Disable local softirqs more thoroughly for instances of

>> bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.

>> Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made

>> with local softirqs disabled as well.

>>

>> Best wishes,

>> Desmond

>>

>>   net/bluetooth/sco.c | 39 ++++++++++++++++++++++++---------------

>>   1 file changed, 24 insertions(+), 15 deletions(-)

>>

>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c

>> index 3bd41563f118..b6dd16153d38 100644

>> --- a/net/bluetooth/sco.c

>> +++ b/net/bluetooth/sco.c

>> @@ -48,6 +48,8 @@ struct sco_conn {

>>          spinlock_t      lock;

>>          struct sock     *sk;

>>

>> +       struct delayed_work     sk_timer;

>> +

>>          unsigned int    mtu;

>>   };

>>

>> @@ -74,9 +76,11 @@ 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,

>> +                                            sk_timer.work);

>> +       struct sock *sk = conn->sk;

>>

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

>>

>> @@ -89,16 +93,18 @@ static void sco_sock_timeout(struct timer_list *t)

>>          sock_put(sk);

>>   }

>>

>> -static void sco_sock_set_timer(struct sock *sk, long timeout)

>> +static void sco_sock_set_timer(struct sock *sk, struct delayed_work *work,

>> +                              long timeout)

>>   {

>>          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(work);

>> +       schedule_delayed_work(work, timeout);

> 

> I guess if you want to really guarantee cancel takes effect you must

> call cancel_delayed_work_sync

> 


Got it, thanks for catching that.

>>   }

>>

>> -static void sco_sock_clear_timer(struct sock *sk)

>> +static void sco_sock_clear_timer(struct sock *sk, struct delayed_work *work)

>>   {

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

>> -       sk_stop_timer(sk, &sk->sk_timer);

>> +       cancel_delayed_work(work);

>>   }

>>

>>   /* ---- SCO connections ---- */

>> @@ -174,7 +180,7 @@ static void sco_conn_del(struct hci_conn *hcon, int err)

>>          if (sk) {

>>                  sock_hold(sk);

>>                  bh_lock_sock(sk);

>> -               sco_sock_clear_timer(sk);

>> +               sco_sock_clear_timer(sk, &conn->sk_timer);

>>                  sco_chan_del(sk, err);

>>                  bh_unlock_sock(sk);

>>                  sco_sock_kill(sk);

>> @@ -193,6 +199,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->sk_timer, sco_sock_timeout);

>> +

>>          if (parent)

>>                  bt_accept_enqueue(parent, sk, true);

>>   }

>> @@ -260,11 +268,11 @@ static int sco_connect(struct sock *sk)

>>                  goto done;

>>

>>          if (hcon->state == BT_CONNECTED) {

>> -               sco_sock_clear_timer(sk);

>> +               sco_sock_clear_timer(sk, &conn->sk_timer);

>>                  sk->sk_state = BT_CONNECTED;

>>          } else {

>>                  sk->sk_state = BT_CONNECT;

>> -               sco_sock_set_timer(sk, sk->sk_sndtimeo);

>> +               sco_sock_set_timer(sk, &conn->sk_timer, sk->sk_sndtimeo);

>>          }

>>

>>   done:

>> @@ -419,7 +427,8 @@ static void __sco_sock_close(struct sock *sk)

>>          case BT_CONFIG:

>>                  if (sco_pi(sk)->conn->hcon) {

>>                          sk->sk_state = BT_DISCONN;

>> -                       sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);

>> +                       sco_sock_set_timer(sk, &sco_pi(sk)->conn->sk_timer,

>> +                                          SCO_DISCONN_TIMEOUT);

>>                          sco_conn_lock(sco_pi(sk)->conn);

>>                          hci_conn_drop(sco_pi(sk)->conn->hcon);

>>                          sco_pi(sk)->conn->hcon = NULL;

>> @@ -443,7 +452,8 @@ static void __sco_sock_close(struct sock *sk)

>>   /* Must be called on unlocked socket. */

>>   static void sco_sock_close(struct sock *sk)

>>   {

>> -       sco_sock_clear_timer(sk);

>> +       if (sco_pi(sk)->conn)

>> +               sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

>>          lock_sock(sk);

>>          __sco_sock_close(sk);

>>          release_sock(sk);

>> @@ -500,8 +510,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;

>>   }

>> @@ -1036,7 +1044,8 @@ static int sco_sock_shutdown(struct socket *sock, int how)

>>

>>          if (!sk->sk_shutdown) {

>>                  sk->sk_shutdown = SHUTDOWN_MASK;

>> -               sco_sock_clear_timer(sk);

>> +               if (sco_pi(sk)->conn)

>> +                       sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

> 

> It probably makes it simpler if we can have the check for

> sco_pi(sk)->conn inside sco_sock_{clear,set}_timer, that way we don't

> need to keep checking like in the code above.

> 


Makes sense, I'll make the change.

Re: testing, this patch passes some local tests I set up to trigger the 
lockdep warning, but I'll run the updated patch through Syzbot again to 
double-check.

Best wishes,
Desmond

>>                  __sco_sock_close(sk);

>>

>>                  if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&

>> @@ -1083,7 +1092,7 @@ static void sco_conn_ready(struct sco_conn *conn)

>>          BT_DBG("conn %p", conn);

>>

>>          if (sk) {

>> -               sco_sock_clear_timer(sk);

>> +               sco_sock_clear_timer(sk, &conn->sk_timer);

>>                  bh_lock_sock(sk);

>>                  sk->sk_state = BT_CONNECTED;

>>                  sk->sk_state_change(sk);

>> --

>> 2.25.1

>>

> 

>
Marcel Holtmann July 29, 2021, 11:30 a.m. UTC | #3
Hi Desmond,

> 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, sco_conn_ready, rfcomm_connect_ind, and

> bt_accept_enqueue also 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.

> 

> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]

> Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com

> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

> ---

> 

> Hi,

> 

> As suggested, this patch addresses the inconsistent lock state while

> avoiding having to deal with local_bh_disable.

> 

> Now that sco_sock_timeout is no longer run in IRQ context, it might

> be the case that bh_lock_sock is no longer needed to sync between

> SOFTIRQ and user contexts, so we can switch to lock_sock.

> 

> I'm not too certain about this, or if there's any benefit to using

> lock_sock instead, so I've left that out of this patch.


I don’t see a reason why we can’t switch to lock_sock, but lets do that in a separate patch in case I missed something it is easier to revert.

> 

> v3 -> v4:

> - Switch to using delayed_work to schedule SCO sock timeouts instead

> of using local_bh_disable. As suggested by Luiz Augusto von Dentz.

> 

> v2 -> v3:

> - Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von

> Dentz.

> - Simplify local bh disabling in SCO by using local_bh_disable/enable

> inside sco_chan_del since local_bh_disable/enable pairs are reentrant.

> 

> v1 -> v2:

> - Instead of pulling out the clean-up code out from sco_chan_del and

> using it directly in sco_conn_del, disable local softirqs for relevant

> sections.

> - Disable local softirqs more thoroughly for instances of

> bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.

> Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made

> with local softirqs disabled as well.

> 

> Best wishes,

> Desmond

> 

> net/bluetooth/sco.c | 39 ++++++++++++++++++++++++---------------

> 1 file changed, 24 insertions(+), 15 deletions(-)

> 

> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c

> index 3bd41563f118..b6dd16153d38 100644

> --- a/net/bluetooth/sco.c

> +++ b/net/bluetooth/sco.c

> @@ -48,6 +48,8 @@ struct sco_conn {

> 	spinlock_t	lock;

> 	struct sock	*sk;

> 

> +	struct delayed_work	sk_timer;

> +


I don’t like the sk_timer name. That is confusing. Maybe better use timeout_work or to_work. The sk_* are really more struct sock fields (hence the sk->sk_xyz naming schema).

> 	unsigned int    mtu;

> };

> 

> @@ -74,9 +76,11 @@ 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,

> +					     sk_timer.work);

> +	struct sock *sk = conn->sk;

> 

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

> 

> @@ -89,16 +93,18 @@ static void sco_sock_timeout(struct timer_list *t)

> 	sock_put(sk);

> }

> 

> -static void sco_sock_set_timer(struct sock *sk, long timeout)

> +static void sco_sock_set_timer(struct sock *sk, struct delayed_work *work,

> +			       long timeout)

> {


I don’t get the extra variable here. Can we not just pass in struct hci_conn.


> 	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(work);

> +	schedule_delayed_work(work, timeout);

> }

> 

> -static void sco_sock_clear_timer(struct sock *sk)

> +static void sco_sock_clear_timer(struct sock *sk, struct delayed_work *work)

> {

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

> -	sk_stop_timer(sk, &sk->sk_timer);

> +	cancel_delayed_work(work);


Same as above, we pass in struct sock just for the debug message.

> }

> 

> /* ---- SCO connections ---- */

> @@ -174,7 +180,7 @@ static void sco_conn_del(struct hci_conn *hcon, int err)

> 	if (sk) {

> 		sock_hold(sk);

> 		bh_lock_sock(sk);

> -		sco_sock_clear_timer(sk);

> +		sco_sock_clear_timer(sk, &conn->sk_timer);

> 		sco_chan_del(sk, err);

> 		bh_unlock_sock(sk);

> 		sco_sock_kill(sk);

> @@ -193,6 +199,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->sk_timer, sco_sock_timeout);

> +

> 	if (parent)

> 		bt_accept_enqueue(parent, sk, true);

> }

> @@ -260,11 +268,11 @@ static int sco_connect(struct sock *sk)

> 		goto done;

> 

> 	if (hcon->state == BT_CONNECTED) {

> -		sco_sock_clear_timer(sk);

> +		sco_sock_clear_timer(sk, &conn->sk_timer);

> 		sk->sk_state = BT_CONNECTED;

> 	} else {

> 		sk->sk_state = BT_CONNECT;

> -		sco_sock_set_timer(sk, sk->sk_sndtimeo);

> +		sco_sock_set_timer(sk, &conn->sk_timer, sk->sk_sndtimeo);

> 	}

> 

> done:

> @@ -419,7 +427,8 @@ static void __sco_sock_close(struct sock *sk)

> 	case BT_CONFIG:

> 		if (sco_pi(sk)->conn->hcon) {

> 			sk->sk_state = BT_DISCONN;

> -			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);

> +			sco_sock_set_timer(sk, &sco_pi(sk)->conn->sk_timer,

> +					   SCO_DISCONN_TIMEOUT);

> 			sco_conn_lock(sco_pi(sk)->conn);

> 			hci_conn_drop(sco_pi(sk)->conn->hcon);

> 			sco_pi(sk)->conn->hcon = NULL;

> @@ -443,7 +452,8 @@ static void __sco_sock_close(struct sock *sk)

> /* Must be called on unlocked socket. */

> static void sco_sock_close(struct sock *sk)

> {

> -	sco_sock_clear_timer(sk);

> +	if (sco_pi(sk)->conn)

> +		sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

> 	lock_sock(sk);

> 	__sco_sock_close(sk);

> 	release_sock(sk);

> @@ -500,8 +510,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;

> }

> @@ -1036,7 +1044,8 @@ static int sco_sock_shutdown(struct socket *sock, int how)

> 

> 	if (!sk->sk_shutdown) {

> 		sk->sk_shutdown = SHUTDOWN_MASK;

> -		sco_sock_clear_timer(sk);

> +		if (sco_pi(sk)->conn)

> +			sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

> 		__sco_sock_close(sk);

> 

> 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&

> @@ -1083,7 +1092,7 @@ static void sco_conn_ready(struct sco_conn *conn)

> 	BT_DBG("conn %p", conn);

> 

> 	if (sk) {

> -		sco_sock_clear_timer(sk);

> +		sco_sock_clear_timer(sk, &conn->sk_timer);

> 		bh_lock_sock(sk);

> 		sk->sk_state = BT_CONNECTED;

> 		sk->sk_state_change(sk);


Other than these minor cleanups, this looks great.

Regards

Marcel
Desmond Cheong Zhi Xi July 29, 2021, 2:02 p.m. UTC | #4
Hi Marcel,

On 29/7/21 7:30 pm, Marcel Holtmann wrote:
> Hi Desmond,

> 

>> 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, sco_conn_ready, rfcomm_connect_ind, and

>> bt_accept_enqueue also 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.

>>

>> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]

>> Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com

>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

>> ---

>>

>> Hi,

>>

>> As suggested, this patch addresses the inconsistent lock state while

>> avoiding having to deal with local_bh_disable.

>>

>> Now that sco_sock_timeout is no longer run in IRQ context, it might

>> be the case that bh_lock_sock is no longer needed to sync between

>> SOFTIRQ and user contexts, so we can switch to lock_sock.

>>

>> I'm not too certain about this, or if there's any benefit to using

>> lock_sock instead, so I've left that out of this patch.

> 

> I don’t see a reason why we can’t switch to lock_sock, but lets do that in a separate patch in case I missed something it is easier to revert.

> 


Sounds good to me.

After further investigation, I believe the switch to lock_sock is needed 
to prevent calls to sco_sock_set_timer while we're trying to remove a 
connection or socket.

Right now _set_timer is called under lock_sock, whereas _clear_timer is 
sometimes called under lock_sock, sometimes under bh_lock_sock, and 
sometimes under no lock. It seems to me that there's potential races 
here. For example:

         CPU0                    CPU1
         ----                    ----
    lock_sock();
                                 bh_lock_sock();
                                 sco_sock_clear_timer();
    sco_sock_set_timer();
                                 sco_chan_del();

So calls to _clear_timer and _set_timer need to be consolidated under 
lock_sock.

But before that there's a circular lock dependency that's currently 
hidden. When changing bh_lock_sock to lock_sock in sco.c, we get a chain 
of sk_lock-AF_BLUETOOTH-BTPROTO_SCO --> &hdev->lock --> hci_cb_list_lock

Assuming that the proper lock hierarchy (from outer to inner) should be 
&hdev->lock --> hci_cb_list_lock --> sk_lock-AF_BLUETOOTH-BTPROTO_SCO,
then the inversion happens in sco_sock_connect where we call lock_sock 
before hci_dev_lock.

So probably this fix needs to happen in a series like so:
- schedule SCO timeouts with delayed_work (which removes the SOFTIRQ)
- break the circular dependency (which enables the switch to lock_sock)
- switch to lock_sock while moving calls to _clear_timer under the lock

Thoughts?

>>

>> v3 -> v4:

>> - Switch to using delayed_work to schedule SCO sock timeouts instead

>> of using local_bh_disable. As suggested by Luiz Augusto von Dentz.

>>

>> v2 -> v3:

>> - Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von

>> Dentz.

>> - Simplify local bh disabling in SCO by using local_bh_disable/enable

>> inside sco_chan_del since local_bh_disable/enable pairs are reentrant.

>>

>> v1 -> v2:

>> - Instead of pulling out the clean-up code out from sco_chan_del and

>> using it directly in sco_conn_del, disable local softirqs for relevant

>> sections.

>> - Disable local softirqs more thoroughly for instances of

>> bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.

>> Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made

>> with local softirqs disabled as well.

>>

>> Best wishes,

>> Desmond

>>

>> net/bluetooth/sco.c | 39 ++++++++++++++++++++++++---------------

>> 1 file changed, 24 insertions(+), 15 deletions(-)

>>

>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c

>> index 3bd41563f118..b6dd16153d38 100644

>> --- a/net/bluetooth/sco.c

>> +++ b/net/bluetooth/sco.c

>> @@ -48,6 +48,8 @@ struct sco_conn {

>> 	spinlock_t	lock;

>> 	struct sock	*sk;

>>

>> +	struct delayed_work	sk_timer;

>> +

> 

> I don’t like the sk_timer name. That is confusing. Maybe better use timeout_work or to_work. The sk_* are really more struct sock fields (hence the sk->sk_xyz naming schema).

> 


Thanks for the feedback. timeout_work sounds good to me, I'll make the 
update.

>> 	unsigned int    mtu;

>> };

>>

>> @@ -74,9 +76,11 @@ 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,

>> +					     sk_timer.work);

>> +	struct sock *sk = conn->sk;

>>

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

>>

>> @@ -89,16 +93,18 @@ static void sco_sock_timeout(struct timer_list *t)

>> 	sock_put(sk);

>> }

>>

>> -static void sco_sock_set_timer(struct sock *sk, long timeout)

>> +static void sco_sock_set_timer(struct sock *sk, struct delayed_work *work,

>> +			       long timeout)

>> {

> 

> I don’t get the extra variable here. Can we not just pass in struct hci_conn.

> 

> 


Right, the extra variable isn't needed.

I think either struct hci_conn or struct sock should go in there. But as 
Luiz suggested in another email, perhaps struct sock would be a better 
candidate.

This is because sometimes we need to check whether sock has been added 
to a connection before calling sco_sock_clear_timer, e.g. in 
sco_sock_shutdown or sco_sock_close. So might as well consolidate all 
the checks and dereferences into sco_sock_{set/clear}_timer.

>> 	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(work);

>> +	schedule_delayed_work(work, timeout);

>> }

>>

>> -static void sco_sock_clear_timer(struct sock *sk)

>> +static void sco_sock_clear_timer(struct sock *sk, struct delayed_work *work)

>> {

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

>> -	sk_stop_timer(sk, &sk->sk_timer);

>> +	cancel_delayed_work(work);

> 

> Same as above, we pass in struct sock just for the debug message.

> 

>> }

>>

>> /* ---- SCO connections ---- */

>> @@ -174,7 +180,7 @@ static void sco_conn_del(struct hci_conn *hcon, int err)

>> 	if (sk) {

>> 		sock_hold(sk);

>> 		bh_lock_sock(sk);

>> -		sco_sock_clear_timer(sk);

>> +		sco_sock_clear_timer(sk, &conn->sk_timer);

>> 		sco_chan_del(sk, err);

>> 		bh_unlock_sock(sk);

>> 		sco_sock_kill(sk);

>> @@ -193,6 +199,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->sk_timer, sco_sock_timeout);

>> +

>> 	if (parent)

>> 		bt_accept_enqueue(parent, sk, true);

>> }

>> @@ -260,11 +268,11 @@ static int sco_connect(struct sock *sk)

>> 		goto done;

>>

>> 	if (hcon->state == BT_CONNECTED) {

>> -		sco_sock_clear_timer(sk);

>> +		sco_sock_clear_timer(sk, &conn->sk_timer);

>> 		sk->sk_state = BT_CONNECTED;

>> 	} else {

>> 		sk->sk_state = BT_CONNECT;

>> -		sco_sock_set_timer(sk, sk->sk_sndtimeo);

>> +		sco_sock_set_timer(sk, &conn->sk_timer, sk->sk_sndtimeo);

>> 	}

>>

>> done:

>> @@ -419,7 +427,8 @@ static void __sco_sock_close(struct sock *sk)

>> 	case BT_CONFIG:

>> 		if (sco_pi(sk)->conn->hcon) {

>> 			sk->sk_state = BT_DISCONN;

>> -			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);

>> +			sco_sock_set_timer(sk, &sco_pi(sk)->conn->sk_timer,

>> +					   SCO_DISCONN_TIMEOUT);

>> 			sco_conn_lock(sco_pi(sk)->conn);

>> 			hci_conn_drop(sco_pi(sk)->conn->hcon);

>> 			sco_pi(sk)->conn->hcon = NULL;

>> @@ -443,7 +452,8 @@ static void __sco_sock_close(struct sock *sk)

>> /* Must be called on unlocked socket. */

>> static void sco_sock_close(struct sock *sk)

>> {

>> -	sco_sock_clear_timer(sk);

>> +	if (sco_pi(sk)->conn)

>> +		sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

>> 	lock_sock(sk);

>> 	__sco_sock_close(sk);

>> 	release_sock(sk);

>> @@ -500,8 +510,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;

>> }

>> @@ -1036,7 +1044,8 @@ static int sco_sock_shutdown(struct socket *sock, int how)

>>

>> 	if (!sk->sk_shutdown) {

>> 		sk->sk_shutdown = SHUTDOWN_MASK;

>> -		sco_sock_clear_timer(sk);

>> +		if (sco_pi(sk)->conn)

>> +			sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);

>> 		__sco_sock_close(sk);

>>

>> 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&

>> @@ -1083,7 +1092,7 @@ static void sco_conn_ready(struct sco_conn *conn)

>> 	BT_DBG("conn %p", conn);

>>

>> 	if (sk) {

>> -		sco_sock_clear_timer(sk);

>> +		sco_sock_clear_timer(sk, &conn->sk_timer);

>> 		bh_lock_sock(sk);

>> 		sk->sk_state = BT_CONNECTED;

>> 		sk->sk_state_change(sk);

> 

> Other than these minor cleanups, this looks great.

> 

> Regards

> 

> Marcel

>
diff mbox series

Patch

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 3bd41563f118..b6dd16153d38 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -48,6 +48,8 @@  struct sco_conn {
 	spinlock_t	lock;
 	struct sock	*sk;
 
+	struct delayed_work	sk_timer;
+
 	unsigned int    mtu;
 };
 
@@ -74,9 +76,11 @@  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,
+					     sk_timer.work);
+	struct sock *sk = conn->sk;
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
@@ -89,16 +93,18 @@  static void sco_sock_timeout(struct timer_list *t)
 	sock_put(sk);
 }
 
-static void sco_sock_set_timer(struct sock *sk, long timeout)
+static void sco_sock_set_timer(struct sock *sk, struct delayed_work *work,
+			       long timeout)
 {
 	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(work);
+	schedule_delayed_work(work, timeout);
 }
 
-static void sco_sock_clear_timer(struct sock *sk)
+static void sco_sock_clear_timer(struct sock *sk, struct delayed_work *work)
 {
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
-	sk_stop_timer(sk, &sk->sk_timer);
+	cancel_delayed_work(work);
 }
 
 /* ---- SCO connections ---- */
@@ -174,7 +180,7 @@  static void sco_conn_del(struct hci_conn *hcon, int err)
 	if (sk) {
 		sock_hold(sk);
 		bh_lock_sock(sk);
-		sco_sock_clear_timer(sk);
+		sco_sock_clear_timer(sk, &conn->sk_timer);
 		sco_chan_del(sk, err);
 		bh_unlock_sock(sk);
 		sco_sock_kill(sk);
@@ -193,6 +199,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->sk_timer, sco_sock_timeout);
+
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
 }
@@ -260,11 +268,11 @@  static int sco_connect(struct sock *sk)
 		goto done;
 
 	if (hcon->state == BT_CONNECTED) {
-		sco_sock_clear_timer(sk);
+		sco_sock_clear_timer(sk, &conn->sk_timer);
 		sk->sk_state = BT_CONNECTED;
 	} else {
 		sk->sk_state = BT_CONNECT;
-		sco_sock_set_timer(sk, sk->sk_sndtimeo);
+		sco_sock_set_timer(sk, &conn->sk_timer, sk->sk_sndtimeo);
 	}
 
 done:
@@ -419,7 +427,8 @@  static void __sco_sock_close(struct sock *sk)
 	case BT_CONFIG:
 		if (sco_pi(sk)->conn->hcon) {
 			sk->sk_state = BT_DISCONN;
-			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
+			sco_sock_set_timer(sk, &sco_pi(sk)->conn->sk_timer,
+					   SCO_DISCONN_TIMEOUT);
 			sco_conn_lock(sco_pi(sk)->conn);
 			hci_conn_drop(sco_pi(sk)->conn->hcon);
 			sco_pi(sk)->conn->hcon = NULL;
@@ -443,7 +452,8 @@  static void __sco_sock_close(struct sock *sk)
 /* Must be called on unlocked socket. */
 static void sco_sock_close(struct sock *sk)
 {
-	sco_sock_clear_timer(sk);
+	if (sco_pi(sk)->conn)
+		sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);
 	lock_sock(sk);
 	__sco_sock_close(sk);
 	release_sock(sk);
@@ -500,8 +510,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;
 }
@@ -1036,7 +1044,8 @@  static int sco_sock_shutdown(struct socket *sock, int how)
 
 	if (!sk->sk_shutdown) {
 		sk->sk_shutdown = SHUTDOWN_MASK;
-		sco_sock_clear_timer(sk);
+		if (sco_pi(sk)->conn)
+			sco_sock_clear_timer(sk, &sco_pi(sk)->conn->sk_timer);
 		__sco_sock_close(sk);
 
 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
@@ -1083,7 +1092,7 @@  static void sco_conn_ready(struct sco_conn *conn)
 	BT_DBG("conn %p", conn);
 
 	if (sk) {
-		sco_sock_clear_timer(sk);
+		sco_sock_clear_timer(sk, &conn->sk_timer);
 		bh_lock_sock(sk);
 		sk->sk_state = BT_CONNECTED;
 		sk->sk_state_change(sk);