linux-kernel - Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <860cb5d7dfea29b4beaee57dd152137837696cb9.camel@kernel.org>
Date:   Fri, 03 Feb 2023 13:10:44 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     Dai Ngo <dai.ngo@...cle.com>, Pumpkin <cc85nod@...il.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open

On Fri, 2023-02-03 at 14:50 +0000, Chuck Lever III wrote:
> 
> > On Feb 2, 2023, at 4:22 PM, Jeff Layton <jlayton@...nel.org> wrote:
> > 
> > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
> > > 
> > > > On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@...il.com> wrote:
> > > > 
> > > > 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.
> > > > 
> > > > Signed-off-by: Pumpkin <cc85nod@...il.com>
> > > > ---
> > > > fs/nfsd/nfs4state.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > 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 = nfs4_file_check_deny(fp, open->op_share_deny);
> > > > -	if (status == nfs_ok) {
> > > > +	if (status != nfs_ok) {
> > > > 		if (status != nfserr_share_denied) {
> > > 
> > > if status == 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.
> > > 
> > 
> > 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 nfs4_file *fp,
> >        /* test and set deny mode */
> >        spin_lock(&fp->fi_lock);
> >        status = nfs4_file_check_deny(fp, open->op_share_deny);
> > -       if (status == nfs_ok) {
> > -               if (status != nfserr_share_denied) {
> > -                       set_deny(open->op_share_deny, stp);
> > -                       fp->fi_share_deny |=
> > -                               (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
> > -               } else {
> > -                       if (nfs4_resolve_deny_conflicts_locked(fp, false,
> > -                                       stp, open->op_share_deny, false))
> > -                               status = nfserr_jukebox;
> > -               }
> > +       switch (status) {
> > +       case nfs_ok:
> > +               set_deny(open->op_share_deny, stp);
> > +               fp->fi_share_deny |=
> > +                       (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 = nfserr_jukebox;
> > +               break;
> >        }
> >        spin_unlock(&fp->fi_lock);
> 
> Would pynfs have a case or two that could test this?
> 
> Can you post an official version of this patch with Reported-by
> and Fixes tags?
> 
> 

Sure, but I may not have time to test it out for a bit though.
-- 
Jeff Layton <jlayton@...nel.org>