Return-path: Received: from mail-iy0-f174.google.com ([209.85.210.174]:46158 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750978Ab1BVBPs convert rfc822-to-8bit (ORCPT ); Mon, 21 Feb 2011 20:15:48 -0500 MIME-Version: 1.0 In-Reply-To: <1298305510.3836.7.camel@jlt3.sipsolutions.net> References: <1298305510.3836.7.camel@jlt3.sipsolutions.net> Date: Tue, 22 Feb 2011 09:15:47 +0800 Message-ID: Subject: Re: [2.6.38-rc5, patch] fix cfg80211_wext_siwfreq lock ordering... From: Daniel J Blueman To: stable@kernel.org Cc: linux-wireless@vger.kernel.org, Linux Kernel , Johannes Berg Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 22 February 2011 00:25, Johannes Berg wrote: > On Tue, 2011-02-22 at 00:11 +0800, Daniel J Blueman wrote: >> I previously managed to reproduce a hang while scanning wireless >> channels (reproducible with airodump-ng hopping channels); subsequent >> lockdep instrumentation revealed a lock ordering issue [1]. >> >> Without knowing the design intent, it looks like the locks should be >> taken in reverse order; please comment. >> >> Signed-off-by: Daniel J Blueman >> >> --- [1] > > Yeah, looks this way, thanks. > > Acked-by: Johannes Berg > >> ======================================================= >> [ INFO: possible circular locking dependency detected ] >> 2.6.38-rc5-341cd #4 >> ------------------------------------------------------- >> airodump-ng/15445 is trying to acquire lock: >> ?(&rdev->devlist_mtx){+.+.+.}, at: [] >> cfg80211_wext_siwfreq+0xc6/0x100 >> >> but task is already holding lock: >> ?(&wdev->mtx){+.+.+.}, at: [] cfg80211_wext_siwfreq+0xbc/0x100 >> >> which lock already depends on the new lock. >> >> the existing dependency chain (in reverse order) is: >> >> -> #1 (&wdev->mtx){+.+.+.}: >> ? ? ? ?[] lock_acquire+0xc6/0x280 >> ? ? ? ?[] mutex_lock_nested+0x6e/0x4b0 >> ? ? ? ?[] cfg80211_netdev_notifier_call+0x430/0x5f0 >> ? ? ? ?[] notifier_call_chain+0x8b/0x100 >> ? ? ? ?[] raw_notifier_call_chain+0x11/0x20 >> ? ? ? ?[] call_netdevice_notifiers+0x32/0x60 >> ? ? ? ?[] __dev_notify_flags+0x34/0x80 >> ? ? ? ?[] dev_change_flags+0x40/0x70 >> ? ? ? ?[] do_setlink+0x1fc/0x8d0 >> ? ? ? ?[] rtnl_setlink+0xf2/0x140 >> ? ? ? ?[] rtnetlink_rcv_msg+0x163/0x270 >> ? ? ? ?[] netlink_rcv_skb+0xa1/0xd0 >> ? ? ? ?[] rtnetlink_rcv+0x20/0x30 >> ? ? ? ?[] netlink_unicast+0x2ba/0x300 >> ? ? ? ?[] netlink_sendmsg+0x267/0x3e0 >> ? ? ? ?[] sock_sendmsg+0xe4/0x110 >> ? ? ? ?[] sys_sendmsg+0x253/0x3b0 >> ? ? ? ?[] system_call_fastpath+0x16/0x1b >> >> -> #0 (&rdev->devlist_mtx){+.+.+.}: >> ? ? ? ?[] __lock_acquire+0x1622/0x1d10 >> ? ? ? ?[] lock_acquire+0xc6/0x280 >> ? ? ? ?[] mutex_lock_nested+0x6e/0x4b0 >> ? ? ? ?[] cfg80211_wext_siwfreq+0xc6/0x100 >> ? ? ? ?[] ioctl_standard_call+0x5d/0xd0 >> ? ? ? ?[] T.808+0x163/0x170 >> ? ? ? ?[] wext_handle_ioctl+0x3a/0x90 >> ? ? ? ?[] dev_ioctl+0x6f2/0x830 >> ? ? ? ?[] sock_ioctl+0xfd/0x290 >> ? ? ? ?[] do_vfs_ioctl+0x9d/0x590 >> ? ? ? ?[] sys_ioctl+0x4a/0x80 >> ? ? ? ?[] system_call_fastpath+0x16/0x1b >> >> other info that might help us debug this: >> >> 2 locks held by airodump-ng/15445: >> ?#0: ?(rtnl_mutex){+.+.+.}, at: [] rtnl_lock+0x12/0x20 >> ?#1: ?(&wdev->mtx){+.+.+.}, at: [] >> cfg80211_wext_siwfreq+0xbc/0x100 >> >> stack backtrace: >> Pid: 15445, comm: airodump-ng Not tainted 2.6.38-rc5-341cd #4 >> Call Trace: >> ?[] ? print_circular_bug+0xfa/0x100 >> ?[] ? __lock_acquire+0x1622/0x1d10 >> ?[] ? trace_hardirqs_off_caller+0x29/0xc0 >> ?[] ? lock_acquire+0xc6/0x280 >> ?[] ? cfg80211_wext_siwfreq+0xc6/0x100 >> ?[] ? mark_held_locks+0x67/0x90 >> ?[] ? mutex_lock_nested+0x6e/0x4b0 >> ?[] ? cfg80211_wext_siwfreq+0xc6/0x100 >> ?[] ? mark_held_locks+0x67/0x90 >> ?[] ? cfg80211_wext_siwfreq+0xc6/0x100 >> ?[] ? cfg80211_wext_siwfreq+0xc6/0x100 >> ?[] ? ioctl_standard_call+0x5d/0xd0 >> ?[] ? __dev_get_by_name+0x9b/0xc0 >> ?[] ? ioctl_standard_call+0x0/0xd0 >> ?[] ? T.808+0x163/0x170 >> ?[] ? might_fault+0x72/0xd0 >> ?[] ? wext_handle_ioctl+0x3a/0x90 >> ?[] ? might_fault+0xbb/0xd0 >> ?[] ? dev_ioctl+0x6f2/0x830 >> ?[] ? put_lock_stats+0xe/0x40 >> ?[] ? lock_release_holdtime+0xac/0x150 >> ?[] ? sock_ioctl+0xfd/0x290 >> ?[] ? do_vfs_ioctl+0x9d/0x590 >> ?[] ? fget_light+0x1df/0x3c0 >> ?[] ? sys_ioctl+0x4a/0x80 >> ?[] ? system_call_fastpath+0x16/0x1b >> >> --- [2] >> >> diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c >> index 3e5dbd4..d112f03 100644 >> --- a/net/wireless/wext-compat.c >> +++ b/net/wireless/wext-compat.c >> @@ -802,11 +802,11 @@ int cfg80211_wext_siwfreq(struct net_device *dev, >> ? ? ? ? ? ? ? ? ? ? ? return freq; >> ? ? ? ? ? ? ? if (freq == 0) >> ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; >> - ? ? ? ? ? ? wdev_lock(wdev); >> ? ? ? ? ? ? ? mutex_lock(&rdev->devlist_mtx); >> + ? ? ? ? ? ? wdev_lock(wdev); >> ? ? ? ? ? ? ? err = cfg80211_set_freq(rdev, wdev, freq, NL80211_CHAN_NO_HT); >> - ? ? ? ? ? ? mutex_unlock(&rdev->devlist_mtx); >> ? ? ? ? ? ? ? wdev_unlock(wdev); >> + ? ? ? ? ? ? mutex_unlock(&rdev->devlist_mtx); >> ? ? ? ? ? ? ? return err; >> ? ? ? default: >> ? ? ? ? ? ? ? return -EOPNOTSUPP; Please consider for -stable. This patch resolves the lock ordering case my test exposed, and passes lockdep and extended testing. Thanks, Daniel -- Daniel J Blueman