From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>, Dai Ngo <dai.ngo@oracle.com>
CC:     Pumpkin <cc85nod@gmail.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open
Thread-Topic: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open
Thread-Index: AQHZNtkQTUc54cGJ8kWHJe+mTHkMtq68DvmA
Date:   Thu, 2 Feb 2023 19:41:26 +0000
Message-ID: <CBAFB3E5-5737-47A7-8234-3A771E908C4F@oracle.com>
References: <20230202073611.13106-1-cc85nod@gmail.com>
In-Reply-To: <20230202073611.13106-1-cc85nod@gmail.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?EP7qpxNcZo/j5Y7vMMRJO5heGKcNlm27SmxRtIj0OBR0IzLk85w30Z29xcEH?=
 =?us-ascii?Q?8GPZIW9HpxsIwoPj0BHIfakLfeQR0O3LMta2AicBAz60G5Fpy10aCrabSqW/?=
 =?us-ascii?Q?wY9JHRfPF/d2UZldfL0IAjy/ldxwL4CLX053MKQRy28czDuxlye/Vx5rGE2x?=
 =?us-ascii?Q?jbbtB2jj0eAeQssFmvja8DLwowysFHYj3hhLZiZ340YNXSSQyJ77Bj/YZ5gj?=
 =?us-ascii?Q?9lvd5XDUNylP/be0dvsArVIERe6Y44a2x3ZI66lVJcRKCwUNABKsjZ1NmFU3?=
 =?us-ascii?Q?pwE/MEUo7AU8dDWdLCGN37g+yXyBZFuIqAS0aYJ5FGhNS5eNDVienCT/QCWF?=
 =?us-ascii?Q?guinrJiywl2853sDFVtUFYynUOxzcNW2xcXpvYo3PkJFxA2l87UKBleX9rPb?=
 =?us-ascii?Q?3/PfPuLxwNZvBdZyudWTGtOOSdPIsknQR602i6gapzfK7OwmGiKufMoEnUiH?=
 =?us-ascii?Q?TiBlerz/z8HpMqdLvhtdnyftIIiIiefGl1Cbi17an7YZaudMgB2dwA8y+rp3?=
 =?us-ascii?Q?CetSmtmxAlvSv90+5L8r8Sm9oymCsZ0t4YybNrToXJ7nfc8vfYCs9B55/RgE?=
 =?us-ascii?Q?jgdLp0IMVCmpYgqKw44ZlzZ9iJAKku578k6JvJEJlmy8WV7lIT7TbdG7hvC7?=
 =?us-ascii?Q?xrvgDSF8A16MKFdJRpkBA/pP18maTpLi6Ga2qEE9KiGsEG21wEeXOepIZC4J?=
 =?us-ascii?Q?AxIegi+ei0nEvm5VbzZoTCNxrBQ8VrW30jDOhHAzicezPObqbNpOum80k8B+?=
 =?us-ascii?Q?apfTR9R/sZVzKftsfogzfOAzFS3mOG95DXlBJ/XbQ4sgiNa8xyDcM5Z5KZMT?=
 =?us-ascii?Q?Y9657zxtTZp+PgS01P0Be1atAbkQ2lZYOnu7GzqWxQ6qCkIZoAJi1aepzcmE?=
 =?us-ascii?Q?q6ecHEwzJz1OloVvO3ulxMOCKw/tDMbmeDLNvCkKmhId2KOr24pkmtM+EmT0?=
 =?us-ascii?Q?O0k4f+IoOhLnXlaFIGrvFvqTvR/2flcbVsarCXX3pC1mt8m/X3DVi5AZiT1I?=
 =?us-ascii?Q?6yeCiYcy2UhK8lEa7EyQEesqF+XEscKiDnjdKimncjEo3fVKUytiQZN5luMX?=
 =?us-ascii?Q?K3a9lxQZXrpSH8J1m6EQJencr4f11bvC63TGLmRQkTlal0RGd16P9LOQR8pm?=
 =?us-ascii?Q?9uw7qUtwVnHAoooMbE86+cAGM1uTfoaP+agwIaedSi9GdqJl9ExdS5yOtza5?=
 =?us-ascii?Q?aSpQ7C5FJqSujvGtFHiLFLHNp6FNuwxcGxst682Ymc/fatIKpTA6gc7oyNRn?=
 =?us-ascii?Q?wJ9Vsg2xnNLgl5Du4KF9K9aJjCkbxeXWj3wKo9mFNsHwdlPdmYOxwyOVDe8c?=
 =?us-ascii?Q?O0Vl6G7baH66nHHKWeoJ6/BtQvRu1zi6BWu7AHRBY3RFcX30wp8bRBNSmVh+?=
 =?us-ascii?Q?18Sy9DWI9Vz0yO6MnlzpdMBPI9VdPGx3s3bWriVVasiBjkGnX0kVvMtwI/2E?=
 =?us-ascii?Q?lpPAPhmdqNRpuS5n0IgQyPM9RLUSda53+XBMhKDHqXM+BOKhdLrHMQwiSG7Z?=
 =?us-ascii?Q?3g6ltKAaCyQVvYLFUB7wEI7mCkVeeLq8ARcpMeEi4iZt+jsBRK6F6nqt2HP6?=
 =?us-ascii?Q?Nq7MSaSfPyrD0W+d3NSCv3bt4idHaSye++Dzg+9NDjAaS+2KHSgSNl2rk7dZ?=
 =?us-ascii?Q?lQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <07ED3409A906C945B9B1D5BC730DD908@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: qxr383XOBhSWOJ7vAWlnhrfQlYYRlMibqU6KR2KrSI6VFMLdIoDTlY/lzWh9JGaoj5I/vFXV2GvNQGodrhe0bvW0ZikGZ+ES8AVqrMJEZFHdOVen5Fm1QQbT+rZrPLy8diTEge4fUzsjL3/lhrt0/HRDtHY6Z/nTzNJUZCsY/h9ZXQrZmev0Rw83H+g5zLu7mYx8Wf2+VlMZjXqL+3/sHMyr5lVAHD1jejD8CGqlK5JtMDJziik8PyRKStqX+OCgwXHTKhMvO5K5qhthfGt0VUMAnKl0K9A2BlG0of38Or+BwzKAxDFlmSrcTHrgxOrS2v1mP8k/SJu6wsaDyA/DE0cBeJV1Iu789b3eXQUHAs9KzF4QLSWYf6HUdzLjCnS/U9Xj5UU8TuQyXz3PfdyYD44CaQxkJ5aLGy6xUpSJfbKY1RFewHsEvzz5354c38NYC0HndNqBg1vkUl35ObVjGreuskyNrakirxxrfiFmk/A/u6fQMAUtJDqgEFWAeZGXJKZE6MswSjD79HmSMUhIu6sepuQRTLofWefptodP7Jx7b9ZKXUWI2EH+qxYg3sqx0jhDszirJtXjssPjfBbwnbjGZFI0WUp8dNzpYsVc8imX4CGCwrjEUv4K8TRvqmfc0rfqOIQhbrbJnaJ++CP5iuGKIordZvxYkb7AzBdl3SWB79hqElL/GAexr39eKwjcKg/DBfflzQ4yQX/gGkPTrgTdKpj/OT+Q34niP9xcmlUMvXyAbMLYxQyhOWyXErCBx+okjdDpZ9K3lyQLOs1BSIGN58qo22i8SDfmQOj1skeQKggPFAgl4b/X4sMEuYdd
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: 7e055a97-909e-40da-b13c-08db0555794e
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Feb 2023 19:41:26.8304
 (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: 7WIiQwd5WanreOb0xwBZwMmrczoC5iSEQPEJ/GxT61OwZ6f1L91zCXFFBCfD/MDkNT5w9Nvr/6ObrFrIry0HiA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB7513
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-02_14,2023-02-02_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 bulkscore=0
 suspectscore=0 mlxlogscore=999 mlxscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2302020175
X-Proofpoint-ORIG-GUID: lT80qK53PCADbXGYQLeYg98Qne7mY3af
X-Proofpoint-GUID: lT80qK53PCADbXGYQLeYg98Qne7mY3af
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> wrote:
>=20
> If the upgrading deny mode is invalid or conflicts with other client, we
> should try to resolve it, but the if-condition makes those error handling
> cannot be executed.
>=20
> Signed-off-by: Pumpkin <cc85nod@gmail.com>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4ef529379..ebdfaf0f9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5298,7 +5298,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nf=
s4_file *fp,
> 	/* test and set deny mode */
> 	spin_lock(&fp->fi_lock);
> 	status =3D nfs4_file_check_deny(fp, open->op_share_deny);
> -	if (status =3D=3D nfs_ok) {
> +	if (status !=3D nfs_ok) {
> 		if (status !=3D nfserr_share_denied) {

if status =3D=3D nfs_ok then status will definitely not equal
share_denied. So this check is a bit nonsensical as it stands.

Usually I prefer "switch (status)" in situations like this
because that avoids this kind of issue and I find it easier
to read quickly.

Jeff, you are the original author of this function, and
Dai, your commit is the last one to touch this area. Can
you guys have a look? The one-liner looks correct, but I
might be missing something.


> 			set_deny(open->op_share_deny, stp);
> 			fp->fi_share_deny |=3D
> --=20
> 2.37.1 (Apple Git-137.1)
>=20

--
Chuck Lever