Message-ID: <7b7edc8bacb863915d00673f9e03d38798341a69.camel@kernel.org>
Subject: Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>,
        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>
Date:   Thu, 02 Feb 2023 16:22:44 -0500
In-Reply-To: <CBAFB3E5-5737-47A7-8234-3A771E908C4F@oracle.com>
References: <20230202073611.13106-1-cc85nod@gmail.com>
         <CBAFB3E5-5737-47A7-8234-3A771E908C4F@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>=20
> > 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, w=
e
> > should try to resolve it, but the if-condition makes those error handli=
ng
> > 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 =
nfs4_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) {
>=20
> if status =3D=3D nfs_ok then status will definitely not equal
> share_denied. So this check is a bit nonsensical as it stands.
>=20
> Usually I prefer "switch (status)" in situations like this
> because that avoids this kind of issue and I find it easier
> to read quickly.
>=20
> 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.
>=20

Yeah, that code is clearly broken and it looks like it was done in
3d69427151806 (NFSD: add support for share reservation conflict to
courteous server).

I don't believe that one-liner is correct though. If the result is
nfs_ok, then we want to set the deny mode here and that won't happen.

Something like this maybe? (completely untested):

---------------8<-------------------

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c39e43742dd6..af22dfdc6fcc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5282,16 +5282,17 @@ 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 nfserr_share_denied) {
-                       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;
-               }
+       switch (status) {
+       case nfs_ok:
+               set_deny(open->op_share_deny, stp);
+               fp->fi_share_deny |=3D
+                       (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+               break;
+       case nfserr_share_denied:
+               if (nfs4_resolve_deny_conflicts_locked(fp, false,
+                               stp, open->op_share_deny, false))
+                       status =3D nfserr_jukebox;
+               break;
        }
        spin_unlock(&fp->fi_lock);

--=20
Jeff Layton <jlayton@kernel.org>