[PATCH 0/3] reexport lock fixes

[PATCH 0/3] reexport lock fixes

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The following fix up some problems that can cause crashes or silently
broken lock guarantees in the reexport case.

Not fixed:
	- Attempts to reclaim locks after a reboot of the reexport
	  server will fail.  This at least seems like an improvement
	  over the current situation, which is that they'll succeed even
	  in cases where they shouldn't.  Complete support for reboot
	  recovery is a bigger job.

	- NFSv4.1+ lock nofications don't work.  So, clients have to
	  poll as they do with NFSv4.0, which is suboptimal, but correct
	  (and an improvement over the current situation, which is a
	  kernel oops).

So what we have at this point is a suboptimal lock implementation that
doesn't support lock recovery.

Another alternative might be to turn off file locking entirely in the
re-export case.  I'd rather take the incremental improvement and fix the
oopses.

--b.

J. Bruce Fields (3):
  nfs: don't atempt blocking locks on nfs reexports
  lockd: lockd server-side shouldn't set fl_ops
  nfs: don't allow reexport reclaims

 fs/lockd/svclock.c       | 30 ++++++++++++------------------
 fs/nfs/export.c          |  2 +-
 fs/nfs/file.c            |  3 +++
 fs/nfsd/nfs4state.c      | 11 +++++++++--
 fs/nfsd/nfsproc.c        |  1 +
 include/linux/exportfs.h |  2 ++
 include/linux/fs.h       |  1 +
 7 files changed, 29 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/3] nfs: don't atempt blocking locks on nfs reexports

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

NFS implements blocking locks by blocking inside its lock method.  In
the reexport case, this blocks the nfs server thread, which could lead
to deadlocks since an nfs server thread might be required to unlock the
conflicting lock.  It also causes a crash, since the nfs server thread
assumes it can free the lock when its lm_notify lock callback is called.

Ideal would be to make the nfs lock method return without blocking in
this case, but for now it works just not to attempt blocking locks.  The
difference is just that the original client will have to poll (as it
does in the v4.0 case) instead of getting a callback when the lock's
available.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/export.c          | 2 +-
 fs/nfsd/nfs4state.c      | 8 ++++++--
 include/linux/exportfs.h | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/export.c b/fs/nfs/export.c
index 37a1a88df771..d772c20bbfd1 100644
--- a/fs/nfs/export.c
+++ b/fs/nfs/export.c
@@ -180,5 +180,5 @@ const struct export_operations nfs_export_ops = {
 	.fetch_iversion = nfs_fetch_iversion,
 	.flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK|
 		EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS|
-		EXPORT_OP_NOATOMIC_ATTR,
+		EXPORT_OP_NOATOMIC_ATTR|EXPORT_OP_SYNC_LOCKS,
 };
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 980bd8903f84..00d98bbab2a6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6835,6 +6835,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfsd4_blocked_lock *nbl = NULL;
 	struct file_lock *file_lock = NULL;
 	struct file_lock *conflock = NULL;
+	struct super_block *sb;
 	__be32 status = 0;
 	int lkflg;
 	int err;
@@ -6856,6 +6857,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		dprintk("NFSD: nfsd4_lock: permission denied!\n");
 		return status;
 	}
+	sb = cstate->current_fh.fh_dentry->d_sb;
 
 	if (lock->lk_is_new) {
 		if (nfsd4_has_session(cstate))
@@ -6904,7 +6906,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	fp = lock_stp->st_stid.sc_file;
 	switch (lock->lk_type) {
 		case NFS4_READW_LT:
-			if (nfsd4_has_session(cstate))
+			if (nfsd4_has_session(cstate) &&
+			    !(sb->s_export_op->flags & EXPORT_OP_SYNC_LOCKS))
 				fl_flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_READ_LT:
@@ -6916,7 +6919,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			fl_type = F_RDLCK;
 			break;
 		case NFS4_WRITEW_LT:
-			if (nfsd4_has_session(cstate))
+			if (nfsd4_has_session(cstate) &&
+			    !(sb->s_export_op->flags & EXPORT_OP_SYNC_LOCKS))
 				fl_flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_WRITE_LT:
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index fe848901fcc3..3260fe714846 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -221,6 +221,8 @@ struct export_operations {
 #define EXPORT_OP_NOATOMIC_ATTR		(0x10) /* Filesystem cannot supply
 						  atomic attribute updates
 						*/
+#define EXPORT_OP_SYNC_LOCKS		(0x20) /* Filesystem can't do
+						  asychronous blocking locks */
 	unsigned long	flags;
 };
 
-- 
2.31.1

[PATCH 2/3] lockd: lockd server-side shouldn't set fl_ops

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Locks have two sets of op arrays, fl_lmops for the lock manager (lockd
or nfsd), fl_ops for the filesystem.  The server-side lockd code has
been setting its own fl_ops, which leads to confusion (and crashes) in
the reexport case, where the filesystem expects to be the only one
setting fl_ops.

And there's no reason for it that I can see-the lm_get/put_owner ops do
the same job.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svclock.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 61d3cc2283dc..1781fc5e9091 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -395,28 +395,10 @@ nlmsvc_release_lockowner(struct nlm_lock *lock)
 		nlmsvc_put_lockowner(lock->fl.fl_owner);
 }
 
-static void nlmsvc_locks_copy_lock(struct file_lock *new, struct file_lock *fl)
-{
-	struct nlm_lockowner *nlm_lo = (struct nlm_lockowner *)fl->fl_owner;
-	new->fl_owner = nlmsvc_get_lockowner(nlm_lo);
-}
-
-static void nlmsvc_locks_release_private(struct file_lock *fl)
-{
-	nlmsvc_put_lockowner((struct nlm_lockowner *)fl->fl_owner);
-}
-
-static const struct file_lock_operations nlmsvc_lock_ops = {
-	.fl_copy_lock = nlmsvc_locks_copy_lock,
-	.fl_release_private = nlmsvc_locks_release_private,
-};
-
 void nlmsvc_locks_init_private(struct file_lock *fl, struct nlm_host *host,
 						pid_t pid)
 {
 	fl->fl_owner = nlmsvc_find_lockowner(host, pid);
-	if (fl->fl_owner != NULL)
-		fl->fl_ops = &nlmsvc_lock_ops;
 }
 
 /*
@@ -788,9 +770,21 @@ nlmsvc_notify_blocked(struct file_lock *fl)
 	printk(KERN_WARNING "lockd: notification for unknown block!\n");
 }
 
+static fl_owner_t nlmsvc_get_owner(fl_owner_t owner)
+{
+	return nlmsvc_get_lockowner(owner);
+}
+
+static void nlmsvc_put_owner(fl_owner_t owner)
+{
+	nlmsvc_put_lockowner(owner);
+}
+
 const struct lock_manager_operations nlmsvc_lock_operations = {
 	.lm_notify = nlmsvc_notify_blocked,
 	.lm_grant = nlmsvc_grant_deferred,
+	.lm_get_owner = nlmsvc_get_owner,
+	.lm_put_owner = nlmsvc_put_owner,
 };
 
 /*
-- 
2.31.1

[PATCH 3/3] nfs: don't allow reexport reclaims

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

In the reexport case, nfsd is currently passing along locks with the
reclaim bit set.  The client sends a new lock request, which is granted
if there's currently no conflict--even if it's possible a conflicting
lock could have been briefly held in the interim.

We don't currently have any way to safely grant reclaim, so for now
let's just deny them all.

I'm doing this by passing the reclaim bit to nfs and letting it fail the
call, with the idea that eventually the client might be able to do
something more forgiving here.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/file.c       | 3 +++
 fs/nfsd/nfs4state.c | 3 +++
 fs/nfsd/nfsproc.c   | 1 +
 include/linux/fs.h  | 1 +
 4 files changed, 8 insertions(+)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 1fef107961bc..35a29b440e3e 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
 
 	nfs_inc_stats(inode, NFSIOS_VFSLOCK);
 
+	if (fl->fl_flags & FL_RECLAIM)
+		return -NFSERR_NO_GRACE;
+
 	/* No mandatory locks over NFS */
 	if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
 		goto out_err;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 00d98bbab2a6..3ef42c0d5d38 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6903,6 +6903,9 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (!locks_in_grace(net) && lock->lk_reclaim)
 		goto out;
 
+	if (lock->lk_reclaim)
+		fl_flags |= FL_RECLAIM;
+
 	fp = lock_stp->st_stid.sc_file;
 	switch (lock->lk_type) {
 		case NFS4_READW_LT:
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 60d7c59e7935..80c430c37ab7 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -881,6 +881,7 @@ nfserrno (int errno)
 		{ nfserr_serverfault, -ENFILE },
 		{ nfserr_io, -EUCLEAN },
 		{ nfserr_perm, -ENOKEY },
+		{ nfserr_no_grace, -NFSERR_NO_GRACE},
 	};
 	int	i;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c3c88fdb9b2a..9be479999109 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -997,6 +997,7 @@ static inline struct file *get_file(struct file *f)
 #define FL_UNLOCK_PENDING	512 /* Lease is being broken */
 #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
 #define FL_LAYOUT	2048	/* outstanding pNFS layout */
+#define FL_RECLAIM	4096	/* reclaiming from a reboot server */
 
 #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
 
-- 
2.31.1

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[Trond Myklebust]

On Mon, 2021-06-14 at 10:48 -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> In the reexport case, nfsd is currently passing along locks with the
> reclaim bit set.  The client sends a new lock request, which is
> granted
> if there's currently no conflict--even if it's possible a conflicting
> lock could have been briefly held in the interim.
> 
> We don't currently have any way to safely grant reclaim, so for now
> let's just deny them all.
> 
> I'm doing this by passing the reclaim bit to nfs and letting it fail
> the
> call, with the idea that eventually the client might be able to do
> something more forgiving here.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfs/file.c       | 3 +++
>  fs/nfsd/nfs4state.c | 3 +++
>  fs/nfsd/nfsproc.c   | 1 +
>  include/linux/fs.h  | 1 +
>  4 files changed, 8 insertions(+)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 1fef107961bc..35a29b440e3e 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd, struct
> file_lock *fl)
>  
>         nfs_inc_stats(inode, NFSIOS_VFSLOCK);
>  
> +       if (fl->fl_flags & FL_RECLAIM)
> +               return -NFSERR_NO_GRACE;

NACK. nfs_lock() is required to return a POSIX error. I know that right
now, nfsd is the only thing setting FL_RECLAIM, but we can't guarantee
that will always be the case.

> +
>         /* No mandatory locks over NFS */
>         if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
>                 goto out_err;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 00d98bbab2a6..3ef42c0d5d38 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6903,6 +6903,9 @@ nfsd4_lock(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
>         if (!locks_in_grace(net) && lock->lk_reclaim)
>                 goto out;
>  
> +       if (lock->lk_reclaim)
> +               fl_flags |= FL_RECLAIM;
> +
>         fp = lock_stp->st_stid.sc_file;
>         switch (lock->lk_type) {
>                 case NFS4_READW_LT:
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 60d7c59e7935..80c430c37ab7 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -881,6 +881,7 @@ nfserrno (int errno)
>                 { nfserr_serverfault, -ENFILE },
>                 { nfserr_io, -EUCLEAN },
>                 { nfserr_perm, -ENOKEY },
> +               { nfserr_no_grace, -NFSERR_NO_GRACE},
>         };
>         int     i;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c3c88fdb9b2a..9be479999109 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -997,6 +997,7 @@ static inline struct file *get_file(struct file
> *f)
>  #define FL_UNLOCK_PENDING      512 /* Lease is being broken */
>  #define FL_OFDLCK      1024    /* lock is "owned" by struct file */
>  #define FL_LAYOUT      2048    /* outstanding pNFS layout */
> +#define FL_RECLAIM     4096    /* reclaiming from a reboot server */
>  
>  #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
>  

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[J. Bruce Fields]

On Mon, Jun 14, 2021 at 02:56:55PM +0000, Trond Myklebust wrote:
> On Mon, 2021-06-14 at 10:48 -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > In the reexport case, nfsd is currently passing along locks with the
> > reclaim bit set.  The client sends a new lock request, which is
> > granted
> > if there's currently no conflict--even if it's possible a conflicting
> > lock could have been briefly held in the interim.
> > 
> > We don't currently have any way to safely grant reclaim, so for now
> > let's just deny them all.
> > 
> > I'm doing this by passing the reclaim bit to nfs and letting it fail
> > the
> > call, with the idea that eventually the client might be able to do
> > something more forgiving here.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfs/file.c       | 3 +++
> >  fs/nfsd/nfs4state.c | 3 +++
> >  fs/nfsd/nfsproc.c   | 1 +
> >  include/linux/fs.h  | 1 +
> >  4 files changed, 8 insertions(+)
> > 
> > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > index 1fef107961bc..35a29b440e3e 100644
> > --- a/fs/nfs/file.c
> > +++ b/fs/nfs/file.c
> > @@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd, struct
> > file_lock *fl)
> >  
> >         nfs_inc_stats(inode, NFSIOS_VFSLOCK);
> >  
> > +       if (fl->fl_flags & FL_RECLAIM)
> > +               return -NFSERR_NO_GRACE;
> 
> NACK. nfs_lock() is required to return a POSIX error. I know that right
> now, nfsd is the only thing setting FL_RECLAIM, but we can't guarantee
> that will always be the case.

Setting FL_RECLAIM tells the filesystem that you're prepared to handle
NFSERR_NO_GRACE.  I'm not seeing the risk.

--b.

> >         /* No mandatory locks over NFS */
> >         if (__mandatory_lock(inode) && fl->fl_type != F_UNLCK)
> >                 goto out_err;
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 00d98bbab2a6..3ef42c0d5d38 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6903,6 +6903,9 @@ nfsd4_lock(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >         if (!locks_in_grace(net) && lock->lk_reclaim)
> >                 goto out;
> >  
> > +       if (lock->lk_reclaim)
> > +               fl_flags |= FL_RECLAIM;
> > +
> >         fp = lock_stp->st_stid.sc_file;
> >         switch (lock->lk_type) {
> >                 case NFS4_READW_LT:
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 60d7c59e7935..80c430c37ab7 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -881,6 +881,7 @@ nfserrno (int errno)
> >                 { nfserr_serverfault, -ENFILE },
> >                 { nfserr_io, -EUCLEAN },
> >                 { nfserr_perm, -ENOKEY },
> > +               { nfserr_no_grace, -NFSERR_NO_GRACE},
> >         };
> >         int     i;
> >  
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index c3c88fdb9b2a..9be479999109 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -997,6 +997,7 @@ static inline struct file *get_file(struct file
> > *f)
> >  #define FL_UNLOCK_PENDING      512 /* Lease is being broken */
> >  #define FL_OFDLCK      1024    /* lock is "owned" by struct file */
> >  #define FL_LAYOUT      2048    /* outstanding pNFS layout */
> > +#define FL_RECLAIM     4096    /* reclaiming from a reboot server */
> >  
> >  #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
> >  
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[Trond Myklebust]

On Mon, 2021-06-14 at 15:34 -0400, J. Bruce Fields wrote:
> On Mon, Jun 14, 2021 at 02:56:55PM +0000, Trond Myklebust wrote:
> > On Mon, 2021-06-14 at 10:48 -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > In the reexport case, nfsd is currently passing along locks with
> > > the
> > > reclaim bit set.  The client sends a new lock request, which is
> > > granted
> > > if there's currently no conflict--even if it's possible a
> > > conflicting
> > > lock could have been briefly held in the interim.
> > > 
> > > We don't currently have any way to safely grant reclaim, so for
> > > now
> > > let's just deny them all.
> > > 
> > > I'm doing this by passing the reclaim bit to nfs and letting it
> > > fail
> > > the
> > > call, with the idea that eventually the client might be able to
> > > do
> > > something more forgiving here.
> > > 
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > ---
> > >  fs/nfs/file.c       | 3 +++
> > >  fs/nfsd/nfs4state.c | 3 +++
> > >  fs/nfsd/nfsproc.c   | 1 +
> > >  include/linux/fs.h  | 1 +
> > >  4 files changed, 8 insertions(+)
> > > 
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index 1fef107961bc..35a29b440e3e 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd,
> > > struct
> > > file_lock *fl)
> > >  
> > >         nfs_inc_stats(inode, NFSIOS_VFSLOCK);
> > >  
> > > +       if (fl->fl_flags & FL_RECLAIM)
> > > +               return -NFSERR_NO_GRACE;
> > 
> > NACK. nfs_lock() is required to return a POSIX error. I know that
> > right
> > now, nfsd is the only thing setting FL_RECLAIM, but we can't
> > guarantee
> > that will always be the case.
> 
> Setting FL_RECLAIM tells the filesystem that you're prepared to
> handle
> NFSERR_NO_GRACE.  I'm not seeing the risk.

You are using a function that is exposed to the VFS. On error, that
function is expected to return a value that is a Linux error between -1
and -4095.

I suggest adding an error value ENOGRACE to include/linux/errno.h.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[bfields]

On Mon, Jun 14, 2021 at 07:53:52PM +0000, Trond Myklebust wrote:
> On Mon, 2021-06-14 at 15:34 -0400, J. Bruce Fields wrote:
> > On Mon, Jun 14, 2021 at 02:56:55PM +0000, Trond Myklebust wrote:
> > > On Mon, 2021-06-14 at 10:48 -0400, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > > 
> > > > In the reexport case, nfsd is currently passing along locks with
> > > > the
> > > > reclaim bit set.  The client sends a new lock request, which is
> > > > granted
> > > > if there's currently no conflict--even if it's possible a
> > > > conflicting
> > > > lock could have been briefly held in the interim.
> > > > 
> > > > We don't currently have any way to safely grant reclaim, so for
> > > > now
> > > > let's just deny them all.
> > > > 
> > > > I'm doing this by passing the reclaim bit to nfs and letting it
> > > > fail
> > > > the
> > > > call, with the idea that eventually the client might be able to
> > > > do
> > > > something more forgiving here.
> > > > 
> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > > ---
> > > >  fs/nfs/file.c       | 3 +++
> > > >  fs/nfsd/nfs4state.c | 3 +++
> > > >  fs/nfsd/nfsproc.c   | 1 +
> > > >  include/linux/fs.h  | 1 +
> > > >  4 files changed, 8 insertions(+)
> > > > 
> > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > > index 1fef107961bc..35a29b440e3e 100644
> > > > --- a/fs/nfs/file.c
> > > > +++ b/fs/nfs/file.c
> > > > @@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd,
> > > > struct
> > > > file_lock *fl)
> > > >  
> > > >         nfs_inc_stats(inode, NFSIOS_VFSLOCK);
> > > >  
> > > > +       if (fl->fl_flags & FL_RECLAIM)
> > > > +               return -NFSERR_NO_GRACE;
> > > 
> > > NACK. nfs_lock() is required to return a POSIX error. I know that
> > > right
> > > now, nfsd is the only thing setting FL_RECLAIM, but we can't
> > > guarantee
> > > that will always be the case.
> > 
> > Setting FL_RECLAIM tells the filesystem that you're prepared to
> > handle
> > NFSERR_NO_GRACE.  I'm not seeing the risk.
> 
> You are using a function that is exposed to the VFS. On error, that
> function is expected to return a value that is a Linux error between -1
> and -4095.

Or 1, actually (FILE_LOCK_DEFERRED).

> I suggest adding an error value ENOGRACE to include/linux/errno.h.

I can live with that, but I'm still curious what exactly you're worried
about.

--b.

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[Trond Myklebust]

On Mon, 2021-06-14 at 16:03 -0400, bfields@fieldses.org wrote:
> On Mon, Jun 14, 2021 at 07:53:52PM +0000, Trond Myklebust wrote:
> > On Mon, 2021-06-14 at 15:34 -0400, J. Bruce Fields wrote:
> > > On Mon, Jun 14, 2021 at 02:56:55PM +0000, Trond Myklebust wrote:
> > > > On Mon, 2021-06-14 at 10:48 -0400, J. Bruce Fields wrote:
> > > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > > > 
> > > > > In the reexport case, nfsd is currently passing along locks
> > > > > with
> > > > > the
> > > > > reclaim bit set.  The client sends a new lock request, which
> > > > > is
> > > > > granted
> > > > > if there's currently no conflict--even if it's possible a
> > > > > conflicting
> > > > > lock could have been briefly held in the interim.
> > > > > 
> > > > > We don't currently have any way to safely grant reclaim, so
> > > > > for
> > > > > now
> > > > > let's just deny them all.
> > > > > 
> > > > > I'm doing this by passing the reclaim bit to nfs and letting
> > > > > it
> > > > > fail
> > > > > the
> > > > > call, with the idea that eventually the client might be able
> > > > > to
> > > > > do
> > > > > something more forgiving here.
> > > > > 
> > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > > > ---
> > > > >  fs/nfs/file.c       | 3 +++
> > > > >  fs/nfsd/nfs4state.c | 3 +++
> > > > >  fs/nfsd/nfsproc.c   | 1 +
> > > > >  include/linux/fs.h  | 1 +
> > > > >  4 files changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > > > index 1fef107961bc..35a29b440e3e 100644
> > > > > --- a/fs/nfs/file.c
> > > > > +++ b/fs/nfs/file.c
> > > > > @@ -806,6 +806,9 @@ int nfs_lock(struct file *filp, int cmd,
> > > > > struct
> > > > > file_lock *fl)
> > > > >  
> > > > >         nfs_inc_stats(inode, NFSIOS_VFSLOCK);
> > > > >  
> > > > > +       if (fl->fl_flags & FL_RECLAIM)
> > > > > +               return -NFSERR_NO_GRACE;
> > > > 
> > > > NACK. nfs_lock() is required to return a POSIX error. I know
> > > > that
> > > > right
> > > > now, nfsd is the only thing setting FL_RECLAIM, but we can't
> > > > guarantee
> > > > that will always be the case.
> > > 
> > > Setting FL_RECLAIM tells the filesystem that you're prepared to
> > > handle
> > > NFSERR_NO_GRACE.  I'm not seeing the risk.
> > 
> > You are using a function that is exposed to the VFS. On error, that
> > function is expected to return a value that is a Linux error
> > between -1
> > and -4095.
> 
> Or 1, actually (FILE_LOCK_DEFERRED).
> 
> > I suggest adding an error value ENOGRACE to include/linux/errno.h.
> 
> I can live with that, but I'm still curious what exactly you're
> worried
> about.
> 

I want to avoid the kind of issues we've be met with earlier when
mixing types just because they happened to be integer valued.

We introduced the mixing of POSIX/Linux and NFS errors in the NFS
client back in the 1990s, and that was a mistake that we're still
paying for. For instance, the value ERR_PTR(-NFSERR_NO_GRACE) will be
happily declared as a valid pointer by the IS_ERR() test, and every so
often we find an Oops around that issue because someone used the return
value from a function that they thought was POSIX/Linux error valued,
because it usually is returning POSIX errors.


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 3/3] nfs: don't allow reexport reclaims

[bfields]

On Mon, Jun 14, 2021 at 09:03:35PM +0000, Trond Myklebust wrote:
> I want to avoid the kind of issues we've be met with earlier when
> mixing types just because they happened to be integer valued.
> 
> We introduced the mixing of POSIX/Linux and NFS errors in the NFS
> client back in the 1990s, and that was a mistake that we're still
> paying for. For instance, the value ERR_PTR(-NFSERR_NO_GRACE) will be
> happily declared as a valid pointer by the IS_ERR() test, and every so
> often we find an Oops around that issue because someone used the return
> value from a function that they thought was POSIX/Linux error valued,
> because it usually is returning POSIX errors.

I did this, by the way, but also ran across a couple more bugs in
testing.

At this point I've got connectathon locking tests passing on a
re-export--I need to do a little more cleanup and then I'll repost.

--b.