diff mbox series

lockd: set file_lock start and end when decoding nlm4 testargs

Message ID 20230314102058.10761-1-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series lockd: set file_lock start and end when decoding nlm4 testargs | expand

Commit Message

Jeffrey Layton March 14, 2023, 10:20 a.m. UTC 
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 overflow")
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 for
me. Can you give it a spin and see if it also helps you? I am still
seeing one test failure with nfstest_lock, but I'm not sure it's related
to this. I'll plan to follow up with Jorge.

Thanks,
Jeff

Comments

Amir Goldstein March 14, 2023, 11:49 a.m. UTC | #1 
On Tue, Mar 14, 2023 at 12:21 PM 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 overflow")
> 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 for
> 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 related
> 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 regression.
Please cc stable and let me know if you want me to post the 5.15.y backport.

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 *xdr, 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 *xdr, struct nlm_res *result)
>         fl->fl_type  = exclusive != 0 ? F_WRLCK : F_RDLCK;
>         p = xdr_decode_hyper(p, &l_offset);
>         xdr_decode_hyper(p, &l_len);
> -       end = l_offset + l_len - 1;
> -
> -       fl->fl_start = (loff_t)l_offset;
> -       if (l_len == 0 || end < 0)
> -               fl->fl_end = OFFSET_MAX;
> -       else
> -               fl->fl_end = (loff_t)end;
> +       nlm4svc_set_file_lock_range(fl, l_offset, l_len);
>         error = 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 len)
> +{
> +       s64 end = off + len - 1;
> +
> +       fl->fl_start = off;
> +       if (len == 0 || end < 0)
> +               fl->fl_end = OFFSET_MAX;
> +       else
> +               fl->fl_end = end;
> +}
> +
>  /*
>   * NLM file handles are defined by specification to be a variable-length
>   * 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 = FL_POSIX;
>         fl->fl_type  = F_RDLCK;
> -
> +       nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
>         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_stream *xdr);
>  bool   nlm4svc_decode_lockargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
> --
> 2.39.2
>
Anna Schumaker March 14, 2023, 7:50 p.m. UTC | #2 
On Tue, Mar 14, 2023 at 7:50 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Mar 14, 2023 at 12:21 PM 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 overflow")
> > 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 for
> > 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 related
> > 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 regression.
> Please cc stable and let me know if you want me to post the 5.15.y backport.

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 *xdr, 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 *xdr, struct nlm_res *result)
> >         fl->fl_type  = exclusive != 0 ? F_WRLCK : F_RDLCK;
> >         p = xdr_decode_hyper(p, &l_offset);
> >         xdr_decode_hyper(p, &l_len);
> > -       end = l_offset + l_len - 1;
> > -
> > -       fl->fl_start = (loff_t)l_offset;
> > -       if (l_len == 0 || end < 0)
> > -               fl->fl_end = OFFSET_MAX;
> > -       else
> > -               fl->fl_end = (loff_t)end;
> > +       nlm4svc_set_file_lock_range(fl, l_offset, l_len);
> >         error = 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 len)
> > +{
> > +       s64 end = off + len - 1;
> > +
> > +       fl->fl_start = off;
> > +       if (len == 0 || end < 0)
> > +               fl->fl_end = OFFSET_MAX;
> > +       else
> > +               fl->fl_end = end;
> > +}
> > +
> >  /*
> >   * NLM file handles are defined by specification to be a variable-length
> >   * 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 = FL_POSIX;
> >         fl->fl_type  = F_RDLCK;
> > -
> > +       nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
> >         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_stream *xdr);
> >  bool   nlm4svc_decode_lockargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
> > --
> > 2.39.2
> >
diff mbox series

Patch

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 *xdr, 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 *xdr, struct nlm_res *result)
 	fl->fl_type  = exclusive != 0 ? F_WRLCK : F_RDLCK;
 	p = xdr_decode_hyper(p, &l_offset);
 	xdr_decode_hyper(p, &l_len);
-	end = l_offset + l_len - 1;
-
-	fl->fl_start = (loff_t)l_offset;
-	if (l_len == 0 || end < 0)
-		fl->fl_end = OFFSET_MAX;
-	else
-		fl->fl_end = (loff_t)end;
+	nlm4svc_set_file_lock_range(fl, l_offset, l_len);
 	error = 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 len)
+{
+	s64 end = off + len - 1;
+
+	fl->fl_start = off;
+	if (len == 0 || end < 0)
+		fl->fl_end = OFFSET_MAX;
+	else
+		fl->fl_end = end;
+}
+
 /*
  * NLM file handles are defined by specification to be a variable-length
  * 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 = FL_POSIX;
 	fl->fl_type  = F_RDLCK;
-
+	nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
 	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_stream *xdr);
 bool	nlm4svc_decode_lockargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);