diff mbox series

Bluetooth: Acquire sk_lock.slock without disabling interrupts

Message ID 20200527193919.1655228-1-bigeasy@linutronix.de (mailing list archive)
State New, archived
Delegated to: Marcel Holtmann
Headers show
Series Bluetooth: Acquire sk_lock.slock without disabling interrupts | expand

Commit Message

Sebastian Andrzej Siewior May 27, 2020, 7:39 p.m. UTC 
There was a lockdep which led to commit
   fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with RFCOMM")

Lockdep noticed that `sk->sk_lock.slock' was acquired without disabling
the softirq while the lock was also used in softirq context.
Unfortunately the solution back then was to disable interrupts before
acquiring the lock which however made lockdep happy.
It would have been enough to simply disable the softirq. Disabling
interrupts before acquiring a spinlock_t is not allowed on PREEMPT_RT
because these locks are converted to 'sleeping' spinlocks.

Use spin_lock_bh() in order to acquire the `sk_lock.slock'.

Reported-by: Luis Claudio R. Goncalves <lclaudio@uudg.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/bluetooth/rfcomm/sock.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

kernel test robot May 28, 2020, 7:11 a.m. UTC | #1 
Hi Sebastian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Andrzej-Siewior/Bluetooth-Acquire-sk_lock-slock-without-disabling-interrupts/20200528-034222
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: i386-randconfig-s001-20200528 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-240-gf0fe1cd9-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> net/bluetooth/rfcomm/sock.c:64:13: sparse: sparse: context imbalance in 'rfcomm_sk_state_change' - wrong count at exit

# https://github.com/0day-ci/linux/commit/dc48a377813eebe5a9e4818d98f51df0da9476fc
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout dc48a377813eebe5a9e4818d98f51df0da9476fc
vim +/rfcomm_sk_state_change +64 net/bluetooth/rfcomm/sock.c

^1da177e4c3f41 Linus Torvalds            2005-04-16   63  
^1da177e4c3f41 Linus Torvalds            2005-04-16  @64  static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
^1da177e4c3f41 Linus Torvalds            2005-04-16   65  {
^1da177e4c3f41 Linus Torvalds            2005-04-16   66  	struct sock *sk = d->owner, *parent;
fad003b6c8e3d9 Gustavo Padovan           2010-08-14   67  
^1da177e4c3f41 Linus Torvalds            2005-04-16   68  	if (!sk)
^1da177e4c3f41 Linus Torvalds            2005-04-16   69  		return;
^1da177e4c3f41 Linus Torvalds            2005-04-16   70  
^1da177e4c3f41 Linus Torvalds            2005-04-16   71  	BT_DBG("dlc %p state %ld err %d", d, d->state, err);
^1da177e4c3f41 Linus Torvalds            2005-04-16   72  
dc48a377813eeb Sebastian Andrzej Siewior 2020-05-27   73  	spin_lock_bh(&sk->sk_lock.slock);
^1da177e4c3f41 Linus Torvalds            2005-04-16   74  
^1da177e4c3f41 Linus Torvalds            2005-04-16   75  	if (err)
^1da177e4c3f41 Linus Torvalds            2005-04-16   76  		sk->sk_err = err;
^1da177e4c3f41 Linus Torvalds            2005-04-16   77  
^1da177e4c3f41 Linus Torvalds            2005-04-16   78  	sk->sk_state = d->state;
^1da177e4c3f41 Linus Torvalds            2005-04-16   79  
^1da177e4c3f41 Linus Torvalds            2005-04-16   80  	parent = bt_sk(sk)->parent;
^1da177e4c3f41 Linus Torvalds            2005-04-16   81  	if (parent) {
^1da177e4c3f41 Linus Torvalds            2005-04-16   82  		if (d->state == BT_CLOSED) {
^1da177e4c3f41 Linus Torvalds            2005-04-16   83  			sock_set_flag(sk, SOCK_ZAPPED);
^1da177e4c3f41 Linus Torvalds            2005-04-16   84  			bt_accept_unlink(sk);
^1da177e4c3f41 Linus Torvalds            2005-04-16   85  		}
676d23690fb62b David S. Miller           2014-04-11   86  		parent->sk_data_ready(parent);
^1da177e4c3f41 Linus Torvalds            2005-04-16   87  	} else {
^1da177e4c3f41 Linus Torvalds            2005-04-16   88  		if (d->state == BT_CONNECTED)
94a86df0108255 Marcel Holtmann           2013-10-13   89  			rfcomm_session_getaddr(d->session,
94a86df0108255 Marcel Holtmann           2013-10-13   90  					       &rfcomm_pi(sk)->src, NULL);
^1da177e4c3f41 Linus Torvalds            2005-04-16   91  		sk->sk_state_change(sk);
^1da177e4c3f41 Linus Torvalds            2005-04-16   92  	}
^1da177e4c3f41 Linus Torvalds            2005-04-16   93  
dc48a377813eeb Sebastian Andrzej Siewior 2020-05-27   94  	spin_lock_bh(&sk->sk_lock.slock);
^1da177e4c3f41 Linus Torvalds            2005-04-16   95  
^1da177e4c3f41 Linus Torvalds            2005-04-16   96  	if (parent && sock_flag(sk, SOCK_ZAPPED)) {
^1da177e4c3f41 Linus Torvalds            2005-04-16   97  		/* We have to drop DLC lock here, otherwise
^1da177e4c3f41 Linus Torvalds            2005-04-16   98  		 * rfcomm_sock_destruct() will dead lock. */
^1da177e4c3f41 Linus Torvalds            2005-04-16   99  		rfcomm_dlc_unlock(d);
^1da177e4c3f41 Linus Torvalds            2005-04-16  100  		rfcomm_sock_kill(sk);
^1da177e4c3f41 Linus Torvalds            2005-04-16  101  		rfcomm_dlc_lock(d);
^1da177e4c3f41 Linus Torvalds            2005-04-16  102  	}
^1da177e4c3f41 Linus Torvalds            2005-04-16  103  }
^1da177e4c3f41 Linus Torvalds            2005-04-16  104  

:::::: The code at line 64 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index b4eaf21360ef2..ad6ba6ce23436 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -64,15 +64,13 @@  static void rfcomm_sk_data_ready(struct rfcomm_dlc *d, struct sk_buff *skb)
 static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 {
 	struct sock *sk = d->owner, *parent;
-	unsigned long flags;
 
 	if (!sk)
 		return;
 
 	BT_DBG("dlc %p state %ld err %d", d, d->state, err);
 
-	local_irq_save(flags);
-	bh_lock_sock(sk);
+	spin_lock_bh(&sk->sk_lock.slock);
 
 	if (err)
 		sk->sk_err = err;
@@ -93,8 +91,7 @@  static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 		sk->sk_state_change(sk);
 	}
 
-	bh_unlock_sock(sk);
-	local_irq_restore(flags);
+	spin_lock_bh(&sk->sk_lock.slock);
 
 	if (parent && sock_flag(sk, SOCK_ZAPPED)) {
 		/* We have to drop DLC lock here, otherwise