Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Zhi Li <yieli@redhat.com>
Subject: Re: [PATCH] nfsd: don't call nfsd_file_put from client states seqfile
 display
Thread-Topic: [PATCH] nfsd: don't call nfsd_file_put from client states
 seqfile display
Thread-Index: AQHY6sbEwHcYgr6RekGiqci+Z7OuE64j3CkA
Date:   Fri, 28 Oct 2022 14:23:33 +0000
Message-ID: <102A618E-29C3-497E-8D7D-771C9C160DE8@oracle.com>
References: <20221028121353.33567-1-jlayton@kernel.org>
In-Reply-To: <20221028121353.33567-1-jlayton@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Ojqa1ey2DjWazqYJHnj4007nRfY+s2jmx0/HO3LE0eAw04qIFnaAw6OL1LxM?=
 =?us-ascii?Q?sBAfD+uYBUYU6RqGjt1AVuNv4dehBx5igjstgtVk+/jEVQOd9wIzBX8jQc5o?=
 =?us-ascii?Q?lpZixsrbMDZxB4vcKc+SGVNOXP3aHJfSD7gIX86A9nkesUXnGVE+4jCTd0Kh?=
 =?us-ascii?Q?tTWtHZfva9g60WNW03JwvjCARwS3aQ0QG1QKgZiIVH1CSyZHg2h3QvuDD8JU?=
 =?us-ascii?Q?ftOThzrhqm4xWpwVL3XEAA5yPQkyNnOtPg+6a5mAUJlXKHAUaau3/C/N9Us4?=
 =?us-ascii?Q?FeXa1NRnRrOdeoiLWIcgyLmEgiFU3IRRLMGmIV297WkSUUSgwEUWuN30B7+w?=
 =?us-ascii?Q?Ld+TL4lJO3ohnw1OlwvZtR0bUyLEprCjJ3hG/weMYOezzlXECFiqBan3MwJu?=
 =?us-ascii?Q?GBBEA1IORFwDc8QRLQTwZJEtkJlYVWxgCoKohICafrcTfc4ASDVPDMdXsml+?=
 =?us-ascii?Q?w4uSJwr09leuhTmVF3EjYr2FjHkAUc4E1UDeJGTo4oJGgShIyBFGNqm7jumI?=
 =?us-ascii?Q?j/Ep4ofKMihYhi4fQDlwVwV1YJXljt7RjVWhd++DkOu1SJJwI8goOPSFPrF8?=
 =?us-ascii?Q?mgJwnTnKyhrjGOwPjml1QPvQil3gw+4xRzRTvBPlG7Y4J6Ax+0BaQxMpQnab?=
 =?us-ascii?Q?NnRiJLuAV13yqwfbHsNXXLM/ZY1DkBj+K7NebRUBh/PbWJc58xMVGf6Pijld?=
 =?us-ascii?Q?YfUUV2FlX5X4H+sYQZyyVcZwJw8rshOtLMADz/9mT2W1wPDrGdsNFEtCMw4q?=
 =?us-ascii?Q?M5wgQhNhohwsjdGgZFyhp53HpZnC6hyomqE52DIZuPBKGjul0qNaLYDP3orI?=
 =?us-ascii?Q?jgCE3Z/EsFHZ93vLTyGJUwV4D7cLkNfJIDFQEz8KBNRdq0zAA982TAFjokol?=
 =?us-ascii?Q?piakgPNx/OWFyDKqaeKLlwb+1yIza3RWFmGWyzQxqzYmk+xOJECyhc8+iZ8X?=
 =?us-ascii?Q?58D80bhuwh5/lYEvdCSxOuRaLVmfoziwrL7PgETcqPcbqR8bgjU2NbX6GpAK?=
 =?us-ascii?Q?+lIQ3SbgK1j18uEJbwOxnlvI/a4w3tISsGir4gUf/gNxyOBZXxAiSDW7ciMp?=
 =?us-ascii?Q?813i4at9ElYomCoubfivKAernoPuug4DnoXCAWoBBwUkqi7FVedNqaH9fKsG?=
 =?us-ascii?Q?zRoy/PbBcu+p+QiM/lpVTJq1VNAegTMq00NvMtThYtYFs1Wwt3Rx7toKfnOR?=
 =?us-ascii?Q?HNhJ1r4TgrhZcYiVl0rTk4Zz/bgwoPXVW9860YK5a/VI5dAJ555PYsmxHfnj?=
 =?us-ascii?Q?4fjXjlom9Tu/CIJoelKt2yNhjLIUiPReK6Aqo5q6YZ612+BRURBTvBsZaOdr?=
 =?us-ascii?Q?416dr8efcn5FLIHlVJez6yAWJU4a3X+SVI5x+nr7298jB5jIhgHGXdbRmmIf?=
 =?us-ascii?Q?KtyFHfBCHlc6AZhoNwPgUNRff4eZh32Qu3NZveInPfK8gG2SefOTVyXdH+fy?=
 =?us-ascii?Q?GIOiR3bDlronZEXMb2K30YA05eHD7i66oEAIrSJ6ZKdk7KIlafMa5UdwFQ/6?=
 =?us-ascii?Q?yP9EVFQEy11Tj82zzscLkLaAQtBlb4kKjXfus2g94tzIllidHqvgt/EYhgSr?=
 =?us-ascii?Q?fjwOl5E3+0kZ1vqLJX78aKfmXc0wr2SKrPx8UW3JOx8FtYFKCNmmebDQbvck?=
 =?us-ascii?Q?Bw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <96B0BF80FC643A448CC7B42E568ABD05@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2da7a74d-6a20-4c9d-847f-08dab8effea5
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2022 14:23:33.5145
 (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: WjvJYWijOSopSRxMnv5F+V2nF5lZ7UgJqasx23wjTuYX0JBC9onk10OilYArRP08LTCeB6c8DB6gGhwSE9ZvSw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6014
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-28_07,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 adultscore=0 malwarescore=0 mlxscore=0 suspectscore=0 bulkscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210280089
X-Proofpoint-GUID: zYTqZM4pSi8QfHKdi-3jqhIZEArvXfKy
X-Proofpoint-ORIG-GUID: zYTqZM4pSi8QfHKdi-3jqhIZEArvXfKy
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 28, 2022, at 8:13 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> We had a report of this:
>=20
>    BUG: sleeping function called from invalid context at fs/nfsd/filecach=
e.c:440
>=20
> ...with a stack trace showing nfsd_file_put being called from
> nfs4_show_open. This code has always tried to call fput while holding a
> spinlock, but we recently changed this to use the filecache, and that
> started triggering the might_sleep() in nfsd_file_put.
>=20
> states_start takes and holds the cl_lock while iterating over the
> client's states, and we can't sleep with that held.
>=20
> Have the various nfs4_show_* functions instead hold the fi_lock instead
> of taking a nfsd_file reference.
>=20
> Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2138357
> Reported-by: Zhi Li <yieli@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/nfs4state.c | 51 +++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 18 deletions(-)

Applied to nfsd's for-next (internally for now; will push later).


> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1ded89235111..dde621debeb2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -675,15 +675,26 @@ find_any_file(struct nfs4_file *f)
> 	return ret;
> }
>=20
> -static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> +static struct nfsd_file *find_any_file_locked(struct nfs4_file *f)
> {
> -	struct nfsd_file *ret =3D NULL;
> +	lockdep_assert_held(&f->fi_lock);
> +
> +	if (f->fi_fds[O_RDWR])
> +		return f->fi_fds[O_RDWR];
> +	if (f->fi_fds[O_WRONLY])
> +		return f->fi_fds[O_WRONLY];
> +	if (f->fi_fds[O_RDONLY])
> +		return f->fi_fds[O_RDONLY];
> +	return NULL;
> +}
> +
> +static struct nfsd_file *find_deleg_file_locked(struct nfs4_file *f)
> +{
> +	lockdep_assert_held(&f->fi_lock);
>=20
> -	spin_lock(&f->fi_lock);
> 	if (f->fi_deleg_file)
> -		ret =3D nfsd_file_get(f->fi_deleg_file);
> -	spin_unlock(&f->fi_lock);
> -	return ret;
> +		return f->fi_deleg_file;
> +	return NULL;
> }
>=20
> static atomic_long_t num_delegations;
> @@ -2616,9 +2627,11 @@ static int nfs4_show_open(struct seq_file *s, stru=
ct nfs4_stid *st)
> 	ols =3D openlockstateid(st);
> 	oo =3D ols->st_stateowner;
> 	nf =3D st->sc_file;
> -	file =3D find_any_file(nf);
> +
> +	spin_lock(&nf->fi_lock);
> +	file =3D find_any_file_locked(nf);
> 	if (!file)
> -		return 0;
> +		goto out;
>=20
> 	seq_printf(s, "- ");
> 	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2640,8 +2653,8 @@ static int nfs4_show_open(struct seq_file *s, struc=
t nfs4_stid *st)
> 	seq_printf(s, ", ");
> 	nfs4_show_owner(s, oo);
> 	seq_printf(s, " }\n");
> -	nfsd_file_put(file);
> -
> +out:
> +	spin_unlock(&nf->fi_lock);
> 	return 0;
> }
>=20
> @@ -2655,9 +2668,10 @@ static int nfs4_show_lock(struct seq_file *s, stru=
ct nfs4_stid *st)
> 	ols =3D openlockstateid(st);
> 	oo =3D ols->st_stateowner;
> 	nf =3D st->sc_file;
> -	file =3D find_any_file(nf);
> +	spin_lock(&nf->fi_lock);
> +	file =3D find_any_file_locked(nf);
> 	if (!file)
> -		return 0;
> +		goto out;
>=20
> 	seq_printf(s, "- ");
> 	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2677,8 +2691,8 @@ static int nfs4_show_lock(struct seq_file *s, struc=
t nfs4_stid *st)
> 	seq_printf(s, ", ");
> 	nfs4_show_owner(s, oo);
> 	seq_printf(s, " }\n");
> -	nfsd_file_put(file);
> -
> +out:
> +	spin_unlock(&nf->fi_lock);
> 	return 0;
> }
>=20
> @@ -2690,9 +2704,10 @@ static int nfs4_show_deleg(struct seq_file *s, str=
uct nfs4_stid *st)
>=20
> 	ds =3D delegstateid(st);
> 	nf =3D st->sc_file;
> -	file =3D find_deleg_file(nf);
> +	spin_lock(&nf->fi_lock);
> +	file =3D find_deleg_file_locked(nf);
> 	if (!file)
> -		return 0;
> +		goto out;
>=20
> 	seq_printf(s, "- ");
> 	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2708,8 +2723,8 @@ static int nfs4_show_deleg(struct seq_file *s, stru=
ct nfs4_stid *st)
> 	seq_printf(s, ", ");
> 	nfs4_show_fname(s, file);
> 	seq_printf(s, " }\n");
> -	nfsd_file_put(file);
> -
> +out:
> +	spin_unlock(&nf->fi_lock);
> 	return 0;
> }
>=20
> --=20
> 2.37.3
>=20

--
Chuck Lever