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+LLAbdwSV61zIGIAgAABKICAAAC9gA==
Date:   Thu, 7 Jul 2022 17:01:48 +0000
Message-ID: <3A439D76-F78E-4449-923D-7E71A47FE36E@oracle.com>
References: <165720933938.1180.14325183467695610136.stgit@klimt.1015granger.net>
 <f3dc1a01fae6759a350adabf944892417a63d529.camel@redhat.com>
 <307aab1000890798345175063c24a77038a78167.camel@redhat.com>
In-Reply-To: <307aab1000890798345175063c24a77038a78167.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?zHfBbmT1qc/Bb3a+gXIjxO8LO1qYI/9rDIvDnRorkvm4kR3coJPMIvczrwVO?=
 =?us-ascii?Q?rFz57gl1gYp4V3sfQSB6qANLLFRi+r0/6wKm09Wzw8q2G6bHbuWDPCFJPmTe?=
 =?us-ascii?Q?DMlckz2pjjcP6bvpCVWe+GUvBTD+JkXtS19qg4tprhZwLt8Ji95i9aSzP5SA?=
 =?us-ascii?Q?uGz2BSRlM5UFWzzewEHXTtvGiG2Hh51BwSDZa3fpnFVqwwJVVCtuZ5XNCKKq?=
 =?us-ascii?Q?elBwJXGiTHXLBdxlGX+qblKzE6D+uObmnydKPaTcD/5UqNLyMIoW6B4+A8Pe?=
 =?us-ascii?Q?KgXmaZjOeXMFtVF3OqyItuZo74TceEc/1wiG3ivZ2/lEm3z0gcH0/z8uTsY1?=
 =?us-ascii?Q?H9nnGmBsKHwMsVC9sb7XS7ynO0hCqp4om2qT7F7CoD78KJBuOpp1E+fjdLJp?=
 =?us-ascii?Q?3crMHVqz+Rx3KnzmaFrVHPribi4X/NlHQ2p6Hg8FAFCRCRQThmVWb2aea0Ls?=
 =?us-ascii?Q?Vf6gcxp1DaNI9XiTtF4MQmXW0ZMcTMqvC/xxj8k/Gr+WNHdKYmEppagq2VL2?=
 =?us-ascii?Q?snQNRCwZAqLz/DuV3RaIjNUE4Hgi9usujT5D5h8Ww/G+HY7jWo0YImOADias?=
 =?us-ascii?Q?YKY56oMDH6qnsVLWLJnOoqSlGLBof8JsmcPwcP9tYCnU16LvF3I+RDJY/SB2?=
 =?us-ascii?Q?qLY0jiWsANDdKQC4eSPsfmD/aVtgNlvzfa7COUU2NB8QScj43qwftJWA0fj2?=
 =?us-ascii?Q?tK0DQshSk8Qhv1IvgVDIq8HR4r+P/kyDN78oR0kNu1uPjBOmRVMACSCEeRR4?=
 =?us-ascii?Q?sR13/92SI68TVBlU4vsjISuAZcxN5LgsX4s/IehIU5QHA5yrcZyC2xBT7gx/?=
 =?us-ascii?Q?yFhtxtAkgZTFepislqpGvOYtrtyF9RWhqj1YWNDjxj4hmpuPeVdlRwIZVA5R?=
 =?us-ascii?Q?q0zHh0dwBtjS1rTTuxKa2n9Y0J1g7m76H/f+ATcrPppLcIoQdhE961cLU6h+?=
 =?us-ascii?Q?fvfN0S+lKwb/H2x42BifXUwLMl3iCSyNWZENxz9+7+brM/AuF60Fbe3wk0JC?=
 =?us-ascii?Q?4QQpKxpp8iPlYTbIb3Or2bLVvsx8j/qHt1afsHYUEl2VSHD9h1R5l1aTu1F7?=
 =?us-ascii?Q?ceSYBx/ACdPKTPbfD7m3BQRkuI0eGCQwOFXjpgYgi6NAQEFR0ONvadaZpjSh?=
 =?us-ascii?Q?7Wlsq/KuF3gHR50Ha37/zD/vW77dnIzovP3Ft3Pd1jdA0ijzxic/v4RvR+/i?=
 =?us-ascii?Q?ft6hLl+nR4Z/TRbVe0oy2LxqGF2kjHx1S0aTMnIPXUDx0waA+05dnhnF+wz5?=
 =?us-ascii?Q?mwmK5QyTX9ylefz6U6+zaI+Oz1vfPfrkmeNCczYla9WSK+daaptzwFOCIVeD?=
 =?us-ascii?Q?+yqQyGleT0OoM1CfXUvtlJWwiaSM3a8Lm5nWnA7EnCl4FlDzeWbq4Pm0hTd2?=
 =?us-ascii?Q?Ifl2MA3LIplDx/40wo/I3IUCWdBNlvc1zX6Qh1Pu/fKSLamxqbVu/sNAJaRZ?=
 =?us-ascii?Q?kr2BOLa9XuwD7kPZrabP7/8ihomav6JvjC9YKzsJuQxq/EhiVI2iuR/3Txby?=
 =?us-ascii?Q?2gZrFLkeJPlYtMsv3RhwxNJFGQe7eo/BwphK7+iFDmu+GwFuaZI/bQ4NY6kA?=
 =?us-ascii?Q?gZ4NBAjQOT0yp5V0QClTGIE4HcdxDOhU5PILv75V03gO2Ssp3+4Vi1V0aw+G?=
 =?us-ascii?Q?hg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2076A4E193F75D40A12E3F2E78829512@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: d0894b4d-950b-4c94-99d8-08da603a613a
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jul 2022 17:01:48.1477
 (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: g135Cp7MhUG+/h7GFWDF+FwwIZbHWBD/PRKmUchLnpXDU3cOej8aeaE1EQePnT+/OEWchVpoGgr63nlLpwiVow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1001MB2226
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
 mlxlogscore=999 spamscore=0 adultscore=0 malwarescore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207070068
X-Proofpoint-GUID: QFxApwV0IJ6x3mM7W9biuj-5gu6ZVocI
X-Proofpoint-ORIG-GUID: QFxApwV0IJ6x3mM7W9biuj-5gu6ZVocI
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:59 PM, Jeff Layton <jlayton@redhat.com> wrote:
>=20
> On Thu, 2022-07-07 at 12:55 -0400, Jeff Layton wrote:
>> 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 h=
ashed
>>> * 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 shou=
ld
>>> * never be dereferenced, only used for comparison.
>>> */
>>>=20
>>> However, nfsd_file_mark_find_or_create() does dereference the pointer s=
tored
>>> 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.
>>=20
>>=20
>=20
> One other spot. We also dereference it in nfsd_file_mark_find_or_create,
> but I think that specific instance is OK. We know that we still hold a
> reference to the inode at that point since it comes from fhp->fh_dentry,
> so we shouldn't need to worry about it disappearing out from under us.

Needs some annotation. I would prefer not to get that pointer from
nf_inode, then. As your comment says: compare only, never deref.


> What did the crash look like?

Jul 06 11:19:32 klimt.1015granger.net kernel: BUG: KASAN: use-after-free in=
 nfsd_file_obj_cmpfn+0x26b/0x49a [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel: Read of size 4 at addr ffff88=
8180623e1c by task nfsd/1003
Jul 06 11:19:32 klimt.1015granger.net kernel:=20
Jul 06 11:19:32 klimt.1015granger.net kernel: CPU: 3 PID: 1003 Comm: nfsd N=
ot tainted 5.19.0-rc5-00037-g17ba024b204f #3522
Jul 06 11:19:32 klimt.1015granger.net kernel: Hardware name: Supermicro Sup=
er Server/X10SRL-F, BIOS 3.3 10/28/2020
Jul 06 11:19:32 klimt.1015granger.net kernel: Call Trace:
Jul 06 11:19:32 klimt.1015granger.net kernel:  <TASK>
Jul 06 11:19:32 klimt.1015granger.net kernel:  dump_stack_lvl+0x56/0x7c
Jul 06 11:19:32 klimt.1015granger.net kernel:  print_report+0x101/0x4bb
Jul 06 11:19:32 klimt.1015granger.net kernel:  kasan_report+0x9f/0xbf
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd_file_obj_cmpfn+0x26b/0x=
49a [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  rhashtable_lookup.constprop.=
0+0x143/0x1ca [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd_file_do_acquire+0x20b/0=
x1189 [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd_write+0x138/0x255 [nfsd=
]
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd3_proc_write+0x37e/0x3fc=
 [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd_dispatch+0x5ed/0x7d0 [n=
fsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  svc_process_common+0x8e9/0xe=
fe [sunrpc]
Jul 06 11:19:32 klimt.1015granger.net kernel:  svc_process+0x34d/0x378 [sun=
rpc]
Jul 06 11:19:32 klimt.1015granger.net kernel:  nfsd+0x26b/0x34c [nfsd]
Jul 06 11:19:32 klimt.1015granger.net kernel:  kthread+0x249/0x258
Jul 06 11:19:32 klimt.1015granger.net kernel:  ret_from_fork+0x22/0x30
Jul 06 11:19:32 klimt.1015granger.net kernel:  </TASK>

The freed object was actually not an inode, so it's a red herring.
Still chasing it.


>>> 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 m=
ay, 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 h=
ashed
>>> - * in the hashtable by inode pointer value. Note that this object does=
n't
>>> - * hold a reference to the inode by itself, so the nf_inode pointer sh=
ould
>>> - * 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
> --=20
> Jeff Layton <jlayton@redhat.com>

--
Chuck Lever