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@redhat.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH RFC] NFSD: Bump the ref count on nf_inode
Thread-Topic: [PATCH RFC] NFSD: Bump the ref count on nf_inode
Thread-Index: AQHYkhpvRGZ0dTioakaq+LLAbdwSV61zIGIAgAAA5oA=
Date:   Thu, 7 Jul 2022 16:58:14 +0000
Message-ID: <98746C90-BB21-4FE5-9000-BCC33662F8F1@oracle.com>
References: <165720933938.1180.14325183467695610136.stgit@klimt.1015granger.net>
 <f3dc1a01fae6759a350adabf944892417a63d529.camel@redhat.com>
In-Reply-To: <f3dc1a01fae6759a350adabf944892417a63d529.camel@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?dsyhmbMZO/DOLx8vY+mo8VbKb3S9pztsAehI79I2XN6AkQkQBQLhzSiw27b+?=
 =?us-ascii?Q?SGyhN5vJVFZQssQpuVM7DJDZFYR6K1gFmC2goS6Hlq9Vz1pTW07Ll3BuPoWm?=
 =?us-ascii?Q?P/0hebdkh+oejBF5iBgniKFI1U9dVCsVd38wa8ZV/av9AycPbRTFdPm9m/cj?=
 =?us-ascii?Q?por133VmxwNQbsm1Gr9uyrpfwnvY/VfQqoYz21sjrjNrBdlAWNYR/DvoYynU?=
 =?us-ascii?Q?dRlK43ijBoEbLFR4Y37ipl3z/t66yW3sgDOhAEPP6QpoEwCUqcTH5NYDUE4a?=
 =?us-ascii?Q?2rfaAdraGHYy2MWWiy0isnHBSArnwb5ZSHfH+93B/XzO7HVol0OL09tW5OFu?=
 =?us-ascii?Q?2qpYbn73Yq/zqBc+r3cPU2MMsjczJmHutAd8G6r1HbXXeeTYWxJyN59/25Xm?=
 =?us-ascii?Q?H/GJWrsi8Ki2HtFCrIkf12b7h9IDAH5hPnWrlF4gfm8Tt5PiZhCPNgWBsXsC?=
 =?us-ascii?Q?xGCJAUYOvNtl9d+tUUsKA2QfCDcOWjArpAsIYJ6MNzyvUSSl2wyF5oAZL3xA?=
 =?us-ascii?Q?WrgSG42xGaTOC/wM6tR8/OlLOfpnrnwlTMCes3VAJy83aj5ClvItuJFglZra?=
 =?us-ascii?Q?9lkyx/wOOaqaCWjEx2lfmVG5lzivtFeA6Fe+TTNxnOkpwOA1tr4JysnNTj35?=
 =?us-ascii?Q?sxOajn5FuPRDIvJeJwL/csSGiKvABKd8UajxjO5ZxCxWyatorcrjUDwIIpqg?=
 =?us-ascii?Q?sW0WGTWnn94raI55HNnWmjiBqhekG6JIlVnubnv0cQF5u+ARkH0XmlCaE2Es?=
 =?us-ascii?Q?QfvZlQB0PXtLsg831gWtjicT4XxUYMwNpY7blf+qrxelNDKrKF0v8i0y/6RO?=
 =?us-ascii?Q?u/UQe7O5m/7802iXTfkdS/Bme7tHpUljxn9AE+pqcv4SJlQY9xS3f3vwZmHD?=
 =?us-ascii?Q?4gWFGuzoZDrZyeJsgHZc3S4xVbVUyEQCPkJbky3ha4toAukBZiZYEhyLFylj?=
 =?us-ascii?Q?IOvpNMxPVphnfLOH3CJRom91Bt1ZMLqtgkdG31HEd/jOpKmlk/O3MGUyi5Rh?=
 =?us-ascii?Q?t33cTz9pYi+8GwIxoROSqhAA2E272QnkGGKMBHjDuOb3+hhSzn2ip/Ggbxo7?=
 =?us-ascii?Q?240nHsRzYFTdcbXCD0UM8/1IEtDYCI/1Cw+pTo+c6dGqHrnSuLBycoD3Zk5S?=
 =?us-ascii?Q?FB3DZP3eY3M5JPwSwuRtRdlSMSlf+MCvp9Vdym8D07YBHZ/+1VltcV7OvB0a?=
 =?us-ascii?Q?JNYCLRc3Qyyex9Ru+wTAjrIJcoPX19Dxp/b1+dP2IRHfUzJglXO+T3lI9fgT?=
 =?us-ascii?Q?jVMmhLwoJNF14Anyrks9zL4yEFlrz69tiznsFYOuQiv83BHRVUTMPO+oxsUw?=
 =?us-ascii?Q?eNFPv9GcFUYlo0IKPXfO6uWBiLZx48GKzhwWCqXpPQgL6cMqkcF3Ugj6L90d?=
 =?us-ascii?Q?vOVCBRs41RFQP+F1X0qwzun5PWR4yrWLxUdTgs6moCqvSycWrFCyt7uZQHXS?=
 =?us-ascii?Q?uVoXPtg2RUA+MkWFmLg1IhskJU2ZzCg25AcuestJQ62sKezOMOn527gEWKnS?=
 =?us-ascii?Q?ZSSxhWNK/hgH+QDytkQm86yHIFfhHwgSzNemvKt1VegD1JLX543yWgqc2M3/?=
 =?us-ascii?Q?nPB+1S6yLdhok94524QvshdNyhx2jUhs1Ea5AkYQpHrt/e84HzWfK7xPhilq?=
 =?us-ascii?Q?Nw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30C893887055F944A66E90F2EA10DC75@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: f3401a3e-49c0-445f-b844-08da6039e1f2
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jul 2022 16:58:14.6372
 (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: ufU7aG7F9aAviSMgYeLzAvU9D0LBHkYd7vdzSVP6g9UIcFMbOVKbUN82ubU+5DOJjr9yY1y4WLZjC98M8OzFWA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5773
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-07_13:2022-06-28,2022-07-07 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0 phishscore=0
 adultscore=0 malwarescore=0 mlxscore=0 spamscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207070068
X-Proofpoint-GUID: RS07WFDvFh64caKA7t47gVBoHx8n5dxe
X-Proofpoint-ORIG-GUID: RS07WFDvFh64caKA7t47gVBoHx8n5dxe
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,T_SCC_BODY_TEXT_LINE
        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 Jul 7, 2022, at 12:55 PM, Jeff Layton <jlayton@redhat.com> wrote:
>=20
> On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
>> The documenting comment for struct nf_file states:
>>=20
>> /*
>> * A representation of a file that has been opened by knfsd. These are ha=
shed
>> * in the hashtable by inode pointer value. Note that this object doesn't
>> * hold a reference to the inode by itself, so the nf_inode pointer shoul=
d
>> * never be dereferenced, only used for comparison.
>> */
>>=20
>> However, nfsd_file_mark_find_or_create() does dereference the pointer st=
ored
>> in this field.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/filecache.c | 3 +++
>> fs/nfsd/filecache.h | 4 +---
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>=20
>> Hi Jeff-
>>=20
>> I'm still testing this one, but I'm wondering what you think of it.
>> I did hit a KASAN splat that might be related, but it's not 100%
>> reproducible.
>>=20
>=20
> My first thought is "what the hell was I thinking, tracking an inode
> field without holding a reference to it?"
>=20
> But now that I look, the nf_inode value only gets dereferenced in one
> place -- nfs4_show_superblock, and I think that's a bug. The comments
> over struct nfsd_file say:
>=20
> "Note that this object doesn't hold a reference to the inode by itself,
> so the nf_inode pointer should never be dereferenced, only used for
> comparison."
>=20
> We should probably annotate nf_inode better. __attribute__((noderef))
> maybe? It would also be good to make nfs4_show_superblock use a
> different way to get the sb.
>=20
> In any case, this is unlikely to fix anything unless the crash happened
> in nfs4_show_superblock.

Thanks for the look. I will treat this as a clean-up, then, and see
what can be done about nfsd_file_mark_find_or_create() and
nfs4_show_superblock().


>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>> index 9cb2d590c036..7b43bb427a53 100644
>> --- a/fs/nfsd/filecache.c
>> +++ b/fs/nfsd/filecache.c
>> @@ -180,6 +180,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int ma=
y, unsigned int hashval,
>> 		nf->nf_cred =3D get_current_cred();
>> 		nf->nf_net =3D net;
>> 		nf->nf_flags =3D 0;
>> +		ihold(inode);
>> 		nf->nf_inode =3D inode;
>> 		nf->nf_hashval =3D hashval;
>> 		refcount_set(&nf->nf_ref, 1);
>> @@ -210,6 +211,7 @@ nfsd_file_free(struct nfsd_file *nf)
>> 		fput(nf->nf_file);
>> 		flush =3D true;
>> 	}
>> +	iput(nf->nf_inode);
>> 	call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
>> 	return flush;
>> }
>> @@ -940,6 +942,7 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct =
svc_fh *fhp,
>> 	if (nf =3D=3D NULL)
>> 		goto open_file;
>> 	spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
>> +	iput(new->nf_inode);
>> 	nfsd_file_slab_free(&new->nf_rcu);
>>=20
>> wait_for_construction:
>> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
>> index 1da0c79a5580..01fbf6e88cce 100644
>> --- a/fs/nfsd/filecache.h
>> +++ b/fs/nfsd/filecache.h
>> @@ -24,9 +24,7 @@ struct nfsd_file_mark {
>>=20
>> /*
>> * A representation of a file that has been opened by knfsd. These are ha=
shed
>> - * in the hashtable by inode pointer value. Note that this object doesn=
't
>> - * hold a reference to the inode by itself, so the nf_inode pointer sho=
uld
>> - * never be dereferenced, only used for comparison.
>> + * in the hashtable by inode pointer value.
>> */
>> struct nfsd_file {
>> 	struct hlist_node	nf_node;
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@redhat.com>

--
Chuck Lever