Received: by 2002:a05:6a10:1287:0:0:0:0 with SMTP id d7csp6018516pxv; Thu, 29 Jul 2021 04:32:52 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyT8/EyZB0QZk0qwyAKZ2PLoUhuklND41nKvcjR6ofyiZ45Y5N7Ji/yiBCAVqrbEl0RZm7o X-Received: by 2002:a17:906:94cb:: with SMTP id d11mr4240344ejy.17.1627558372556; Thu, 29 Jul 2021 04:32:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1627558372; cv=none; d=google.com; s=arc-20160816; b=LVCTa1cbOjsZ/mWO7IXfp8PcDdYptOeGXe9On4/qAopb3hUcZCZIqFeJ4z5HUAEu4w Cbh2aSNlOegiZQ/hEtNxOT0cXG0s56bmm5JUhj1EfsUXwx7A5g+8sokcY2HcT2fvixBx 6Qmf9Ixh+hL2Ot1bD1ZKzhcSJh2n0/fOl/KRh+yCEDE5JRifjFl7YLWmRvLZEtrwv0Eb AMRumGw08tD6cDbd0HY8XJG/ibR/atXJfBPMqpfJIE/ZqK7DNQalK+zQx2fiSKJqPetO YBoLJNs7WC1rOyLffXcHGp5Y46L03zzUySIAOB277WAo6Z4E2z3YEHYqRfieT1XIOjBA d49A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:to:references:message-id :content-transfer-encoding:cc:date:in-reply-to:from:subject :mime-version; bh=+IW3u7adL/vNYEHYdtE/cGjRwIRHXpIfRbhPPVR3Ixc=; b=mzZ48v8Vv520/NAvERvUvPA/efK9D6JaUvNh+1BRgg9SCkSRgF1VE7wfHi/CUg5ORu M8ifxyjQ2qUkDQnpVqnVbPsMekiiaj4R8PxrFlh8BCMSGIvskMZxgguekx2rQdkIrppF h8ZDFEY7FG3H8sR0ApJL58+gjJqqpTYZj9YPmdviLnXojUmWAMYZluMafCENddmJyq/N vbNEpW4ivz1puFO7su9zyozK0GI9On1xIon5vnebB+yvUBbTi7/+bvOUEp5F2IE0yo8Q zeSADu325Vg0/KiRW58yIWp9uALMgxnk8XQr5xRu/a0M7TG+aKvs8MoGMWq02BsnMBCu S2QQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id h15si2445653ejj.98.2021.07.29.04.32.29; Thu, 29 Jul 2021 04:32:52 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234737AbhG2LaX convert rfc822-to-8bit (ORCPT + 99 others); Thu, 29 Jul 2021 07:30:23 -0400 Received: from coyote.holtmann.net ([212.227.132.17]:35408 "EHLO mail.holtmann.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231837AbhG2LaU (ORCPT ); Thu, 29 Jul 2021 07:30:20 -0400 Received: from smtpclient.apple (p5b3d23f8.dip0.t-ipconnect.de [91.61.35.248]) by mail.holtmann.org (Postfix) with ESMTPSA id 5FDB1CED14; Thu, 29 Jul 2021 13:30:15 +0200 (CEST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\)) Subject: Re: [PATCH v4] Bluetooth: schedule SCO timeouts with delayed_work From: Marcel Holtmann In-Reply-To: <20210728071721.411669-1-desmondcheongzx@gmail.com> Date: Thu, 29 Jul 2021 13:30:14 +0200 Cc: Johan Hedberg , Luiz Augusto von Dentz , "David S. Miller" , Jakub Kicinski , linux-bluetooth , "open list:NETWORKING [GENERAL]" , open list , skhan@linuxfoundation.org, Greg Kroah-Hartman , linux-kernel-mentees@lists.linuxfoundation.org, syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com Content-Transfer-Encoding: 8BIT Message-Id: <565F72A4-F9B6-430F-A35D-8EAC7545C141@holtmann.org> References: <20210728071721.411669-1-desmondcheongzx@gmail.com> To: Desmond Cheong Zhi Xi X-Mailer: Apple Mail (2.3654.100.0.2.22) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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); > > 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 > --- > > 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