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:     Benjamin Coddington <bcodding@redhat.com>
CC:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NLM: Defend against file_lock changes after
 vfs_test_lock()
Thread-Topic: [PATCH] NLM: Defend against file_lock changes after
 vfs_test_lock()
Thread-Index: AQHYfysdIoOx/JiCV0ytFXEmbRLY1a1NqUmA
Date:   Mon, 13 Jun 2022 18:31:36 +0000
Message-ID: <53767203-004C-4538-8E29-1241CFB19F43@oracle.com>
References: <9688295e35c07d3b3d6c71970b6996348c2d8f1e.1654798464.git.bcodding@redhat.com>
In-Reply-To: <9688295e35c07d3b3d6c71970b6996348c2d8f1e.1654798464.git.bcodding@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?6poy3de8ToH/yv26FXkiudqqGuSWMy3lbKQh8Tg5ip3VlcXeF3Y2p+3KOhTE?=
 =?us-ascii?Q?8Hyj0CuueWa+bYe49BYNYmZvrcg+M6PGmWhL6a5nW4xpkXEiIgxZgvlEkMC+?=
 =?us-ascii?Q?/XK8U7i0gqXjZOWa/gCw0FH8OlhdlvFskwz3x6bgwRvTsMbDIfe+gGT9oXEX?=
 =?us-ascii?Q?CMxEduMd/oUGZUPDoBwsyEmeD6AqzeNKSxLFNHe+KMG3CAOz1KfOBfMKIvR4?=
 =?us-ascii?Q?KQYL84O70Bpa7CUwECQbNhTd4j9CMJ7nrF+gv6DXwCx9Ohv6GuGggt5xwVAA?=
 =?us-ascii?Q?o/XdnKMhdfcaGLmy7o5vq2zvzKrxtKc0z4vlSkcvXuXNGbAKS4S6mPED0kkC?=
 =?us-ascii?Q?bDzqrEfPkdQOeY82DwmAcSCU5yPf9orcKy1+k4hvZ6u96NAMA27oWXSo2dJG?=
 =?us-ascii?Q?HRyXEi5BCDn1GhzMKQ9lVUvBJuKsvcgIHa8uiL/cdVgOMa5+SuNiAdN+u4oS?=
 =?us-ascii?Q?Wuh5sQYxzqL3b/VTWATEG3S8HwldQtYU5Bs61OQNLTCDuKBtVSufim7mOhDN?=
 =?us-ascii?Q?VAnyZnXGAPqB4EXMch0raRIwPf7N3A0f1kQ7+X42roq45ItqNHLKU9pf3ln1?=
 =?us-ascii?Q?+rxNpG6zwF+2DTzFw1x8iGjM6iFsO5wrFmvTDiQBJg2wgErIPrZKR2rqnJhi?=
 =?us-ascii?Q?KCxulUPd42KVkoBz+d3A66Y+kqQu7GMIUcxHkRFvG7nieEFBRdXbz9kKCsdx?=
 =?us-ascii?Q?oOURXNvOmA/rJQR3Iz0QowLLr09LePYPBixhH5xRrOurVZtQEB03YnIEi0xy?=
 =?us-ascii?Q?joX+R3V+ZX+pSydlLubFNnilxhKmCfLKD3eeZ/1H836gwCmAx5xBlx9qTijk?=
 =?us-ascii?Q?itgyFitccNQqhWdCprEYRGjn5gILWkaS9t7F764E1EMvBbeG6dolhpszRUHz?=
 =?us-ascii?Q?rbtLM09FWp/Y98+13mQH2sAfXUYd0HIYEjJCnxc/2De9p7OcxgnrBEhA5N02?=
 =?us-ascii?Q?l9BjhAMbivBKxrtDW/N07MffoIaOsvYhnyIRlRzt84vDqhYsaiGqTzC+424A?=
 =?us-ascii?Q?CjqUwXhhsFRoenJTaEIlR87/yPP/UEf6VBzY4RAnhQJtd0UJZXrFYDclbYel?=
 =?us-ascii?Q?Kd+fg0ozNJlxjsw0ONB9v8f/9DSaidP9xC99D1Lb3+jIxcmMl7mJMbDlnygS?=
 =?us-ascii?Q?8MxLgqFdAflMYrafZUY/SNRy8/jXG/orhmObjHlHwfDTMnLfjR2nf8Ptax7b?=
 =?us-ascii?Q?Qxvu9I1I6SYVmGVBqWdyJR1wrjLMCJQUU6M6hH6QQgtwGvE/4p5ei8q87VPY?=
 =?us-ascii?Q?uzjZt+KqBcVFx5hArN4nfDDAHxQ6zzrtA1Q/jZRO7p8nJ+nI5l6l1ZmHyNEc?=
 =?us-ascii?Q?9qEk+wV9x6vZlovTWWoQAnpdTb/IN4yMmBzA2UwXtuJnM/NcW+e5ZDRPKSUF?=
 =?us-ascii?Q?XkNIora2Fw8Qua0jU+d+WhYUKE5BMXvQmwa3SHrFbiXv6hVRBiBuPxr4fvcu?=
 =?us-ascii?Q?13208lyP4dLhif4SjyE9IOzLsd3crTrOHY6REeVVch2gs+Pap4s5jeoQliFa?=
 =?us-ascii?Q?oqXC6aLqOq1iygpXlMfF3fABHYGncRoy4cBLu8qmHkWE477AKIOCuZhftM/o?=
 =?us-ascii?Q?802TEQ5Ub9YLw903FQ8oooTkVZne+5XpmCvnjYkXRHTbdkj4St2Dm3mYUy0o?=
 =?us-ascii?Q?RlV6G7Ejg2Dmc6qoJxFwK+BhM+NRzeJ0uPF1ZYWqwHsIQG84abh2ZeNCQ59v?=
 =?us-ascii?Q?huZAhS7/5CS3fyDx+YlcNi9MrmDhIT2pr7GSvuim3cBT8/Zmo2WXr+nJZvk1?=
 =?us-ascii?Q?9XfTABzezitJOXcUx79nq+XMEtTz2EM=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <86F760062C4E4646A8CE9A6BBEF0ECDE@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: 56cd47de-b011-48e4-ab61-08da4d6af2ec
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jun 2022 18:31:36.3346
 (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: Hb+yK9PouZTEwBj8Iq0IyHqmGr2vpbtWIOYzxjMhVFuUgBc4E0GIfv+vJYYwvBSx87Eaan3t8TqbJFocrJoc2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4619
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.874
 definitions=2022-06-13_08:2022-06-13,2022-06-13 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 phishscore=0
 adultscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2206130078
X-Proofpoint-ORIG-GUID: yjZ64fC6hRmLcb_Le2z0An6RHna3RoYg
X-Proofpoint-GUID: yjZ64fC6hRmLcb_Le2z0An6RHna3RoYg
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 Jun 13, 2022, at 9:40 AM, Benjamin Coddington <bcodding@redhat.com> wr=
ote:
>=20
> Instead of trusting that struct file_lock returns completely unchanged
> after vfs_test_lock() when there's no conflicting lock, stash away our
> nlm_lockowner reference so we can properly release it for all cases.
>=20
> This defends against another file_lock implementation overwriting fl_owne=
r
> when the return type is F_UNLCK.
>=20
> Reported-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> Tested-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>

LGTM. Applied internally for more testing. Comment period
still open.


> ---
> fs/lockd/svc4proc.c         |  4 +++-
> fs/lockd/svclock.c          | 10 +---------
> fs/lockd/svcproc.c          |  5 ++++-
> include/linux/lockd/lockd.h |  1 +
> 4 files changed, 9 insertions(+), 11 deletions(-)
>=20
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 176b468a61c7..4f247ab8be61 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -87,6 +87,7 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_=
res *resp)
> 	struct nlm_args *argp =3D rqstp->rq_argp;
> 	struct nlm_host	*host;
> 	struct nlm_file	*file;
> +	struct nlm_lockowner *test_owner;
> 	__be32 rc =3D rpc_success;
>=20
> 	dprintk("lockd: TEST4        called\n");
> @@ -96,6 +97,7 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_=
res *resp)
> 	if ((resp->status =3D nlm4svc_retrieve_args(rqstp, argp, &host, &file)))
> 		return resp->status =3D=3D nlm_drop_reply ? rpc_drop_reply :rpc_success=
;
>=20
> +	test_owner =3D argp->lock.fl.fl_owner;
> 	/* Now check for conflicting locks */
> 	resp->status =3D nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->=
lock, &resp->cookie);
> 	if (resp->status =3D=3D nlm_drop_reply)
> @@ -103,7 +105,7 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nl=
m_res *resp)
> 	else
> 		dprintk("lockd: TEST4        status %d\n", ntohl(resp->status));
>=20
> -	nlmsvc_release_lockowner(&argp->lock);
> +	nlmsvc_put_lockowner(test_owner);
> 	nlmsvc_release_host(host);
> 	nlm_release_file(file);
> 	return rc;
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index cb3658ab9b7a..9c1aa75441e1 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -340,7 +340,7 @@ nlmsvc_get_lockowner(struct nlm_lockowner *lockowner)
> 	return lockowner;
> }
>=20
> -static void nlmsvc_put_lockowner(struct nlm_lockowner *lockowner)
> +void nlmsvc_put_lockowner(struct nlm_lockowner *lockowner)
> {
> 	if (!refcount_dec_and_lock(&lockowner->count, &lockowner->host->h_lock))
> 		return;
> @@ -590,7 +590,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_fi=
le *file,
> 	int			error;
> 	int			mode;
> 	__be32			ret;
> -	struct nlm_lockowner	*test_owner;
>=20
> 	dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=3D%d, %Ld-%Ld)\n",
> 				nlmsvc_file_inode(file)->i_sb->s_id,
> @@ -604,9 +603,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_fi=
le *file,
> 		goto out;
> 	}
>=20
> -	/* If there's a conflicting lock, remember to clean up the test lock */
> -	test_owner =3D (struct nlm_lockowner *)lock->fl.fl_owner;
> -
> 	mode =3D lock_to_openmode(&lock->fl);
> 	error =3D vfs_test_lock(file->f_file[mode], &lock->fl);
> 	if (error) {
> @@ -635,10 +631,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_f=
ile *file,
> 	conflock->fl.fl_end =3D lock->fl.fl_end;
> 	locks_release_private(&lock->fl);
>=20
> -	/* Clean up the test lock */
> -	lock->fl.fl_owner =3D NULL;
> -	nlmsvc_put_lockowner(test_owner);
> -
> 	ret =3D nlm_lck_denied;
> out:
> 	return ret;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 4dc1b40a489a..b09ca35b527c 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -116,6 +116,7 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm=
_res *resp)
> 	struct nlm_args *argp =3D rqstp->rq_argp;
> 	struct nlm_host	*host;
> 	struct nlm_file	*file;
> +	struct nlm_lockowner *test_owner;
> 	__be32 rc =3D rpc_success;
>=20
> 	dprintk("lockd: TEST          called\n");
> @@ -125,6 +126,8 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm=
_res *resp)
> 	if ((resp->status =3D nlmsvc_retrieve_args(rqstp, argp, &host, &file)))
> 		return resp->status =3D=3D nlm_drop_reply ? rpc_drop_reply :rpc_success=
;
>=20
> +	test_owner =3D argp->lock.fl.fl_owner;
> +
> 	/* Now check for conflicting locks */
> 	resp->status =3D cast_status(nlmsvc_testlock(rqstp, file, host, &argp->l=
ock, &resp->lock, &resp->cookie));
> 	if (resp->status =3D=3D nlm_drop_reply)
> @@ -133,7 +136,7 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm=
_res *resp)
> 		dprintk("lockd: TEST          status %d vers %d\n",
> 			ntohl(resp->status), rqstp->rq_vers);
>=20
> -	nlmsvc_release_lockowner(&argp->lock);
> +	nlmsvc_put_lockowner(test_owner);
> 	nlmsvc_release_host(host);
> 	nlm_release_file(file);
> 	return rc;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index fcef192e5e45..70ce419e2709 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -292,6 +292,7 @@ void		  nlmsvc_locks_init_private(struct file_lock *,=
 struct nlm_host *, pid_t);
> __be32		  nlm_lookup_file(struct svc_rqst *, struct nlm_file **,
> 					struct nlm_lock *);
> void		  nlm_release_file(struct nlm_file *);
> +void		  nlmsvc_put_lockowner(struct nlm_lockowner *);
> void		  nlmsvc_release_lockowner(struct nlm_lock *);
> void		  nlmsvc_mark_resources(struct net *);
> void		  nlmsvc_free_host_resources(struct nlm_host *);
> --=20
> 2.31.1
>=20

--
Chuck Lever