Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Joe Korty <joe.korty@concurrent-rt.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Bruce Fields <bfields@redhat.com>,
        Sebastian Siewior <bigeasy@linutronix.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        "linux-rt-users@vger.kernel.org" <linux-rt-users@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] Repair misuse of sv_lock in 5.10.16-rt30.
Thread-Topic: [PATCH] Repair misuse of sv_lock in 5.10.16-rt30.
Thread-Index: AQHXDFArNpUDpBnnO0SmlDmeu86JQ6pqi+GA
Date:   Fri, 26 Feb 2021 15:15:46 +0000
Message-ID: <AA751435-2DB2-467F-B0EC-133BE62A8581@oracle.com>
References: <20210226143820.GA49043@zipoli.concurrent-rt.com>
 <YDkNFKmzb7rbumYf@pick.fieldses.org>
In-Reply-To: <YDkNFKmzb7rbumYf@pick.fieldses.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <581026D233933544930285DB8594FD7D@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: dce97c76-539f-4470-0174-08d8da6964a0
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Feb 2021 15:15:46.5901
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3BpFfoFg6Wcm2bRDbHk5rYPaYiKT7hPQsy7aMLBcBvDja15QGhfkBQ5Q2hubLBOG2ngKppQH2vnc0t8DuONAoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2439
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9907 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 malwarescore=0 mlxlogscore=999 adultscore=0 bulkscore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2102260118
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9907 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 priorityscore=1501 impostorscore=0 bulkscore=0 mlxscore=0 malwarescore=0
 clxscore=1011 phishscore=0 mlxlogscore=999 lowpriorityscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2102260118
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 26, 2021, at 10:00 AM, J. Bruce Fields <bfields@redhat.com> wrote:
>=20
> Adding Chuck, linux-nfs.
>=20
> Makes sense to me.--b.

Joe, I can add this to nfsd-5.12-rc. Would it be appropriate to add:

Fixes: 719f8bcc883e ("svcrpc: fix xpt_list traversal locking on shutdown")


> On Fri, Feb 26, 2021 at 09:38:20AM -0500, Joe Korty wrote:
>> Repair misuse of sv_lock in 5.10.16-rt30.
>>=20
>> [ This problem is in mainline, but only rt has the chops to be
>> able to detect it. ]
>>=20
>> Lockdep reports a circular lock dependency between serv->sv_lock and
>> softirq_ctl.lock on system shutdown, when using a kernel built with
>> CONFIG_PREEMPT_RT=3Dy, and a nfs mount exists.
>>=20
>> This is due to the definition of spin_lock_bh on rt:
>>=20
>> 	local_bh_disable();
>> 	rt_spin_lock(lock);
>>=20
>> which forces a softirq_ctl.lock -> serv->sv_lock dependency.  This is
>> not a problem as long as _every_ lock of serv->sv_lock is a:
>>=20
>> 	spin_lock_bh(&serv->sv_lock);
>>=20
>> but there is one of the form:
>>=20
>> 	spin_lock(&serv->sv_lock);
>>=20
>> This is what is causing the circular dependency splat.  The spin_lock()
>> grabs the lock without first grabbing softirq_ctl.lock via local_bh_disa=
ble.
>> If later on in the critical region,  someone does a local_bh_disable, we
>> get a serv->sv_lock -> softirq_ctrl.lock dependency established.  Deadlo=
ck.
>>=20
>> Fix is to make serv->sv_lock be locked with spin_lock_bh everywhere, no
>> exceptions.
>>=20
>> Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>
>>=20
>>=20
>>=20
>>=20
>> [  OK  ] Stopped target NFS client services.
>>         Stopping Logout off all iSCSI sessions on shutdown...
>>         Stopping NFS server and services...
>> [  109.442380]=20
>> [  109.442385] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> [  109.442386] WARNING: possible circular locking dependency detected
>> [  109.442387] 5.10.16-rt30 #1 Not tainted
>> [  109.442389] ------------------------------------------------------
>> [  109.442390] nfsd/1032 is trying to acquire lock:
>> [  109.442392] ffff994237617f60 ((softirq_ctrl.lock).lock){+.+.}-{2:2}, =
at: __local_bh_disable_ip+0xd9/0x270
>> [  109.442405]=20
>> [  109.442405] but task is already holding lock:
>> [  109.442406] ffff994245cb00b0 (&serv->sv_lock){+.+.}-{0:0}, at: svc_cl=
ose_list+0x1f/0x90
>> [  109.442415]=20
>> [  109.442415] which lock already depends on the new lock.
>> [  109.442415]=20
>> [  109.442416]=20
>> [  109.442416] the existing dependency chain (in reverse order) is:
>> [  109.442417]=20
>> [  109.442417] -> #1 (&serv->sv_lock){+.+.}-{0:0}:
>> [  109.442421]        rt_spin_lock+0x2b/0xc0
>> [  109.442428]        svc_add_new_perm_xprt+0x42/0xa0
>> [  109.442430]        svc_addsock+0x135/0x220
>> [  109.442434]        write_ports+0x4b3/0x620
>> [  109.442438]        nfsctl_transaction_write+0x45/0x80
>> [  109.442440]        vfs_write+0xff/0x420
>> [  109.442444]        ksys_write+0x4f/0xc0
>> [  109.442446]        do_syscall_64+0x33/0x40
>> [  109.442450]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [  109.442454]=20
>> [  109.442454] -> #0 ((softirq_ctrl.lock).lock){+.+.}-{2:2}:
>> [  109.442457]        __lock_acquire+0x1264/0x20b0
>> [  109.442463]        lock_acquire+0xc2/0x400
>> [  109.442466]        rt_spin_lock+0x2b/0xc0
>> [  109.442469]        __local_bh_disable_ip+0xd9/0x270
>> [  109.442471]        svc_xprt_do_enqueue+0xc0/0x4d0
>> [  109.442474]        svc_close_list+0x60/0x90
>> [  109.442476]        svc_close_net+0x49/0x1a0
>> [  109.442478]        svc_shutdown_net+0x12/0x40
>> [  109.442480]        nfsd_destroy+0xc5/0x180
>> [  109.442482]        nfsd+0x1bc/0x270
>> [  109.442483]        kthread+0x194/0x1b0
>> [  109.442487]        ret_from_fork+0x22/0x30
>> [  109.442492]=20
>> [  109.442492] other info that might help us debug this:
>> [  109.442492]=20
>> [  109.442493]  Possible unsafe locking scenario:
>> [  109.442493]=20
>> [  109.442493]        CPU0                    CPU1
>> [  109.442494]        ----                    ----
>> [  109.442495]   lock(&serv->sv_lock);
>> [  109.442496]                                lock((softirq_ctrl.lock).l=
ock);
>> [  109.442498]                                lock(&serv->sv_lock);
>> [  109.442499]   lock((softirq_ctrl.lock).lock);
>> [  109.442501]=20
>> [  109.442501]  *** DEADLOCK ***
>> [  109.442501]=20
>> [  109.442501] 3 locks held by nfsd/1032:
>> [  109.442503]  #0: ffffffff93b49258 (nfsd_mutex){+.+.}-{3:3}, at: nfsd+=
0x19a/0x270
>> [  109.442508]  #1: ffff994245cb00b0 (&serv->sv_lock){+.+.}-{0:0}, at: s=
vc_close_list+0x1f/0x90
>> [  109.442512]  #2: ffffffff93a81b20 (rcu_read_lock){....}-{1:2}, at: rt=
_spin_lock+0x5/0xc0
>> [  109.442518]=20
>> [  109.442518] stack backtrace:
>> [  109.442519] CPU: 0 PID: 1032 Comm: nfsd Not tainted 5.10.16-rt30 #1
>> [  109.442522] Hardware name: Supermicro X9DRL-3F/iF/X9DRL-3F/iF, BIOS 3=
.2 09/22/2015
>> [  109.442524] Call Trace:
>> [  109.442527]  dump_stack+0x77/0x97
>> [  109.442533]  check_noncircular+0xdc/0xf0
>> [  109.442546]  __lock_acquire+0x1264/0x20b0
>> [  109.442553]  lock_acquire+0xc2/0x400
>> [  109.442564]  rt_spin_lock+0x2b/0xc0
>> [  109.442570]  __local_bh_disable_ip+0xd9/0x270
>> [  109.442573]  svc_xprt_do_enqueue+0xc0/0x4d0
>> [  109.442577]  svc_close_list+0x60/0x90
>> [  109.442581]  svc_close_net+0x49/0x1a0
>> [  109.442585]  svc_shutdown_net+0x12/0x40
>> [  109.442588]  nfsd_destroy+0xc5/0x180
>> [  109.442590]  nfsd+0x1bc/0x270
>> [  109.442595]  kthread+0x194/0x1b0
>> [  109.442600]  ret_from_fork+0x22/0x30
>> [  109.518225] nfsd: last server has exited, flushing export cache
>> [  OK  ] Stopped NFSv4 ID-name mapping service.
>> [  OK  ] Stopped GSSAPI Proxy Daemon.
>> [  OK  ] Stopped NFS Mount Daemon.
>> [  OK  ] Stopped NFS status monitor for NFSv2/3 locking..
>> Index: b/net/sunrpc/svc_xprt.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -1062,7 +1062,7 @@ static int svc_close_list(struct svc_ser
>> 	struct svc_xprt *xprt;
>> 	int ret =3D 0;
>>=20
>> -	spin_lock(&serv->sv_lock);
>> +	spin_lock_bh(&serv->sv_lock);
>> 	list_for_each_entry(xprt, xprt_list, xpt_list) {
>> 		if (xprt->xpt_net !=3D net)
>> 			continue;
>> @@ -1070,7 +1070,7 @@ static int svc_close_list(struct svc_ser
>> 		set_bit(XPT_CLOSE, &xprt->xpt_flags);
>> 		svc_xprt_enqueue(xprt);
>> 	}
>> -	spin_unlock(&serv->sv_lock);
>> +	spin_unlock_bh(&serv->sv_lock);
>> 	return ret;
>> }
>>=20
>>=20
>=20

--
Chuck Lever