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:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Dai Ngo <dai.ngo@oracle.com>
CC:     Jeff Layton <jlayton@kernel.org>,
        Bruce Fields <bfields@fieldses.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NFSD: Fix the share reservation conflict to courteous
 server logic in nfs4_upgrade_open()
Thread-Topic: [PATCH] NFSD: Fix the share reservation conflict to courteous
 server logic in nfs4_upgrade_open()
Thread-Index: AQHY7DD5/V7HQv9STUORmP83olqFpa4nD5EA
Date:   Sun, 30 Oct 2022 15:26:27 +0000
Message-ID: <FB543B1D-A3BF-4403-9BFD-CE3AA40A89B5@oracle.com>
References: <7ed2d8f1ee8c441a13b450c5e5c50f13fae3a2b9.1667114760.git.christophe.jaillet@wanadoo.fr>
In-Reply-To: <7ed2d8f1ee8c441a13b450c5e5c50f13fae3a2b9.1667114760.git.christophe.jaillet@wanadoo.fr>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?9ffrjn71NqQVzB2f9DUzqZd79Rk4gMR+4hTLCc30/m/vIG7VUOgp4zysNsaJ?=
 =?us-ascii?Q?z/kGZ4Vz+YRW9n+SyE1juv6jpFmrHYZ8NS5sytDL4TOy3YgE6oogCO6z6KVm?=
 =?us-ascii?Q?N5V3CBaetfmqIRSmARi2NpeERw282pMJFJ8BpvqU51kWMlaEfTOPo03RVMkq?=
 =?us-ascii?Q?KpCHvNlR3DiSZzASoGR7Ka/93zgAsMu5kSdTuuE8twIydMAiyVqyStw43IvH?=
 =?us-ascii?Q?SaBtLdcx+rX+mtpXp4ks1548g4gfMsSwgYVcEcg/sNPlKhaYp3TLifHaMHac?=
 =?us-ascii?Q?YgH3OGdR9g/1YjcFDpRs6P5QqFk0wGZjHgw9E620nZm14H7/HNmyTMJ2WuvX?=
 =?us-ascii?Q?N/n87Z5B64XroKx1x/rZeGt0hZl0xyzHZ6ObiS2HA9l5Pss6CiXEIDHcgHF/?=
 =?us-ascii?Q?pMQnMnh0N1SUUNyCia7bezhwQS611HCTxUGo5uSQtUQ5/lZAZMqv31gj9EP9?=
 =?us-ascii?Q?xo1BqB5A+Yxn0QmBJKJ4Ro84iiBK6fY7yxrIF7J+M7Rnp9JnP+0+hjwKP+t3?=
 =?us-ascii?Q?soP8icXHhOqOybGR/um+U73scHdrS+cqsPNT/WSfbccjQqcQ4DWZ2vP5Y6JH?=
 =?us-ascii?Q?6QRrKjYWanOebwbcnQel0x4SdztSkZ1YSqUvjI6j3MwgzrwJHeXC0oAK8+x0?=
 =?us-ascii?Q?NYNKreI9Sapnc6jsOd6oThJ9lHsltUq4OTzY0XbQ1U6w2jp8eVbOgxEIiSvJ?=
 =?us-ascii?Q?KWZmPWLEEhdpiR9kAptTvOIhtS1GByPy6Msh3ntoVOTGubJIzDXz5bfmq9HJ?=
 =?us-ascii?Q?9aw75sHDwkrfIneXrRM6LF1/0wp0hG2ieQU8gNC6Y6ZGsadQVj0DFLVRVOlx?=
 =?us-ascii?Q?5IwjHqk5McgvS++qJyVwawAhPGS4iM6p/teNn3U4qM1R2QiL6itAP95QbfGr?=
 =?us-ascii?Q?2XMNsmydyK2W8ociKfyleJaTZ903VDmAHI0CuQkzybwT2R6G4hu79lm+hwO3?=
 =?us-ascii?Q?iaPUp+fgv2CkR9l4VGW+K+HlRsx9rikCDVrYr3+oLDpqhxwnIejzucTFJW7x?=
 =?us-ascii?Q?/zSPV6Mzd+w7FdpSghGXOlxBC5c4V4pANTAM3jSU1SHUdv40CwT7JnjeBgrM?=
 =?us-ascii?Q?A1bRx5/juJ597vZoXI414oR4EgWj1gXTYW4sU/d/QSyViVBud6KTDn3KWmrv?=
 =?us-ascii?Q?k0KH6zpZVr4aHWXnyHhaBVD/Ge0nAOhY6BQ0QYqj+P2fpdmBn599Jt7+EFJK?=
 =?us-ascii?Q?3DTFHC9hnVlXsFAB/c29oSuqfDiIv3yfl1xYB7XdDL+opbCYIf0wtJa3/UkC?=
 =?us-ascii?Q?xh8nCyq5eMAtsA4wRdUGpGRRKc+t9Kb43zAOScTvYkgnYftVB8E0PDFlsotL?=
 =?us-ascii?Q?RjEuNAlZaRKM//CnjvRFzHaVdogTJ+j0E2Q5MZrkByQNmjSZ4+9Mwpi/gvSu?=
 =?us-ascii?Q?drolcrLgC13hp9Nk/pU2uKbCDgAliFV23NqB5GbtQ1JlgWavK13XbMlgITOG?=
 =?us-ascii?Q?jKKy6ZnWiHiQzK7KnBoUR1VnfiZqbwA0Y/2dj9k/P5orl/TT7SJk3mIU7fQ2?=
 =?us-ascii?Q?S2LST/tJF+SUhR8ySuMQdmTzaJwEqpF7LABzeh/h+b0R3OXCk1yo0CEFr9ZJ?=
 =?us-ascii?Q?paUUM9Xrgtx86T+7QaJ9wjO5/GklNt5KbREdZagDzHgipGhfN/jTjeEyjcIB?=
 =?us-ascii?Q?5g=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3A7DE69FB9EB994CA233BFBD95C6EC0C@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: aaba3ab9-240e-4831-b00e-08daba8b1cfc
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Oct 2022 15:26:27.5419
 (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: grHtFb+Cc/tGrhgtDan9WdvDtoCQU5eXk77CKbCjDqLCWBrOBrWp+YMbprcPxp9KeR391knBCRn4De2Vjc92Hg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5222
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-30_08,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 suspectscore=0
 phishscore=0 malwarescore=0 spamscore=0 mlxscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2210300101
X-Proofpoint-GUID: uxK0ODfw2rxdeh8oeOHelm8NIKI0M0FR
X-Proofpoint-ORIG-GUID: uxK0ODfw2rxdeh8oeOHelm8NIKI0M0FR
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 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 Oct 30, 2022, at 3:26 AM, Christophe JAILLET <christophe.jaillet@wanad=
oo.fr> wrote:
>=20
> 'status !=3D nfserr_share_denied' is known to be true because we test
> 'status =3D=3D nfs_ok' the line just above.
>=20
> So nfs4_resolve_deny_conflicts_locked() can never be called.
>=20
> Fix the logic and avoid the dead code.
>=20
> Fixes: 3d6942715180 ("NFSD: add support for share reservation conflict to=
 courteous server")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is speculative.
> It is compile tested only.
>=20
> REVIEW WITH CARE.
> ---
> fs/nfsd/nfs4state.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1ded89235111..de0565e9485c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5260,15 +5260,13 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct =
nfs4_file *fp,
> 	spin_lock(&fp->fi_lock);
> 	status =3D nfs4_file_check_deny(fp, open->op_share_deny);

I agree there's dead code here. I believe the bug is the first check is
supposed to be "if (status !=3D nfs_ok)". I will let Dai have a look at
this to confirm.

But, in the fix, let's replace this logic with "switch (status) { }".


> 	if (status =3D=3D nfs_ok) {
> -		if (status !=3D nfserr_share_denied) {
> -			set_deny(open->op_share_deny, stp);
> -			fp->fi_share_deny |=3D
> +		set_deny(open->op_share_deny, stp);
> +		fp->fi_share_deny |=3D
> 				(open->op_share_deny & NFS4_SHARE_DENY_BOTH);
> -		} else {
> -			if (nfs4_resolve_deny_conflicts_locked(fp, false,
> -					stp, open->op_share_deny, false))
> -				status =3D nfserr_jukebox;
> -		}
> +	} else if (status =3D=3D nfserr_share_denied) {
> +		if (nfs4_resolve_deny_conflicts_locked(fp, false, stp,
> +				open->op_share_deny, false))
> +			status =3D nfserr_jukebox;
> 	}
> 	spin_unlock(&fp->fi_lock);
>=20
> --=20
> 2.34.1
>=20

--
Chuck Lever