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+LLAbdwSV61zIGIAgAAIbIA=
Date:   Thu, 7 Jul 2022 17:25:09 +0000
Message-ID: <5B84D6D2-E9BC-4518-B52C-ABAF240DE2A1@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?WsMCb367nrvkgdH8eFyHrrQHUukU5DHpdh3giYsF6jR6goOQT0gcOLfrWQkn?=
 =?us-ascii?Q?E7oqv4eeFTdicRZWzeCLArfnfPTih5V0Tau7VocfeYrnhhjUlvQx1Mqly4V7?=
 =?us-ascii?Q?l8DuW4w2KfiITrQZIBEzvh6LC2nJBfpNVOOWn60PBeaXWnuk0TQdMwBu2g3v?=
 =?us-ascii?Q?6LL63L9ZCQPsM671EoloB6z5+J+2IuRY3md7ifpt1zjDr1NzPSj6/Gnjl/DZ?=
 =?us-ascii?Q?0+oNDrKCkMjywDkw2JHOG2SfIjLAS7QrAU9SvrB+eyTrV49N9IKdDq6aupV6?=
 =?us-ascii?Q?viNJFPGlyBdGJq2oEEqhjrEKDPq5xDh3sc5tFWL39FadIvU9USLUZBcN2zT2?=
 =?us-ascii?Q?/SZQlE1FYlKSorb6ntqeoQkQPVIHoaUt6TAGFt4t3I7/y0Gog339aYAaX/DT?=
 =?us-ascii?Q?ezDqrg3bMQMCP/7KjSqWv16DDa12SxrXd4BivqDNL9yTaG78UTVKjsUzObcu?=
 =?us-ascii?Q?Aef1vi3qAXadNHRASGNSy/JJf6P5W4xQZ0sDvAZJBv4TMW5vfKQvytfwwqTa?=
 =?us-ascii?Q?uhqLjgeCxvAjHcZP5XQVe0ai+0OmbdZuqdl7bNg9y2G/h7AlpxlxOikrkrHm?=
 =?us-ascii?Q?el/Q681c4VMmqjUTJOrGvEJWThsz9z5+2sq5Tm48/IIasi8ctNfiOD0ziPpc?=
 =?us-ascii?Q?R0XVToucuuHqqUZvWAlO/6Fv2G++LyP16HslecWxlm+5ATMGqrhWfgGY778K?=
 =?us-ascii?Q?MBwZoCu006Q6mByUHxJqZAzs5lRU+p8ykKno5IT3JJQRMySETL8eAVO0PeGJ?=
 =?us-ascii?Q?ZQlVry3IrfhyJhA1skGg76i09SF9KhTWx8TImMakGzXJWHGZWfANcT+N5/C+?=
 =?us-ascii?Q?w+8uy7Er2do/YJgvAVYHei+uROogpc+RKQzm2B1HwmSeVg/r1bHxuFXnO98Y?=
 =?us-ascii?Q?swswSOA3y5/Uh8/KBl1Z/6FjaQaXNeq5cAjQMu8z7XjmmbX5J4Vb9F83ZAVY?=
 =?us-ascii?Q?dihly4cyBi5FET496/hzUTaMCOhax2cgtjJAQQ+UmfXPEvG497kTWuUb7eVR?=
 =?us-ascii?Q?dW14hWP/6jzvQ5xAcJlxcu8VXv4z/gFNeoQKoax0t+d/tfxPhWaExv97DjpC?=
 =?us-ascii?Q?f3/Sk8hHn1pJ7ZyhfcC0sL0UiCSsrwZT7OiEajjoqOrb13igHkJxQsgmrb0w?=
 =?us-ascii?Q?at24PSULk2wZM4Py9ZdKa3wR+CfU/8zYB5P/OucNR89t9lplqyQshenq2YDY?=
 =?us-ascii?Q?sqAA/8YRoHYmdIwdtT+PbTSFYpErugrRR9SI2QN3si39iq3hweKNl+2J2WsG?=
 =?us-ascii?Q?F9T58qASACpgNfPv9ltwxF/9jcQZv0QbvqKzDs7wKFihuFvHhUO5sB2QQIHM?=
 =?us-ascii?Q?tz94RTi0W0Aps3yUeRdPyUDpB20ydh70EZlGxLTFKRafZYuWYvitHVULtP0N?=
 =?us-ascii?Q?HikuxPEDGL+f65S57mStPxp6YohsZYZ4yDx7lW2LfyaDbTl9N/1oP81hn65X?=
 =?us-ascii?Q?NVYqm7A6177PgwAyRy8qWhRUFR5cOsSNvOtiJN6svoqMmxsuJHEMq+09mN7r?=
 =?us-ascii?Q?K6nyeGWfwIIuW8mToFUQdY2E2ZyPkWUH3BF2U9SyRZL1Fnq5+BaNrT8leSPt?=
 =?us-ascii?Q?MxYQvLIbOq05WuPmZIS5CppnU6EEDqDG11YXv4tW1R6ncsF9SA4gueCnEUPb?=
 =?us-ascii?Q?HA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F621720106D167459D524336413A7E6B@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: a5ea958d-e994-4252-4cf2-08da603da49b
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jul 2022 17:25:09.6790
 (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: 7sKOdGaLJGVDrC4HcQ900gGKdrBvWtHQMhyQ+UXOgdzLqcrdXkufdqsLieofSYlClCUUrzn5zPVC4uSx5l7dAA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4242
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 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 spamscore=0 suspectscore=0 adultscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207070069
X-Proofpoint-ORIG-GUID: 7PxQyJxTbGsr7vBX7iEQ8JB6hJX5j9WJ
X-Proofpoint-GUID: 7PxQyJxTbGsr7vBX7iEQ8JB6hJX5j9WJ
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.

How about f->nf_file->f_inode ?


--
Chuck Lever