MIME-Version: 1.0
References: <20230314102058.10761-1-jlayton@kernel.org> <CAOQ4uxgrgNAJHGjqo1B05aEa5zXO+UYUPNUHfJA6GKLoiPfUUw@mail.gmail.com>
In-Reply-To: <CAOQ4uxgrgNAJHGjqo1B05aEa5zXO+UYUPNUHfJA6GKLoiPfUUw@mail.gmail.com>
From:   Anna Schumaker <anna@kernel.org>
Date:   Tue, 14 Mar 2023 15:50:28 -0400
Message-ID: <CAFX2JfnZ9KBp3nJPoCT1tte+iL-_Lz8GD=2yo1SHzSdFsQMJ3A@mail.gmail.com>
Subject: Re: [PATCH] lockd: set file_lock start and end when decoding nlm4 testargs
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com,
        trond.myklebust@hammerspace.com, linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 14, 2023 at 7:50=E2=80=AFAM Amir Goldstein <amir73il@gmail.com>=
 wrote:
>
> On Tue, Mar 14, 2023 at 12:21=E2=80=AFPM Jeff Layton <jlayton@kernel.org>=
 wrote:
> >
> > Commit 6930bcbfb6ce dropped the setting of the file_lock range when
> > decoding a nlm_lock off the wire. This causes the client side grant
> > callback to miss matching blocks and reject the lock, only to rerequest
> > it 30s later.
> >
> > Add a helper function to set the file_lock range from the start and end
> > values that the protocol uses, and have the nlm_lock decoder call that =
to
> > set up the file_lock args properly.
> >
> > Fixes: 6930bcbfb6ce ("lockd: detect and reject lock arguments that over=
flow")
> > Reported-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/lockd/clnt4xdr.c        |  9 +--------
> >  fs/lockd/xdr4.c            | 13 ++++++++++++-
> >  include/linux/lockd/xdr4.h |  1 +
> >  3 files changed, 14 insertions(+), 9 deletions(-)
> >
> > Hi Amir,
> >
> > Thanks for the bug report. This patch seems to fix up the 30s stalls fo=
r
> > me. Can you give it a spin and see if it also helps you? I am still
>
> Works like a charm :)
>
> I tested a backport to 5.15.y (trivial conflict in h file).
> against 5.10.109 and 5.15.88 servers.
>
> Tested-by: Amir Goldstein <amir73il@gmail.com>
>
> > seeing one test failure with nfstest_lock, but I'm not sure it's relate=
d
> > to this. I'll plan to follow up with Jorge.
>
> I don't see this failure in my tests.
>
> Please fast track this fix which also fixes an LTS 5.15.y kernel regressi=
on.
> Please cc stable and let me know if you want me to post the 5.15.y backpo=
rt.

Thanks Jeff and Amir! I have the patch queued up, and I'll do a
bugfixes pull request later this week.

Anna

>
> Thanks for the prompt fix,
> Amir.
>
> >
> > Thanks,
> > Jeff
> >
> > diff --git a/fs/lockd/clnt4xdr.c b/fs/lockd/clnt4xdr.c
> > index 7df6324ccb8a..8161667c976f 100644
> > --- a/fs/lockd/clnt4xdr.c
> > +++ b/fs/lockd/clnt4xdr.c
> > @@ -261,7 +261,6 @@ static int decode_nlm4_holder(struct xdr_stream *xd=
r, struct nlm_res *result)
> >         u32 exclusive;
> >         int error;
> >         __be32 *p;
> > -       s32 end;
> >
> >         memset(lock, 0, sizeof(*lock));
> >         locks_init_lock(fl);
> > @@ -285,13 +284,7 @@ static int decode_nlm4_holder(struct xdr_stream *x=
dr, struct nlm_res *result)
> >         fl->fl_type  =3D exclusive !=3D 0 ? F_WRLCK : F_RDLCK;
> >         p =3D xdr_decode_hyper(p, &l_offset);
> >         xdr_decode_hyper(p, &l_len);
> > -       end =3D l_offset + l_len - 1;
> > -
> > -       fl->fl_start =3D (loff_t)l_offset;
> > -       if (l_len =3D=3D 0 || end < 0)
> > -               fl->fl_end =3D OFFSET_MAX;
> > -       else
> > -               fl->fl_end =3D (loff_t)end;
> > +       nlm4svc_set_file_lock_range(fl, l_offset, l_len);
> >         error =3D 0;
> >  out:
> >         return error;
> > diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> > index 712fdfeb8ef0..5fcbf30cd275 100644
> > --- a/fs/lockd/xdr4.c
> > +++ b/fs/lockd/xdr4.c
> > @@ -33,6 +33,17 @@ loff_t_to_s64(loff_t offset)
> >         return res;
> >  }
> >
> > +void nlm4svc_set_file_lock_range(struct file_lock *fl, u64 off, u64 le=
n)
> > +{
> > +       s64 end =3D off + len - 1;
> > +
> > +       fl->fl_start =3D off;
> > +       if (len =3D=3D 0 || end < 0)
> > +               fl->fl_end =3D OFFSET_MAX;
> > +       else
> > +               fl->fl_end =3D end;
> > +}
> > +
> >  /*
> >   * NLM file handles are defined by specification to be a variable-leng=
th
> >   * XDR opaque no longer than 1024 bytes. However, this implementation
> > @@ -80,7 +91,7 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm=
_lock *lock)
> >         locks_init_lock(fl);
> >         fl->fl_flags =3D FL_POSIX;
> >         fl->fl_type  =3D F_RDLCK;
> > -
> > +       nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_le=
n);
> >         return true;
> >  }
> >
> > diff --git a/include/linux/lockd/xdr4.h b/include/linux/lockd/xdr4.h
> > index 9a6b55da8fd6..72831e35dca3 100644
> > --- a/include/linux/lockd/xdr4.h
> > +++ b/include/linux/lockd/xdr4.h
> > @@ -22,6 +22,7 @@
> >  #define        nlm4_fbig               cpu_to_be32(NLM_FBIG)
> >  #define        nlm4_failed             cpu_to_be32(NLM_FAILED)
> >
> > +void   nlm4svc_set_file_lock_range(struct file_lock *fl, u64 off, u64 =
len);
> >  bool   nlm4svc_decode_void(struct svc_rqst *rqstp, struct xdr_stream *=
xdr);
> >  bool   nlm4svc_decode_testargs(struct svc_rqst *rqstp, struct xdr_stre=
am *xdr);
> >  bool   nlm4svc_decode_lockargs(struct svc_rqst *rqstp, struct xdr_stre=
am *xdr);
> > --
> > 2.39.2
> >