All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] reexport lock fixes v3
@ 2021-08-20 21:01 J. Bruce Fields
  2021-08-20 21:01 ` [PATCH 1/8] lockd: lockd server-side shouldn't set fl_ops J. Bruce Fields
                   ` (9 more replies)
  0 siblings, 10 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:01 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, 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.

Note:
	- patches 1-5 are server side
	- patches 6-7 are client side
	- patch 8 affects both

Simplest might be for Trond or Anna to ACK 6-8, if they look OK, and
then submit them all through the server.  But those three sets of
patches are all independent if you'd rather split them up.

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.

Change since v2:
	- keep nlmsvc_file_inode a static inline to address build
	  failure identified by the kernel test robot
Changes since v1:
	- Use ENOGRACE instead of returning NFS-specific error from vfs lock
	  method.
	- Take write opens for write locks in the NLM case (as we always
	  have in the NFSv4 case).
	- Don't block NLM threads waiting for blocking locks.

With those changes I'm passing connecthon tests for NFSv3-4.2 reexports
of an NFSv4.0 filesystem.

--b.

J. Bruce Fields (8):
  lockd: lockd server-side shouldn't set fl_ops
  nlm: minor nlm_lookup_file argument change
  nlm: minor refactoring
  lockd: update nlm_lookup_file reexport comment
  Keep read and write fds with each nlm_file
  nfs: don't atempt blocking locks on nfs reexports
  lockd: don't attempt blocking locks on nfs reexports
  nfs: don't allow reexport reclaims

 fs/lockd/svc4proc.c         |   6 +-
 fs/lockd/svclock.c          |  80 ++++++++++++++----------
 fs/lockd/svcproc.c          |   6 +-
 fs/lockd/svcsubs.c          | 117 +++++++++++++++++++++++++-----------
 fs/nfs/export.c             |   2 +-
 fs/nfs/file.c               |   3 +
 fs/nfsd/lockd.c             |   8 ++-
 fs/nfsd/nfs4state.c         |  11 +++-
 fs/nfsd/nfsproc.c           |   1 +
 include/linux/errno.h       |   1 +
 include/linux/exportfs.h    |   2 +
 include/linux/fs.h          |   1 +
 include/linux/lockd/bind.h  |   3 +-
 include/linux/lockd/lockd.h |  11 +++-
 14 files changed, 170 insertions(+), 82 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/8] lockd: lockd server-side shouldn't set fl_ops
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
@ 2021-08-20 21:01 ` J. Bruce Fields
  2021-08-20 21:02 ` [PATCH 2/8] nlm: minor nlm_lookup_file argument change J. Bruce Fields
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:01 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, 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.

Reported-by: Daire Byrne <daire@dneg.com>
Tested-by: Daire Byrne <daire@dneg.com>
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


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 2/8] nlm: minor nlm_lookup_file argument change
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
  2021-08-20 21:01 ` [PATCH 1/8] lockd: lockd server-side shouldn't set fl_ops J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-21 16:30   ` Chuck Lever III
  2021-08-20 21:02 ` [PATCH 3/8] nlm: minor refactoring J. Bruce Fields
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, J. Bruce Fields

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

It'll come in handy to get the whole nlm_lock.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svc4proc.c         |  2 +-
 fs/lockd/svcproc.c          |  2 +-
 fs/lockd/svcsubs.c          | 14 +++++++-------
 include/linux/lockd/lockd.h |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 4c10fb5138f1..aa8eca7c38a1 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -40,7 +40,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 	/* Obtain file pointer. Not used by FREE_ALL call. */
 	if (filp != NULL) {
-		if ((error = nlm_lookup_file(rqstp, &file, &lock->fh)) != 0)
+		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
 			goto no_locks;
 		*filp = file;
 
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 4ae4b63b5392..f4e5e0eb30fd 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -69,7 +69,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 	/* Obtain file pointer. Not used by FREE_ALL call. */
 	if (filp != NULL) {
-		error = cast_status(nlm_lookup_file(rqstp, &file, &lock->fh));
+		error = cast_status(nlm_lookup_file(rqstp, &file, lock));
 		if (error != 0)
 			goto no_locks;
 		*filp = file;
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 028fc152da22..bbd2bdde4bea 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -82,31 +82,31 @@ static inline unsigned int file_hash(struct nfs_fh *f)
  */
 __be32
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
-					struct nfs_fh *f)
+					struct nlm_lock *lock)
 {
 	struct nlm_file	*file;
 	unsigned int	hash;
 	__be32		nfserr;
 
-	nlm_debug_print_fh("nlm_lookup_file", f);
+	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
 
-	hash = file_hash(f);
+	hash = file_hash(&lock->fh);
 
 	/* Lock file table */
 	mutex_lock(&nlm_file_mutex);
 
 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
-		if (!nfs_compare_fh(&file->f_handle, f))
+		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
 			goto found;
 
-	nlm_debug_print_fh("creating file for", f);
+	nlm_debug_print_fh("creating file for", &lock->fh);
 
 	nfserr = nlm_lck_denied_nolocks;
 	file = kzalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
 		goto out_unlock;
 
-	memcpy(&file->f_handle, f, sizeof(struct nfs_fh));
+	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
 	mutex_init(&file->f_mutex);
 	INIT_HLIST_NODE(&file->f_list);
 	INIT_LIST_HEAD(&file->f_blocks);
@@ -117,7 +117,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, f, &file->f_file)) != 0) {
+	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
 		dprintk("lockd: open failed (error %d)\n", nfserr);
 		goto out_free;
 	}
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 666f5f310a04..81b71ad2040a 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -286,7 +286,7 @@ void		  nlmsvc_locks_init_private(struct file_lock *, struct nlm_host *, pid_t);
  * File handling for the server personality
  */
 __be32		  nlm_lookup_file(struct svc_rqst *, struct nlm_file **,
-					struct nfs_fh *);
+					struct nlm_lock *);
 void		  nlm_release_file(struct nlm_file *);
 void		  nlmsvc_release_lockowner(struct nlm_lock *);
 void		  nlmsvc_mark_resources(struct net *);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 3/8] nlm: minor refactoring
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
  2021-08-20 21:01 ` [PATCH 1/8] lockd: lockd server-side shouldn't set fl_ops J. Bruce Fields
  2021-08-20 21:02 ` [PATCH 2/8] nlm: minor nlm_lookup_file argument change J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-21 16:30   ` Chuck Lever III
  2021-08-20 21:02 ` [PATCH 4/8] lockd: update nlm_lookup_file reexport comment J. Bruce Fields
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, J. Bruce Fields

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

---
 fs/lockd/svclock.c | 16 ++++++++--------
 fs/lockd/svcsubs.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 1781fc5e9091..bc1cf31f3cce 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -474,8 +474,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	__be32			ret;
 
 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_type, lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end,
@@ -581,8 +581,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	struct nlm_lockowner	*test_owner;
 
 	dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_type,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
@@ -644,8 +644,8 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 	int	error;
 
 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
@@ -673,8 +673,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 	int status = 0;
 
 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index bbd2bdde4bea..2558598610a9 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -45,7 +45,7 @@ static inline void nlm_debug_print_fh(char *msg, struct nfs_fh *f)
 
 static inline void nlm_debug_print_file(char *msg, struct nlm_file *file)
 {
-	struct inode *inode = locks_inode(file->f_file);
+	struct inode *inode = nlmsvc_file_inode(file);
 
 	dprintk("lockd: %s %s/%ld\n",
 		msg, inode->i_sb->s_id, inode->i_ino);
@@ -415,7 +415,7 @@ nlmsvc_match_sb(void *datap, struct nlm_file *file)
 {
 	struct super_block *sb = datap;
 
-	return sb == locks_inode(file->f_file)->i_sb;
+	return sb == nlmsvc_file_inode(file)->i_sb;
 }
 
 /**
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 4/8] lockd: update nlm_lookup_file reexport comment
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (2 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 3/8] nlm: minor refactoring J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-20 21:02 ` [PATCH 5/8] Keep read and write fds with each nlm_file J. Bruce Fields
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, J. Bruce Fields

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

Update comment to reflect that we *do* allow reexport, whether it's a
good idea or not....

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svcsubs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 2558598610a9..f43d89e89c45 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -111,8 +111,9 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	INIT_HLIST_NODE(&file->f_list);
 	INIT_LIST_HEAD(&file->f_blocks);
 
-	/* Open the file. Note that this must not sleep for too long, else
-	 * we would lock up lockd:-) So no NFS re-exports, folks.
+	/*
+	 * Open the file. Note that if we're reexporting, for example,
+	 * this could block the lockd thread for a while.
 	 *
 	 * We have to make sure we have the right credential to open
 	 * the file.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (3 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 4/8] lockd: update nlm_lookup_file reexport comment J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-21 16:30   ` Chuck Lever III
  2021-08-20 21:02 ` [PATCH 6/8] nfs: don't atempt blocking locks on nfs reexports J. Bruce Fields
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, J. Bruce Fields

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

We shouldn't really be using a read-only file descriptor to take a write
lock.

Most filesystems will put up with it.  But NFS, for example, won't.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svc4proc.c         |   4 +-
 fs/lockd/svclock.c          |  25 ++++++---
 fs/lockd/svcproc.c          |   4 +-
 fs/lockd/svcsubs.c          | 104 +++++++++++++++++++++++++-----------
 fs/nfsd/lockd.c             |   8 ++-
 include/linux/lockd/bind.h  |   3 +-
 include/linux/lockd/lockd.h |   9 +++-
 7 files changed, 114 insertions(+), 43 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index aa8eca7c38a1..c7587de948e4 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -40,12 +40,14 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 	/* Obtain file pointer. Not used by FREE_ALL call. */
 	if (filp != NULL) {
+		int mode = lock_to_openmode(&lock->fl);
+
 		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
 			goto no_locks;
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
-		lock->fl.fl_file  = file->f_file;
+		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index bc1cf31f3cce..d60e6eea2d57 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -471,6 +471,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 {
 	struct nlm_block	*block = NULL;
 	int			error;
+	int			mode;
 	__be32			ret;
 
 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
@@ -524,7 +525,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 
 	if (!wait)
 		lock->fl.fl_flags &= ~FL_SLEEP;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
 	lock->fl.fl_flags &= ~FL_SLEEP;
 
 	dprintk("lockd: vfs_lock_file returned %d\n", error);
@@ -577,6 +579,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		struct nlm_lock *conflock, struct nlm_cookie *cookie)
 {
 	int			error;
+	int			mode;
 	__be32			ret;
 	struct nlm_lockowner	*test_owner;
 
@@ -595,7 +598,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	/* If there's a conflicting lock, remember to clean up the test lock */
 	test_owner = (struct nlm_lockowner *)lock->fl.fl_owner;
 
-	error = vfs_test_lock(file->f_file, &lock->fl);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_test_lock(file->f_file[mode], &lock->fl);
 	if (error) {
 		/* We can't currently deal with deferred test requests */
 		if (error == FILE_LOCK_DEFERRED)
@@ -641,7 +645,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 __be32
 nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 {
-	int	error;
+	int	error = 0;
 
 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
 				nlmsvc_file_inode(file)->i_sb->s_id,
@@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 	nlmsvc_cancel_blocked(net, file, lock);
 
 	lock->fl.fl_type = F_UNLCK;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	if (file->f_file[0])
+		error = vfs_lock_file(file->f_file[0], F_SETLK,
+					&lock->fl, NULL);
+	if (file->f_file[1])
+		error = vfs_lock_file(file->f_file[1], F_SETLK,
+					&lock->fl, NULL);
 
 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
 }
@@ -671,6 +680,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 {
 	struct nlm_block	*block;
 	int status = 0;
+	int mode;
 
 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
 				nlmsvc_file_inode(file)->i_sb->s_id,
@@ -686,7 +696,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 	block = nlmsvc_lookup_block(file, lock);
 	mutex_unlock(&file->f_mutex);
 	if (block != NULL) {
-		vfs_cancel_lock(block->b_file->f_file,
+		mode = lock_to_openmode(&lock->fl);
+		vfs_cancel_lock(block->b_file->f_file[mode],
 				&block->b_call->a_args.lock.fl);
 		status = nlmsvc_unlink_block(block);
 		nlmsvc_release_block(block);
@@ -803,6 +814,7 @@ nlmsvc_grant_blocked(struct nlm_block *block)
 {
 	struct nlm_file		*file = block->b_file;
 	struct nlm_lock		*lock = &block->b_call->a_args.lock;
+	int			mode;
 	int			error;
 	loff_t			fl_start, fl_end;
 
@@ -828,7 +840,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
 	lock->fl.fl_flags |= FL_SLEEP;
 	fl_start = lock->fl.fl_start;
 	fl_end = lock->fl.fl_end;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
 	lock->fl.fl_flags &= ~FL_SLEEP;
 	lock->fl.fl_start = fl_start;
 	lock->fl.fl_end = fl_end;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index f4e5e0eb30fd..99696d3f6dd6 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -55,6 +55,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 	struct nlm_host		*host = NULL;
 	struct nlm_file		*file = NULL;
 	struct nlm_lock		*lock = &argp->lock;
+	int			mode;
 	__be32			error = 0;
 
 	/* nfsd callbacks must have been installed for this procedure */
@@ -75,7 +76,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
-		lock->fl.fl_file  = file->f_file;
+		mode = lock_to_openmode(&lock->fl);
+		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index f43d89e89c45..a0adaee245ae 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
 	return tmp & (FILE_NRHASH - 1);
 }
 
+int lock_to_openmode(struct file_lock *lock)
+{
+	if (lock->fl_type == F_WRLCK)
+		return O_WRONLY;
+	else
+		return O_RDONLY;
+}
+
+/*
+ * Open the file. Note that if we're reexporting, for example,
+ * this could block the lockd thread for a while.
+ *
+ * We have to make sure we have the right credential to open
+ * the file.
+ */
+static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
+			   struct nlm_file *file, int mode)
+{
+	struct file **fp = &file->f_file[mode];
+	__be32	nfserr;
+
+	if (*fp)
+		return 0;
+	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
+	if (nfserr)
+		dprintk("lockd: open failed (error %d)\n", nfserr);
+	return nfserr;
+}
+
 /*
  * Lookup file info. If it doesn't exist, create a file info struct
  * and open a (VFS) file for the given inode.
- *
- * FIXME:
- * Note that we open the file O_RDONLY even when creating write locks.
- * This is not quite right, but for now, we assume the client performs
- * the proper R/W checking.
  */
 __be32
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
@@ -87,41 +111,38 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	struct nlm_file	*file;
 	unsigned int	hash;
 	__be32		nfserr;
+	int		mode;
 
 	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
 
 	hash = file_hash(&lock->fh);
+	mode = lock_to_openmode(&lock->fl);
 
 	/* Lock file table */
 	mutex_lock(&nlm_file_mutex);
 
 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
-		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
+		if (!nfs_compare_fh(&file->f_handle, &lock->fh)) {
+			mutex_lock(&file->f_mutex);
+			nfserr = nlm_do_fopen(rqstp, file, mode);
+			mutex_unlock(&file->f_mutex);
 			goto found;
-
+		}
 	nlm_debug_print_fh("creating file for", &lock->fh);
 
 	nfserr = nlm_lck_denied_nolocks;
 	file = kzalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
-		goto out_unlock;
+		goto out_free;
 
 	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
 	mutex_init(&file->f_mutex);
 	INIT_HLIST_NODE(&file->f_list);
 	INIT_LIST_HEAD(&file->f_blocks);
 
-	/*
-	 * Open the file. Note that if we're reexporting, for example,
-	 * this could block the lockd thread for a while.
-	 *
-	 * We have to make sure we have the right credential to open
-	 * the file.
-	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
-		dprintk("lockd: open failed (error %d)\n", nfserr);
-		goto out_free;
-	}
+	nfserr = nlm_do_fopen(rqstp, file, mode);
+	if (nfserr)
+		goto out_unlock;
 
 	hlist_add_head(&file->f_list, &nlm_files[hash]);
 
@@ -129,7 +150,6 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
 	*result = file;
 	file->f_count++;
-	nfserr = 0;
 
 out_unlock:
 	mutex_unlock(&nlm_file_mutex);
@@ -149,13 +169,34 @@ nlm_delete_file(struct nlm_file *file)
 	nlm_debug_print_file("closing file", file);
 	if (!hlist_unhashed(&file->f_list)) {
 		hlist_del(&file->f_list);
-		nlmsvc_ops->fclose(file->f_file);
+		if (file->f_file[O_RDONLY])
+			nlmsvc_ops->fclose(file->f_file[O_RDONLY]);
+		if (file->f_file[O_WRONLY])
+			nlmsvc_ops->fclose(file->f_file[O_WRONLY]);
 		kfree(file);
 	} else {
 		printk(KERN_WARNING "lockd: attempt to release unknown file!\n");
 	}
 }
 
+static int nlm_unlock_files(struct nlm_file *file)
+{
+	struct file_lock lock;
+	struct file *f;
+
+	lock.fl_type  = F_UNLCK;
+	lock.fl_start = 0;
+	lock.fl_end   = OFFSET_MAX;
+	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
+		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
+			printk("lockd: unlock failure in %s:%d\n",
+				__FILE__, __LINE__);
+			return 1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Loop over all locks on the given file and perform the specified
  * action.
@@ -183,17 +224,10 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
 
 		lockhost = ((struct nlm_lockowner *)fl->fl_owner)->host;
 		if (match(lockhost, host)) {
-			struct file_lock lock = *fl;
 
 			spin_unlock(&flctx->flc_lock);
-			lock.fl_type  = F_UNLCK;
-			lock.fl_start = 0;
-			lock.fl_end   = OFFSET_MAX;
-			if (vfs_lock_file(file->f_file, F_SETLK, &lock, NULL) < 0) {
-				printk("lockd: unlock failure in %s:%d\n",
-						__FILE__, __LINE__);
+			if (nlm_unlock_files(file))
 				return 1;
-			}
 			goto again;
 		}
 	}
@@ -247,6 +281,15 @@ nlm_file_inuse(struct nlm_file *file)
 	return 0;
 }
 
+static void nlm_close_files(struct nlm_file *file)
+{
+	struct file *f;
+
+	for (f = file->f_file[0]; f <= file->f_file[1]; f++)
+		if (f)
+			nlmsvc_ops->fclose(f);
+}
+
 /*
  * Loop over all files in the file table.
  */
@@ -277,7 +320,7 @@ nlm_traverse_files(void *data, nlm_host_match_fn_t match,
 			if (list_empty(&file->f_blocks) && !file->f_locks
 			 && !file->f_shares && !file->f_count) {
 				hlist_del(&file->f_list);
-				nlmsvc_ops->fclose(file->f_file);
+				nlm_close_files(file);
 				kfree(file);
 			}
 		}
@@ -411,6 +454,7 @@ nlmsvc_invalidate_all(void)
 	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
 }
 
+
 static int
 nlmsvc_match_sb(void *datap, struct nlm_file *file)
 {
diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
index 3f5b3d7b62b7..606fa155c28a 100644
--- a/fs/nfsd/lockd.c
+++ b/fs/nfsd/lockd.c
@@ -25,9 +25,11 @@
  * Note: we hold the dentry use count while the file is open.
  */
 static __be32
-nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
+nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
+		int mode)
 {
 	__be32		nfserr;
+	int		access;
 	struct svc_fh	fh;
 
 	/* must initialize before using! but maxsize doesn't matter */
@@ -36,7 +38,9 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
 	memcpy((char*)&fh.fh_handle.fh_base, f->data, f->size);
 	fh.fh_export = NULL;
 
-	nfserr = nfsd_open(rqstp, &fh, S_IFREG, NFSD_MAY_LOCK, filp);
+	access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
+	access |= NFSD_MAY_LOCK;
+	nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
 	fh_put(&fh);
  	/* We return nlm error codes as nlm doesn't know
 	 * about nfsd, but nfsd does know about nlm..
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 0520c0cd73f4..3bc9f7410e21 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -27,7 +27,8 @@ struct rpc_task;
 struct nlmsvc_binding {
 	__be32			(*fopen)(struct svc_rqst *,
 						struct nfs_fh *,
-						struct file **);
+						struct file **,
+						int mode);
 	void			(*fclose)(struct file *);
 };
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 81b71ad2040a..da319de7e557 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -10,6 +10,8 @@
 #ifndef LINUX_LOCKD_LOCKD_H
 #define LINUX_LOCKD_LOCKD_H
 
+/* XXX: a lot of this should really be under fs/lockd. */
+
 #include <linux/in.h>
 #include <linux/in6.h>
 #include <net/ipv6.h>
@@ -154,7 +156,8 @@ struct nlm_rqst {
 struct nlm_file {
 	struct hlist_node	f_list;		/* linked list */
 	struct nfs_fh		f_handle;	/* NFS file handle */
-	struct file *		f_file;		/* VFS file pointer */
+	struct file *		f_file[2];	/* VFS file pointers,
+						   indexed by O_ flags */
 	struct nlm_share *	f_shares;	/* DOS shares */
 	struct list_head	f_blocks;	/* blocked locks */
 	unsigned int		f_locks;	/* guesstimate # of locks */
@@ -267,6 +270,7 @@ typedef int	  (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
 /*
  * Server-side lock handling
  */
+int		  lock_to_openmode(struct file_lock *);
 __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
 			      struct nlm_host *, struct nlm_lock *, int,
 			      struct nlm_cookie *, int);
@@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
 
 static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
 {
-	return locks_inode(file->f_file);
+	return locks_inode(file->f_file[0] ?
+				file->f_file[0] : file->f_file[1]);
 }
 
 static inline int __nlm_privileged_request4(const struct sockaddr *sap)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 6/8] nfs: don't atempt blocking locks on nfs reexports
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (4 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 5/8] Keep read and write fds with each nlm_file J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-20 21:02 ` [PATCH 7/8] lockd: don't attempt " J. Bruce Fields
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, 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 fa67ecd5fe63..bebe86cce7c7 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


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 7/8] lockd: don't attempt blocking locks on nfs reexports
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (5 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 6/8] nfs: don't atempt blocking locks on nfs reexports J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-20 21:02 ` [PATCH 8/8] nfs: don't allow reexport reclaims J. Bruce Fields
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, J. Bruce Fields

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

As in the v4 case, it doesn't work well to block waiting for a lock on
an nfs filesystem.

As in the v4 case, that means we're depending on the client to poll.
It's probably incorrect to depend on that, but I *think* clients do poll
in practice.  In any case, it's an improvement over hanging the lockd
thread indefinitely as we currently are.

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

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index d60e6eea2d57..c99acefb9ec9 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -31,6 +31,7 @@
 #include <linux/lockd/nlm.h>
 #include <linux/lockd/lockd.h>
 #include <linux/kthread.h>
+#include <linux/exportfs.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCLOCK
 
@@ -470,18 +471,24 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	    struct nlm_cookie *cookie, int reclaim)
 {
 	struct nlm_block	*block = NULL;
+	struct inode		*inode = nlmsvc_file_inode(file);
 	int			error;
 	int			mode;
+	int			async_block = 0;
 	__be32			ret;
 
 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
-				nlmsvc_file_inode(file)->i_sb->s_id,
-				nlmsvc_file_inode(file)->i_ino,
+				inode->i_sb->s_id, inode->i_ino,
 				lock->fl.fl_type, lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end,
 				wait);
 
+	if (inode->i_sb->s_export_op->flags & EXPORT_OP_SYNC_LOCKS) {
+		async_block = wait;
+		wait = 0;
+	}
+
 	/* Lock file against concurrent access */
 	mutex_lock(&file->f_mutex);
 	/* Get existing block (in case client is busy-waiting)
@@ -542,7 +549,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 			 */
 			if (wait)
 				break;
-			ret = nlm_lck_denied;
+			ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
 			goto out;
 		case FILE_LOCK_DEFERRED:
 			if (wait)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 8/8] nfs: don't allow reexport reclaims
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (6 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 7/8] lockd: don't attempt " J. Bruce Fields
@ 2021-08-20 21:02 ` J. Bruce Fields
  2021-08-25  2:35 ` [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
  2021-08-26 19:05 ` [PATCH 0/8] reexport lock fixes v3 Anna Schumaker
  9 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-20 21:02 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker
  Cc: daire, linux-nfs, 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/errno.h | 1 +
 include/linux/fs.h    | 1 +
 5 files changed, 9 insertions(+)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 1fef107961bc..7411658f8b05 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 -ENOGRACE;
+
 	/* 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 bebe86cce7c7..212f43fa5cba 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..90fcd6178823 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, -ENOGRACE},
 	};
 	int	i;
 
diff --git a/include/linux/errno.h b/include/linux/errno.h
index d73f597a2484..8b0c754bab02 100644
--- a/include/linux/errno.h
+++ b/include/linux/errno.h
@@ -31,5 +31,6 @@
 #define EJUKEBOX	528	/* Request initiated, but will not complete before timeout */
 #define EIOCBQUEUED	529	/* iocb queued, will get completion event */
 #define ERECALLCONFLICT	530	/* conflict with recalled state */
+#define ENOGRACE	531	/* NFS file lock reclaim refused */
 
 #endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 640574294216..1f5c3dbce1da 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


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] nlm: minor nlm_lookup_file argument change
  2021-08-20 21:02 ` [PATCH 2/8] nlm: minor nlm_lookup_file argument change J. Bruce Fields
@ 2021-08-21 16:30   ` Chuck Lever III
  2021-08-23 16:01     ` J. Bruce Fields
  0 siblings, 1 reply; 28+ messages in thread
From: Chuck Lever III @ 2021-08-21 16:30 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Trond Myklebust, Anna Schumaker, daire, Linux NFS Mailing List



> On Aug 20, 2021, at 5:02 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> It'll come in handy to get the whole nlm_lock.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/lockd/svc4proc.c         |  2 +-
> fs/lockd/svcproc.c          |  2 +-
> fs/lockd/svcsubs.c          | 14 +++++++-------
> include/linux/lockd/lockd.h |  2 +-
> 4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 4c10fb5138f1..aa8eca7c38a1 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -40,7 +40,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 
> 	/* Obtain file pointer. Not used by FREE_ALL call. */
> 	if (filp != NULL) {
> -		if ((error = nlm_lookup_file(rqstp, &file, &lock->fh)) != 0)
> +		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)

Style: Replace the "assignment in if statement" in these spots,
bitte?

Since we're dealing with a __be32 result, a direct comparison
with "0" is misleading. It might be better documentation to
go with:

    error = nlm_lookup_file(rqstp, &file, lock);
    if (error)


> 			goto no_locks;
> 		*filp = file;
> 
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 4ae4b63b5392..f4e5e0eb30fd 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -69,7 +69,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 
> 	/* Obtain file pointer. Not used by FREE_ALL call. */
> 	if (filp != NULL) {
> -		error = cast_status(nlm_lookup_file(rqstp, &file, &lock->fh));
> +		error = cast_status(nlm_lookup_file(rqstp, &file, lock));
> 		if (error != 0)
> 			goto no_locks;
> 		*filp = file;
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index 028fc152da22..bbd2bdde4bea 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -82,31 +82,31 @@ static inline unsigned int file_hash(struct nfs_fh *f)
>  */
> __be32
> nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> -					struct nfs_fh *f)
> +					struct nlm_lock *lock)
> {
> 	struct nlm_file	*file;
> 	unsigned int	hash;
> 	__be32		nfserr;
> 
> -	nlm_debug_print_fh("nlm_lookup_file", f);
> +	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
> 
> -	hash = file_hash(f);
> +	hash = file_hash(&lock->fh);
> 
> 	/* Lock file table */
> 	mutex_lock(&nlm_file_mutex);
> 
> 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
> -		if (!nfs_compare_fh(&file->f_handle, f))
> +		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
> 			goto found;
> 
> -	nlm_debug_print_fh("creating file for", f);
> +	nlm_debug_print_fh("creating file for", &lock->fh);
> 
> 	nfserr = nlm_lck_denied_nolocks;
> 	file = kzalloc(sizeof(*file), GFP_KERNEL);
> 	if (!file)
> 		goto out_unlock;
> 
> -	memcpy(&file->f_handle, f, sizeof(struct nfs_fh));
> +	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
> 	mutex_init(&file->f_mutex);
> 	INIT_HLIST_NODE(&file->f_list);
> 	INIT_LIST_HEAD(&file->f_blocks);
> @@ -117,7 +117,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	 * We have to make sure we have the right credential to open
> 	 * the file.
> 	 */
> -	if ((nfserr = nlmsvc_ops->fopen(rqstp, f, &file->f_file)) != 0) {
> +	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {

Ditto.


> 		dprintk("lockd: open failed (error %d)\n", nfserr);
> 		goto out_free;
> 	}
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 666f5f310a04..81b71ad2040a 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -286,7 +286,7 @@ void		  nlmsvc_locks_init_private(struct file_lock *, struct nlm_host *, pid_t);
>  * File handling for the server personality
>  */
> __be32		  nlm_lookup_file(struct svc_rqst *, struct nlm_file **,
> -					struct nfs_fh *);
> +					struct nlm_lock *);
> void		  nlm_release_file(struct nlm_file *);
> void		  nlmsvc_release_lockowner(struct nlm_lock *);
> void		  nlmsvc_mark_resources(struct net *);
> -- 
> 2.31.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] nlm: minor refactoring
  2021-08-20 21:02 ` [PATCH 3/8] nlm: minor refactoring J. Bruce Fields
@ 2021-08-21 16:30   ` Chuck Lever III
  2021-08-23 15:26     ` J. Bruce Fields
  0 siblings, 1 reply; 28+ messages in thread
From: Chuck Lever III @ 2021-08-21 16:30 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Trond Myklebust, Anna Schumaker, daire, Linux NFS Mailing List



> On Aug 20, 2021, at 5:02 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>

Missing Signed-off-by?

And add just a little text that explains why this change is needed?


> ---
> fs/lockd/svclock.c | 16 ++++++++--------
> fs/lockd/svcsubs.c |  4 ++--
> 2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 1781fc5e9091..bc1cf31f3cce 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -474,8 +474,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> 	__be32			ret;
> 
> 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
> -				locks_inode(file->f_file)->i_sb->s_id,
> -				locks_inode(file->f_file)->i_ino,
> +				nlmsvc_file_inode(file)->i_sb->s_id,
> +				nlmsvc_file_inode(file)->i_ino,
> 				lock->fl.fl_type, lock->fl.fl_pid,
> 				(long long)lock->fl.fl_start,
> 				(long long)lock->fl.fl_end,
> @@ -581,8 +581,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 	struct nlm_lockowner	*test_owner;
> 
> 	dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
> -				locks_inode(file->f_file)->i_sb->s_id,
> -				locks_inode(file->f_file)->i_ino,
> +				nlmsvc_file_inode(file)->i_sb->s_id,
> +				nlmsvc_file_inode(file)->i_ino,
> 				lock->fl.fl_type,
> 				(long long)lock->fl.fl_start,
> 				(long long)lock->fl.fl_end);
> @@ -644,8 +644,8 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> 	int	error;
> 
> 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
> -				locks_inode(file->f_file)->i_sb->s_id,
> -				locks_inode(file->f_file)->i_ino,
> +				nlmsvc_file_inode(file)->i_sb->s_id,
> +				nlmsvc_file_inode(file)->i_ino,
> 				lock->fl.fl_pid,
> 				(long long)lock->fl.fl_start,
> 				(long long)lock->fl.fl_end);
> @@ -673,8 +673,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> 	int status = 0;
> 
> 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
> -				locks_inode(file->f_file)->i_sb->s_id,
> -				locks_inode(file->f_file)->i_ino,
> +				nlmsvc_file_inode(file)->i_sb->s_id,
> +				nlmsvc_file_inode(file)->i_ino,
> 				lock->fl.fl_pid,
> 				(long long)lock->fl.fl_start,
> 				(long long)lock->fl.fl_end);
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index bbd2bdde4bea..2558598610a9 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -45,7 +45,7 @@ static inline void nlm_debug_print_fh(char *msg, struct nfs_fh *f)
> 
> static inline void nlm_debug_print_file(char *msg, struct nlm_file *file)
> {
> -	struct inode *inode = locks_inode(file->f_file);
> +	struct inode *inode = nlmsvc_file_inode(file);
> 
> 	dprintk("lockd: %s %s/%ld\n",
> 		msg, inode->i_sb->s_id, inode->i_ino);
> @@ -415,7 +415,7 @@ nlmsvc_match_sb(void *datap, struct nlm_file *file)
> {
> 	struct super_block *sb = datap;
> 
> -	return sb == locks_inode(file->f_file)->i_sb;
> +	return sb == nlmsvc_file_inode(file)->i_sb;
> }
> 
> /**
> -- 
> 2.31.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-20 21:02 ` [PATCH 5/8] Keep read and write fds with each nlm_file J. Bruce Fields
@ 2021-08-21 16:30   ` Chuck Lever III
  2021-08-23 16:08     ` J. Bruce Fields
  2021-08-23 18:57     ` J. Bruce Fields
  0 siblings, 2 replies; 28+ messages in thread
From: Chuck Lever III @ 2021-08-21 16:30 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Trond Myklebust, Anna Schumaker, daire, Linux NFS Mailing List

Nit: short description should start with "nlm:"


> On Aug 20, 2021, at 5:02 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We shouldn't really be using a read-only file descriptor to take a write
> lock.
> 
> Most filesystems will put up with it.  But NFS, for example, won't.

Two overall concerns:

1. Would it be easy/possible to split this patch further?

2. What kind of testing would provide a level of confidence
   that no regressions have been introduced by this change?


> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/lockd/svc4proc.c         |   4 +-
> fs/lockd/svclock.c          |  25 ++++++---
> fs/lockd/svcproc.c          |   4 +-
> fs/lockd/svcsubs.c          | 104 +++++++++++++++++++++++++-----------
> fs/nfsd/lockd.c             |   8 ++-
> include/linux/lockd/bind.h  |   3 +-
> include/linux/lockd/lockd.h |   9 +++-
> 7 files changed, 114 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index aa8eca7c38a1..c7587de948e4 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -40,12 +40,14 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 
> 	/* Obtain file pointer. Not used by FREE_ALL call. */
> 	if (filp != NULL) {
> +		int mode = lock_to_openmode(&lock->fl);
> +
> 		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
> 			goto no_locks;
> 		*filp = file;
> 
> 		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.fl_file  = file->f_file;
> +		lock->fl.fl_file  = file->f_file[mode];
> 		lock->fl.fl_pid = current->tgid;
> 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index bc1cf31f3cce..d60e6eea2d57 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -471,6 +471,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> {
> 	struct nlm_block	*block = NULL;
> 	int			error;
> +	int			mode;
> 	__be32			ret;
> 
> 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
> @@ -524,7 +525,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> 
> 	if (!wait)
> 		lock->fl.fl_flags &= ~FL_SLEEP;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> 	lock->fl.fl_flags &= ~FL_SLEEP;
> 
> 	dprintk("lockd: vfs_lock_file returned %d\n", error);
> @@ -577,6 +579,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 		struct nlm_lock *conflock, struct nlm_cookie *cookie)
> {
> 	int			error;
> +	int			mode;
> 	__be32			ret;
> 	struct nlm_lockowner	*test_owner;
> 
> @@ -595,7 +598,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 	/* If there's a conflicting lock, remember to clean up the test lock */
> 	test_owner = (struct nlm_lockowner *)lock->fl.fl_owner;
> 
> -	error = vfs_test_lock(file->f_file, &lock->fl);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_test_lock(file->f_file[mode], &lock->fl);
> 	if (error) {
> 		/* We can't currently deal with deferred test requests */
> 		if (error == FILE_LOCK_DEFERRED)
> @@ -641,7 +645,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> __be32
> nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> {
> -	int	error;
> +	int	error = 0;
> 
> 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
> 				nlmsvc_file_inode(file)->i_sb->s_id,
> @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> 	nlmsvc_cancel_blocked(net, file, lock);
> 
> 	lock->fl.fl_type = F_UNLCK;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	if (file->f_file[0])
> +		error = vfs_lock_file(file->f_file[0], F_SETLK,
> +					&lock->fl, NULL);
> +	if (file->f_file[1])
> +		error = vfs_lock_file(file->f_file[1], F_SETLK,
> +					&lock->fl, NULL);

Eschew raw integers :-) Should the f_file array be indexed
using O_ flags as the comment below suggests?


> 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
> }
> @@ -671,6 +680,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> {
> 	struct nlm_block	*block;
> 	int status = 0;
> +	int mode;
> 
> 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
> 				nlmsvc_file_inode(file)->i_sb->s_id,
> @@ -686,7 +696,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> 	block = nlmsvc_lookup_block(file, lock);
> 	mutex_unlock(&file->f_mutex);
> 	if (block != NULL) {
> -		vfs_cancel_lock(block->b_file->f_file,
> +		mode = lock_to_openmode(&lock->fl);
> +		vfs_cancel_lock(block->b_file->f_file[mode],
> 				&block->b_call->a_args.lock.fl);
> 		status = nlmsvc_unlink_block(block);
> 		nlmsvc_release_block(block);
> @@ -803,6 +814,7 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> {
> 	struct nlm_file		*file = block->b_file;
> 	struct nlm_lock		*lock = &block->b_call->a_args.lock;
> +	int			mode;
> 	int			error;
> 	loff_t			fl_start, fl_end;
> 
> @@ -828,7 +840,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> 	lock->fl.fl_flags |= FL_SLEEP;
> 	fl_start = lock->fl.fl_start;
> 	fl_end = lock->fl.fl_end;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> 	lock->fl.fl_flags &= ~FL_SLEEP;
> 	lock->fl.fl_start = fl_start;
> 	lock->fl.fl_end = fl_end;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index f4e5e0eb30fd..99696d3f6dd6 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -55,6 +55,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 	struct nlm_host		*host = NULL;
> 	struct nlm_file		*file = NULL;
> 	struct nlm_lock		*lock = &argp->lock;
> +	int			mode;
> 	__be32			error = 0;
> 
> 	/* nfsd callbacks must have been installed for this procedure */
> @@ -75,7 +76,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 		*filp = file;
> 
> 		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.fl_file  = file->f_file;
> +		mode = lock_to_openmode(&lock->fl);
> +		lock->fl.fl_file  = file->f_file[mode];
> 		lock->fl.fl_pid = current->tgid;
> 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index f43d89e89c45..a0adaee245ae 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
> 	return tmp & (FILE_NRHASH - 1);
> }
> 
> +int lock_to_openmode(struct file_lock *lock)
> +{
> +	if (lock->fl_type == F_WRLCK)
> +		return O_WRONLY;
> +	else
> +		return O_RDONLY;

Style: a ternary would be more consistent with other areas
of the code you change in this patch.


> +}
> +
> +/*
> + * Open the file. Note that if we're reexporting, for example,
> + * this could block the lockd thread for a while.
> + *
> + * We have to make sure we have the right credential to open
> + * the file.
> + */
> +static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
> +			   struct nlm_file *file, int mode)
> +{
> +	struct file **fp = &file->f_file[mode];
> +	__be32	nfserr;
> +
> +	if (*fp)
> +		return 0;
> +	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
> +	if (nfserr)
> +		dprintk("lockd: open failed (error %d)\n", nfserr);
> +	return nfserr;
> +}
> +
> /*
>  * Lookup file info. If it doesn't exist, create a file info struct
>  * and open a (VFS) file for the given inode.
> - *
> - * FIXME:
> - * Note that we open the file O_RDONLY even when creating write locks.
> - * This is not quite right, but for now, we assume the client performs
> - * the proper R/W checking.
>  */
> __be32
> nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> @@ -87,41 +111,38 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	struct nlm_file	*file;
> 	unsigned int	hash;
> 	__be32		nfserr;
> +	int		mode;
> 
> 	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
> 
> 	hash = file_hash(&lock->fh);
> +	mode = lock_to_openmode(&lock->fl);
> 
> 	/* Lock file table */
> 	mutex_lock(&nlm_file_mutex);
> 
> 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
> -		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
> +		if (!nfs_compare_fh(&file->f_handle, &lock->fh)) {
> +			mutex_lock(&file->f_mutex);
> +			nfserr = nlm_do_fopen(rqstp, file, mode);
> +			mutex_unlock(&file->f_mutex);
> 			goto found;
> -
> +		}
> 	nlm_debug_print_fh("creating file for", &lock->fh);
> 
> 	nfserr = nlm_lck_denied_nolocks;
> 	file = kzalloc(sizeof(*file), GFP_KERNEL);
> 	if (!file)
> -		goto out_unlock;
> +		goto out_free;
> 
> 	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
> 	mutex_init(&file->f_mutex);
> 	INIT_HLIST_NODE(&file->f_list);
> 	INIT_LIST_HEAD(&file->f_blocks);
> 
> -	/*
> -	 * Open the file. Note that if we're reexporting, for example,
> -	 * this could block the lockd thread for a while.
> -	 *
> -	 * We have to make sure we have the right credential to open
> -	 * the file.
> -	 */
> -	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
> -		dprintk("lockd: open failed (error %d)\n", nfserr);
> -		goto out_free;
> -	}
> +	nfserr = nlm_do_fopen(rqstp, file, mode);
> +	if (nfserr)
> +		goto out_unlock;
> 
> 	hlist_add_head(&file->f_list, &nlm_files[hash]);
> 
> @@ -129,7 +150,6 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
> 	*result = file;
> 	file->f_count++;
> -	nfserr = 0;
> 
> out_unlock:
> 	mutex_unlock(&nlm_file_mutex);
> @@ -149,13 +169,34 @@ nlm_delete_file(struct nlm_file *file)
> 	nlm_debug_print_file("closing file", file);
> 	if (!hlist_unhashed(&file->f_list)) {
> 		hlist_del(&file->f_list);
> -		nlmsvc_ops->fclose(file->f_file);
> +		if (file->f_file[O_RDONLY])
> +			nlmsvc_ops->fclose(file->f_file[O_RDONLY]);
> +		if (file->f_file[O_WRONLY])
> +			nlmsvc_ops->fclose(file->f_file[O_WRONLY]);
> 		kfree(file);
> 	} else {
> 		printk(KERN_WARNING "lockd: attempt to release unknown file!\n");
> 	}
> }
> 
> +static int nlm_unlock_files(struct nlm_file *file)
> +{
> +	struct file_lock lock;
> +	struct file *f;
> +
> +	lock.fl_type  = F_UNLCK;
> +	lock.fl_start = 0;
> +	lock.fl_end   = OFFSET_MAX;
> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
> +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
> +			printk("lockd: unlock failure in %s:%d\n",
> +				__FILE__, __LINE__);

This needs a KERN_LEVEL and maybe a _once.


> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> /*
>  * Loop over all locks on the given file and perform the specified
>  * action.
> @@ -183,17 +224,10 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
> 
> 		lockhost = ((struct nlm_lockowner *)fl->fl_owner)->host;
> 		if (match(lockhost, host)) {
> -			struct file_lock lock = *fl;
> 
> 			spin_unlock(&flctx->flc_lock);
> -			lock.fl_type  = F_UNLCK;
> -			lock.fl_start = 0;
> -			lock.fl_end   = OFFSET_MAX;
> -			if (vfs_lock_file(file->f_file, F_SETLK, &lock, NULL) < 0) {
> -				printk("lockd: unlock failure in %s:%d\n",
> -						__FILE__, __LINE__);
> +			if (nlm_unlock_files(file))
> 				return 1;
> -			}
> 			goto again;
> 		}
> 	}
> @@ -247,6 +281,15 @@ nlm_file_inuse(struct nlm_file *file)
> 	return 0;
> }
> 
> +static void nlm_close_files(struct nlm_file *file)
> +{
> +	struct file *f;
> +
> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++)
> +		if (f)
> +			nlmsvc_ops->fclose(f);
> +}
> +
> /*
>  * Loop over all files in the file table.
>  */
> @@ -277,7 +320,7 @@ nlm_traverse_files(void *data, nlm_host_match_fn_t match,
> 			if (list_empty(&file->f_blocks) && !file->f_locks
> 			 && !file->f_shares && !file->f_count) {
> 				hlist_del(&file->f_list);
> -				nlmsvc_ops->fclose(file->f_file);
> +				nlm_close_files(file);
> 				kfree(file);
> 			}
> 		}
> @@ -411,6 +454,7 @@ nlmsvc_invalidate_all(void)
> 	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
> }
> 
> +
> static int
> nlmsvc_match_sb(void *datap, struct nlm_file *file)
> {
> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> index 3f5b3d7b62b7..606fa155c28a 100644
> --- a/fs/nfsd/lockd.c
> +++ b/fs/nfsd/lockd.c
> @@ -25,9 +25,11 @@
>  * Note: we hold the dentry use count while the file is open.
>  */
> static __be32
> -nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> +nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> +		int mode)
> {
> 	__be32		nfserr;
> +	int		access;
> 	struct svc_fh	fh;
> 
> 	/* must initialize before using! but maxsize doesn't matter */
> @@ -36,7 +38,9 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> 	memcpy((char*)&fh.fh_handle.fh_base, f->data, f->size);
> 	fh.fh_export = NULL;
> 
> -	nfserr = nfsd_open(rqstp, &fh, S_IFREG, NFSD_MAY_LOCK, filp);
> +	access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
> +	access |= NFSD_MAY_LOCK;
> +	nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
> 	fh_put(&fh);
>  	/* We return nlm error codes as nlm doesn't know
> 	 * about nfsd, but nfsd does know about nlm..
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 0520c0cd73f4..3bc9f7410e21 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -27,7 +27,8 @@ struct rpc_task;
> struct nlmsvc_binding {
> 	__be32			(*fopen)(struct svc_rqst *,
> 						struct nfs_fh *,
> -						struct file **);
> +						struct file **,
> +						int mode);

Style: "mode_t mode" might be better internal documentation.


> 	void			(*fclose)(struct file *);
> };
> 
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 81b71ad2040a..da319de7e557 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -10,6 +10,8 @@
> #ifndef LINUX_LOCKD_LOCKD_H
> #define LINUX_LOCKD_LOCKD_H
> 
> +/* XXX: a lot of this should really be under fs/lockd. */
> +
> #include <linux/in.h>
> #include <linux/in6.h>
> #include <net/ipv6.h>
> @@ -154,7 +156,8 @@ struct nlm_rqst {
> struct nlm_file {
> 	struct hlist_node	f_list;		/* linked list */
> 	struct nfs_fh		f_handle;	/* NFS file handle */
> -	struct file *		f_file;		/* VFS file pointer */
> +	struct file *		f_file[2];	/* VFS file pointers,
> +						   indexed by O_ flags */

Right, except that the new code in this patch always indexes
f_file with raw integers, making the comment misleading. My
preference is to keep the comment and change the new code to
index f_file symbolically.


> 	struct nlm_share *	f_shares;	/* DOS shares */
> 	struct list_head	f_blocks;	/* blocked locks */
> 	unsigned int		f_locks;	/* guesstimate # of locks */
> @@ -267,6 +270,7 @@ typedef int	  (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
> /*
>  * Server-side lock handling
>  */
> +int		  lock_to_openmode(struct file_lock *);
> __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
> 			      struct nlm_host *, struct nlm_lock *, int,
> 			      struct nlm_cookie *, int);
> @@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
> 
> static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
> {
> -	return locks_inode(file->f_file);
> +	return locks_inode(file->f_file[0] ?
> +				file->f_file[0] : file->f_file[1]);
> }
> 
> static inline int __nlm_privileged_request4(const struct sockaddr *sap)
> -- 
> 2.31.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] nlm: minor refactoring
  2021-08-21 16:30   ` Chuck Lever III
@ 2021-08-23 15:26     ` J. Bruce Fields
  0 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-23 15:26 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

From: "J. Bruce Fields" <bfields@redhat.com>
Subject: [PATCH] nlm: minor refactoring

Make this lookup slightly more concise, and prepare for changing how we
look this up in a following patch.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svclock.c | 16 ++++++++--------
 fs/lockd/svcsubs.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

> Missing Signed-off-by?
> 
> And add just a little text that explains why this change is needed?

Whoops, thanks, how about this?--b.

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 1781fc5e9091..bc1cf31f3cce 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -474,8 +474,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	__be32			ret;
 
 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_type, lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end,
@@ -581,8 +581,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	struct nlm_lockowner	*test_owner;
 
 	dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_type,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
@@ -644,8 +644,8 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 	int	error;
 
 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
@@ -673,8 +673,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 	int status = 0;
 
 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
-				locks_inode(file->f_file)->i_sb->s_id,
-				locks_inode(file->f_file)->i_ino,
+				nlmsvc_file_inode(file)->i_sb->s_id,
+				nlmsvc_file_inode(file)->i_ino,
 				lock->fl.fl_pid,
 				(long long)lock->fl.fl_start,
 				(long long)lock->fl.fl_end);
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index bbd2bdde4bea..2558598610a9 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -45,7 +45,7 @@ static inline void nlm_debug_print_fh(char *msg, struct nfs_fh *f)
 
 static inline void nlm_debug_print_file(char *msg, struct nlm_file *file)
 {
-	struct inode *inode = locks_inode(file->f_file);
+	struct inode *inode = nlmsvc_file_inode(file);
 
 	dprintk("lockd: %s %s/%ld\n",
 		msg, inode->i_sb->s_id, inode->i_ino);
@@ -415,7 +415,7 @@ nlmsvc_match_sb(void *datap, struct nlm_file *file)
 {
 	struct super_block *sb = datap;
 
-	return sb == locks_inode(file->f_file)->i_sb;
+	return sb == nlmsvc_file_inode(file)->i_sb;
 }
 
 /**
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] nlm: minor nlm_lookup_file argument change
  2021-08-21 16:30   ` Chuck Lever III
@ 2021-08-23 16:01     ` J. Bruce Fields
  2021-08-23 17:02       ` Chuck Lever III
  0 siblings, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-23 16:01 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

From: "J. Bruce Fields" <bfields@redhat.com>
Subject: [PATCH] nlm: minor style issue

Make the assignment separate.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svc4proc.c | 3 ++-
 fs/lockd/svcsubs.c  | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

> Style: Replace the "assignment in if statement" in these spots,
> bitte?

Feel free to fold this in if you'd prefer.--b.

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index aa8eca7c38a1..bc496bbd696b 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -40,7 +40,8 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 	/* Obtain file pointer. Not used by FREE_ALL call. */
 	if (filp != NULL) {
-		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
+		error = nlm_lookup_file(rqstp, &file, lock);
+		if (error)
 			goto no_locks;
 		*filp = file;
 
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index bbd2bdde4bea..2d62633b39e5 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -117,7 +117,8 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
+	nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file);
+	if (nfserr) {
 		dprintk("lockd: open failed (error %d)\n", nfserr);
 		goto out_free;
 	}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-21 16:30   ` Chuck Lever III
@ 2021-08-23 16:08     ` J. Bruce Fields
  2021-08-23 18:57     ` J. Bruce Fields
  1 sibling, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-23 16:08 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

On Sat, Aug 21, 2021 at 04:30:43PM +0000, Chuck Lever III wrote:
> Nit: short description should start with "nlm:"
> 
> 
> > On Aug 20, 2021, at 5:02 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > We shouldn't really be using a read-only file descriptor to take a write
> > lock.
> > 
> > Most filesystems will put up with it.  But NFS, for example, won't.
> 
> Two overall concerns:
> 
> 1. Would it be easy/possible to split this patch further?

I'm not sure.

> 2. What kind of testing would provide a level of confidence
>    that no regressions have been introduced by this change?

I've just run my usual tests; the only ones relevant to this are
"cthon -a" over v3 (with and without krb5), and a few tests of my own I
wrote for some long-ago regressions
(http://git.linux-nfs.org/?p=bfields/lock-tests.git;a=summary).

(And I also did the same over v3 re-export of a 4.2 mount, but that's
more to check for the fix than for regressions.)

--b.

> 
> 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> > fs/lockd/svc4proc.c         |   4 +-
> > fs/lockd/svclock.c          |  25 ++++++---
> > fs/lockd/svcproc.c          |   4 +-
> > fs/lockd/svcsubs.c          | 104 +++++++++++++++++++++++++-----------
> > fs/nfsd/lockd.c             |   8 ++-
> > include/linux/lockd/bind.h  |   3 +-
> > include/linux/lockd/lockd.h |   9 +++-
> > 7 files changed, 114 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > index aa8eca7c38a1..c7587de948e4 100644
> > --- a/fs/lockd/svc4proc.c
> > +++ b/fs/lockd/svc4proc.c
> > @@ -40,12 +40,14 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > 
> > 	/* Obtain file pointer. Not used by FREE_ALL call. */
> > 	if (filp != NULL) {
> > +		int mode = lock_to_openmode(&lock->fl);
> > +
> > 		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
> > 			goto no_locks;
> > 		*filp = file;
> > 
> > 		/* Set up the missing parts of the file_lock structure */
> > -		lock->fl.fl_file  = file->f_file;
> > +		lock->fl.fl_file  = file->f_file[mode];
> > 		lock->fl.fl_pid = current->tgid;
> > 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> > 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index bc1cf31f3cce..d60e6eea2d57 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -471,6 +471,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> > {
> > 	struct nlm_block	*block = NULL;
> > 	int			error;
> > +	int			mode;
> > 	__be32			ret;
> > 
> > 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
> > @@ -524,7 +525,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> > 
> > 	if (!wait)
> > 		lock->fl.fl_flags &= ~FL_SLEEP;
> > -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> > +	mode = lock_to_openmode(&lock->fl);
> > +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> > 	lock->fl.fl_flags &= ~FL_SLEEP;
> > 
> > 	dprintk("lockd: vfs_lock_file returned %d\n", error);
> > @@ -577,6 +579,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > 		struct nlm_lock *conflock, struct nlm_cookie *cookie)
> > {
> > 	int			error;
> > +	int			mode;
> > 	__be32			ret;
> > 	struct nlm_lockowner	*test_owner;
> > 
> > @@ -595,7 +598,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > 	/* If there's a conflicting lock, remember to clean up the test lock */
> > 	test_owner = (struct nlm_lockowner *)lock->fl.fl_owner;
> > 
> > -	error = vfs_test_lock(file->f_file, &lock->fl);
> > +	mode = lock_to_openmode(&lock->fl);
> > +	error = vfs_test_lock(file->f_file[mode], &lock->fl);
> > 	if (error) {
> > 		/* We can't currently deal with deferred test requests */
> > 		if (error == FILE_LOCK_DEFERRED)
> > @@ -641,7 +645,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > __be32
> > nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> > {
> > -	int	error;
> > +	int	error = 0;
> > 
> > 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
> > 				nlmsvc_file_inode(file)->i_sb->s_id,
> > @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> > 	nlmsvc_cancel_blocked(net, file, lock);
> > 
> > 	lock->fl.fl_type = F_UNLCK;
> > -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> > +	if (file->f_file[0])
> > +		error = vfs_lock_file(file->f_file[0], F_SETLK,
> > +					&lock->fl, NULL);
> > +	if (file->f_file[1])
> > +		error = vfs_lock_file(file->f_file[1], F_SETLK,
> > +					&lock->fl, NULL);
> 
> Eschew raw integers :-) Should the f_file array be indexed
> using O_ flags as the comment below suggests?
> 
> 
> > 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
> > }
> > @@ -671,6 +680,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> > {
> > 	struct nlm_block	*block;
> > 	int status = 0;
> > +	int mode;
> > 
> > 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
> > 				nlmsvc_file_inode(file)->i_sb->s_id,
> > @@ -686,7 +696,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> > 	block = nlmsvc_lookup_block(file, lock);
> > 	mutex_unlock(&file->f_mutex);
> > 	if (block != NULL) {
> > -		vfs_cancel_lock(block->b_file->f_file,
> > +		mode = lock_to_openmode(&lock->fl);
> > +		vfs_cancel_lock(block->b_file->f_file[mode],
> > 				&block->b_call->a_args.lock.fl);
> > 		status = nlmsvc_unlink_block(block);
> > 		nlmsvc_release_block(block);
> > @@ -803,6 +814,7 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> > {
> > 	struct nlm_file		*file = block->b_file;
> > 	struct nlm_lock		*lock = &block->b_call->a_args.lock;
> > +	int			mode;
> > 	int			error;
> > 	loff_t			fl_start, fl_end;
> > 
> > @@ -828,7 +840,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> > 	lock->fl.fl_flags |= FL_SLEEP;
> > 	fl_start = lock->fl.fl_start;
> > 	fl_end = lock->fl.fl_end;
> > -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> > +	mode = lock_to_openmode(&lock->fl);
> > +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> > 	lock->fl.fl_flags &= ~FL_SLEEP;
> > 	lock->fl.fl_start = fl_start;
> > 	lock->fl.fl_end = fl_end;
> > diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> > index f4e5e0eb30fd..99696d3f6dd6 100644
> > --- a/fs/lockd/svcproc.c
> > +++ b/fs/lockd/svcproc.c
> > @@ -55,6 +55,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > 	struct nlm_host		*host = NULL;
> > 	struct nlm_file		*file = NULL;
> > 	struct nlm_lock		*lock = &argp->lock;
> > +	int			mode;
> > 	__be32			error = 0;
> > 
> > 	/* nfsd callbacks must have been installed for this procedure */
> > @@ -75,7 +76,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > 		*filp = file;
> > 
> > 		/* Set up the missing parts of the file_lock structure */
> > -		lock->fl.fl_file  = file->f_file;
> > +		mode = lock_to_openmode(&lock->fl);
> > +		lock->fl.fl_file  = file->f_file[mode];
> > 		lock->fl.fl_pid = current->tgid;
> > 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> > 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> > index f43d89e89c45..a0adaee245ae 100644
> > --- a/fs/lockd/svcsubs.c
> > +++ b/fs/lockd/svcsubs.c
> > @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
> > 	return tmp & (FILE_NRHASH - 1);
> > }
> > 
> > +int lock_to_openmode(struct file_lock *lock)
> > +{
> > +	if (lock->fl_type == F_WRLCK)
> > +		return O_WRONLY;
> > +	else
> > +		return O_RDONLY;
> 
> Style: a ternary would be more consistent with other areas
> of the code you change in this patch.
> 
> 
> > +}
> > +
> > +/*
> > + * Open the file. Note that if we're reexporting, for example,
> > + * this could block the lockd thread for a while.
> > + *
> > + * We have to make sure we have the right credential to open
> > + * the file.
> > + */
> > +static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
> > +			   struct nlm_file *file, int mode)
> > +{
> > +	struct file **fp = &file->f_file[mode];
> > +	__be32	nfserr;
> > +
> > +	if (*fp)
> > +		return 0;
> > +	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
> > +	if (nfserr)
> > +		dprintk("lockd: open failed (error %d)\n", nfserr);
> > +	return nfserr;
> > +}
> > +
> > /*
> >  * Lookup file info. If it doesn't exist, create a file info struct
> >  * and open a (VFS) file for the given inode.
> > - *
> > - * FIXME:
> > - * Note that we open the file O_RDONLY even when creating write locks.
> > - * This is not quite right, but for now, we assume the client performs
> > - * the proper R/W checking.
> >  */
> > __be32
> > nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> > @@ -87,41 +111,38 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> > 	struct nlm_file	*file;
> > 	unsigned int	hash;
> > 	__be32		nfserr;
> > +	int		mode;
> > 
> > 	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
> > 
> > 	hash = file_hash(&lock->fh);
> > +	mode = lock_to_openmode(&lock->fl);
> > 
> > 	/* Lock file table */
> > 	mutex_lock(&nlm_file_mutex);
> > 
> > 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
> > -		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
> > +		if (!nfs_compare_fh(&file->f_handle, &lock->fh)) {
> > +			mutex_lock(&file->f_mutex);
> > +			nfserr = nlm_do_fopen(rqstp, file, mode);
> > +			mutex_unlock(&file->f_mutex);
> > 			goto found;
> > -
> > +		}
> > 	nlm_debug_print_fh("creating file for", &lock->fh);
> > 
> > 	nfserr = nlm_lck_denied_nolocks;
> > 	file = kzalloc(sizeof(*file), GFP_KERNEL);
> > 	if (!file)
> > -		goto out_unlock;
> > +		goto out_free;
> > 
> > 	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
> > 	mutex_init(&file->f_mutex);
> > 	INIT_HLIST_NODE(&file->f_list);
> > 	INIT_LIST_HEAD(&file->f_blocks);
> > 
> > -	/*
> > -	 * Open the file. Note that if we're reexporting, for example,
> > -	 * this could block the lockd thread for a while.
> > -	 *
> > -	 * We have to make sure we have the right credential to open
> > -	 * the file.
> > -	 */
> > -	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
> > -		dprintk("lockd: open failed (error %d)\n", nfserr);
> > -		goto out_free;
> > -	}
> > +	nfserr = nlm_do_fopen(rqstp, file, mode);
> > +	if (nfserr)
> > +		goto out_unlock;
> > 
> > 	hlist_add_head(&file->f_list, &nlm_files[hash]);
> > 
> > @@ -129,7 +150,6 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> > 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
> > 	*result = file;
> > 	file->f_count++;
> > -	nfserr = 0;
> > 
> > out_unlock:
> > 	mutex_unlock(&nlm_file_mutex);
> > @@ -149,13 +169,34 @@ nlm_delete_file(struct nlm_file *file)
> > 	nlm_debug_print_file("closing file", file);
> > 	if (!hlist_unhashed(&file->f_list)) {
> > 		hlist_del(&file->f_list);
> > -		nlmsvc_ops->fclose(file->f_file);
> > +		if (file->f_file[O_RDONLY])
> > +			nlmsvc_ops->fclose(file->f_file[O_RDONLY]);
> > +		if (file->f_file[O_WRONLY])
> > +			nlmsvc_ops->fclose(file->f_file[O_WRONLY]);
> > 		kfree(file);
> > 	} else {
> > 		printk(KERN_WARNING "lockd: attempt to release unknown file!\n");
> > 	}
> > }
> > 
> > +static int nlm_unlock_files(struct nlm_file *file)
> > +{
> > +	struct file_lock lock;
> > +	struct file *f;
> > +
> > +	lock.fl_type  = F_UNLCK;
> > +	lock.fl_start = 0;
> > +	lock.fl_end   = OFFSET_MAX;
> > +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
> > +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
> > +			printk("lockd: unlock failure in %s:%d\n",
> > +				__FILE__, __LINE__);
> 
> This needs a KERN_LEVEL and maybe a _once.
> 
> 
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > /*
> >  * Loop over all locks on the given file and perform the specified
> >  * action.
> > @@ -183,17 +224,10 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
> > 
> > 		lockhost = ((struct nlm_lockowner *)fl->fl_owner)->host;
> > 		if (match(lockhost, host)) {
> > -			struct file_lock lock = *fl;
> > 
> > 			spin_unlock(&flctx->flc_lock);
> > -			lock.fl_type  = F_UNLCK;
> > -			lock.fl_start = 0;
> > -			lock.fl_end   = OFFSET_MAX;
> > -			if (vfs_lock_file(file->f_file, F_SETLK, &lock, NULL) < 0) {
> > -				printk("lockd: unlock failure in %s:%d\n",
> > -						__FILE__, __LINE__);
> > +			if (nlm_unlock_files(file))
> > 				return 1;
> > -			}
> > 			goto again;
> > 		}
> > 	}
> > @@ -247,6 +281,15 @@ nlm_file_inuse(struct nlm_file *file)
> > 	return 0;
> > }
> > 
> > +static void nlm_close_files(struct nlm_file *file)
> > +{
> > +	struct file *f;
> > +
> > +	for (f = file->f_file[0]; f <= file->f_file[1]; f++)
> > +		if (f)
> > +			nlmsvc_ops->fclose(f);
> > +}
> > +
> > /*
> >  * Loop over all files in the file table.
> >  */
> > @@ -277,7 +320,7 @@ nlm_traverse_files(void *data, nlm_host_match_fn_t match,
> > 			if (list_empty(&file->f_blocks) && !file->f_locks
> > 			 && !file->f_shares && !file->f_count) {
> > 				hlist_del(&file->f_list);
> > -				nlmsvc_ops->fclose(file->f_file);
> > +				nlm_close_files(file);
> > 				kfree(file);
> > 			}
> > 		}
> > @@ -411,6 +454,7 @@ nlmsvc_invalidate_all(void)
> > 	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
> > }
> > 
> > +
> > static int
> > nlmsvc_match_sb(void *datap, struct nlm_file *file)
> > {
> > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > index 3f5b3d7b62b7..606fa155c28a 100644
> > --- a/fs/nfsd/lockd.c
> > +++ b/fs/nfsd/lockd.c
> > @@ -25,9 +25,11 @@
> >  * Note: we hold the dentry use count while the file is open.
> >  */
> > static __be32
> > -nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> > +nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> > +		int mode)
> > {
> > 	__be32		nfserr;
> > +	int		access;
> > 	struct svc_fh	fh;
> > 
> > 	/* must initialize before using! but maxsize doesn't matter */
> > @@ -36,7 +38,9 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> > 	memcpy((char*)&fh.fh_handle.fh_base, f->data, f->size);
> > 	fh.fh_export = NULL;
> > 
> > -	nfserr = nfsd_open(rqstp, &fh, S_IFREG, NFSD_MAY_LOCK, filp);
> > +	access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
> > +	access |= NFSD_MAY_LOCK;
> > +	nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
> > 	fh_put(&fh);
> >  	/* We return nlm error codes as nlm doesn't know
> > 	 * about nfsd, but nfsd does know about nlm..
> > diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> > index 0520c0cd73f4..3bc9f7410e21 100644
> > --- a/include/linux/lockd/bind.h
> > +++ b/include/linux/lockd/bind.h
> > @@ -27,7 +27,8 @@ struct rpc_task;
> > struct nlmsvc_binding {
> > 	__be32			(*fopen)(struct svc_rqst *,
> > 						struct nfs_fh *,
> > -						struct file **);
> > +						struct file **,
> > +						int mode);
> 
> Style: "mode_t mode" might be better internal documentation.
> 
> 
> > 	void			(*fclose)(struct file *);
> > };
> > 
> > diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> > index 81b71ad2040a..da319de7e557 100644
> > --- a/include/linux/lockd/lockd.h
> > +++ b/include/linux/lockd/lockd.h
> > @@ -10,6 +10,8 @@
> > #ifndef LINUX_LOCKD_LOCKD_H
> > #define LINUX_LOCKD_LOCKD_H
> > 
> > +/* XXX: a lot of this should really be under fs/lockd. */
> > +
> > #include <linux/in.h>
> > #include <linux/in6.h>
> > #include <net/ipv6.h>
> > @@ -154,7 +156,8 @@ struct nlm_rqst {
> > struct nlm_file {
> > 	struct hlist_node	f_list;		/* linked list */
> > 	struct nfs_fh		f_handle;	/* NFS file handle */
> > -	struct file *		f_file;		/* VFS file pointer */
> > +	struct file *		f_file[2];	/* VFS file pointers,
> > +						   indexed by O_ flags */
> 
> Right, except that the new code in this patch always indexes
> f_file with raw integers, making the comment misleading. My
> preference is to keep the comment and change the new code to
> index f_file symbolically.
> 
> 
> > 	struct nlm_share *	f_shares;	/* DOS shares */
> > 	struct list_head	f_blocks;	/* blocked locks */
> > 	unsigned int		f_locks;	/* guesstimate # of locks */
> > @@ -267,6 +270,7 @@ typedef int	  (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
> > /*
> >  * Server-side lock handling
> >  */
> > +int		  lock_to_openmode(struct file_lock *);
> > __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
> > 			      struct nlm_host *, struct nlm_lock *, int,
> > 			      struct nlm_cookie *, int);
> > @@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
> > 
> > static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
> > {
> > -	return locks_inode(file->f_file);
> > +	return locks_inode(file->f_file[0] ?
> > +				file->f_file[0] : file->f_file[1]);
> > }
> > 
> > static inline int __nlm_privileged_request4(const struct sockaddr *sap)
> > -- 
> > 2.31.1
> > 
> 
> --
> Chuck Lever
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] nlm: minor nlm_lookup_file argument change
  2021-08-23 16:01     ` J. Bruce Fields
@ 2021-08-23 17:02       ` Chuck Lever III
  2021-08-23 17:21         ` Bruce Fields
  0 siblings, 1 reply; 28+ messages in thread
From: Chuck Lever III @ 2021-08-23 17:02 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List



> On Aug 23, 2021, at 12:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> Subject: [PATCH] nlm: minor style issue
> 
> Make the assignment separate.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/lockd/svc4proc.c | 3 ++-
> fs/lockd/svcsubs.c  | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
> 
>> Style: Replace the "assignment in if statement" in these spots,
>> bitte?
> 
> Feel free to fold this in if you'd prefer.--b.

Squashed. However, this change conflicts with 5/8. I've fixed
that up in my tree.

There are still several outstanding inline comments for 5/8,
in case you missed those.


> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index aa8eca7c38a1..bc496bbd696b 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -40,7 +40,8 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 
> 	/* Obtain file pointer. Not used by FREE_ALL call. */
> 	if (filp != NULL) {
> -		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
> +		error = nlm_lookup_file(rqstp, &file, lock);
> +		if (error)
> 			goto no_locks;
> 		*filp = file;
> 
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index bbd2bdde4bea..2d62633b39e5 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -117,7 +117,8 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	 * We have to make sure we have the right credential to open
> 	 * the file.
> 	 */
> -	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
> +	nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file);
> +	if (nfserr) {
> 		dprintk("lockd: open failed (error %d)\n", nfserr);
> 		goto out_free;
> 	}
> -- 
> 2.31.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] nlm: minor nlm_lookup_file argument change
  2021-08-23 17:02       ` Chuck Lever III
@ 2021-08-23 17:21         ` Bruce Fields
  0 siblings, 0 replies; 28+ messages in thread
From: Bruce Fields @ 2021-08-23 17:21 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

On Mon, Aug 23, 2021 at 05:02:19PM +0000, Chuck Lever III wrote:
> 
> 
> > On Aug 23, 2021, at 12:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > Subject: [PATCH] nlm: minor style issue
> > 
> > Make the assignment separate.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> > fs/lockd/svc4proc.c | 3 ++-
> > fs/lockd/svcsubs.c  | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> >> Style: Replace the "assignment in if statement" in these spots,
> >> bitte?
> > 
> > Feel free to fold this in if you'd prefer.--b.
> 
> Squashed. However, this change conflicts with 5/8. I've fixed
> that up in my tree.

Thanks.

> There are still several outstanding inline comments for 5/8,
> in case you missed those.

Whoops, I did.  Looking....--b.

> 
> 
> > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > index aa8eca7c38a1..bc496bbd696b 100644
> > --- a/fs/lockd/svc4proc.c
> > +++ b/fs/lockd/svc4proc.c
> > @@ -40,7 +40,8 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > 
> > 	/* Obtain file pointer. Not used by FREE_ALL call. */
> > 	if (filp != NULL) {
> > -		if ((error = nlm_lookup_file(rqstp, &file, lock)) != 0)
> > +		error = nlm_lookup_file(rqstp, &file, lock);
> > +		if (error)
> > 			goto no_locks;
> > 		*filp = file;
> > 
> > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> > index bbd2bdde4bea..2d62633b39e5 100644
> > --- a/fs/lockd/svcsubs.c
> > +++ b/fs/lockd/svcsubs.c
> > @@ -117,7 +117,8 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> > 	 * We have to make sure we have the right credential to open
> > 	 * the file.
> > 	 */
> > -	if ((nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file)) != 0) {
> > +	nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file);
> > +	if (nfserr) {
> > 		dprintk("lockd: open failed (error %d)\n", nfserr);
> > 		goto out_free;
> > 	}
> > -- 
> > 2.31.1
> > 
> 
> --
> Chuck Lever
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-21 16:30   ` Chuck Lever III
  2021-08-23 16:08     ` J. Bruce Fields
@ 2021-08-23 18:57     ` J. Bruce Fields
  2021-08-23 18:59       ` Chuck Lever III
  1 sibling, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-23 18:57 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

On Sat, Aug 21, 2021 at 04:30:43PM +0000, Chuck Lever III wrote:
> > @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> > 	nlmsvc_cancel_blocked(net, file, lock);
> > 
> > 	lock->fl.fl_type = F_UNLCK;
> > -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> > +	if (file->f_file[0])
> > +		error = vfs_lock_file(file->f_file[0], F_SETLK,
> > +					&lock->fl, NULL);
> > +	if (file->f_file[1])
> > +		error = vfs_lock_file(file->f_file[1], F_SETLK,
> > +					&lock->fl, NULL);
> 
> Eschew raw integers :-) Should the f_file array be indexed
> using O_ flags as the comment below suggests?

Sure, done.

> > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> > index f43d89e89c45..a0adaee245ae 100644
> > --- a/fs/lockd/svcsubs.c
> > +++ b/fs/lockd/svcsubs.c
> > @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
> > 	return tmp & (FILE_NRHASH - 1);
> > }
> > 
> > +int lock_to_openmode(struct file_lock *lock)
> > +{
> > +	if (lock->fl_type == F_WRLCK)
> > +		return O_WRONLY;
> > +	else
> > +		return O_RDONLY;
> 
> Style: a ternary would be more consistent with other areas
> of the code you change in this patch.

OK.

> > +static int nlm_unlock_files(struct nlm_file *file)
> > +{
> > +	struct file_lock lock;
> > +	struct file *f;
> > +
> > +	lock.fl_type  = F_UNLCK;
> > +	lock.fl_start = 0;
> > +	lock.fl_end   = OFFSET_MAX;
> > +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
> > +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
> > +			printk("lockd: unlock failure in %s:%d\n",
> > +				__FILE__, __LINE__);
> 
> This needs a KERN_LEVEL and maybe a _once.

How about going with pr_warn for now.  I don't *think* there's much
danger of spamming the logs from this one.  And I'm wondering there
might be some causes (unresponsive server?) that could resolve
themselves, and then come back, and that you'd still want to hear about
the second time.

> > @@ -27,7 +27,8 @@ struct rpc_task;
> > struct nlmsvc_binding {
> > 	__be32			(*fopen)(struct svc_rqst *,
> > 						struct nfs_fh *,
> > -						struct file **);
> > +						struct file **,
> > +						int mode);
> 
> Style: "mode_t mode" might be better internal documentation.

It's confusing that we use the word "mode" both for "access mode" (O_
flags) and "mode bits" (permission bits).  This is the former, and I
thought mode_t was the for the latter.

> > @@ -154,7 +156,8 @@ struct nlm_rqst {
> > struct nlm_file {
> > 	struct hlist_node	f_list;		/* linked list */
> > 	struct nfs_fh		f_handle;	/* NFS file handle */
> > -	struct file *		f_file;		/* VFS file pointer */
> > +	struct file *		f_file[2];	/* VFS file pointers,
> > +						   indexed by O_ flags */
> 
> Right, except that the new code in this patch always indexes
> f_file with raw integers, making the comment misleading. My
> preference is to keep the comment and change the new code to
> index f_file symbolically.

OK.

--b.

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-23 18:57     ` J. Bruce Fields
@ 2021-08-23 18:59       ` Chuck Lever III
  2021-08-23 20:44         ` Bruce Fields
  0 siblings, 1 reply; 28+ messages in thread
From: Chuck Lever III @ 2021-08-23 18:59 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List



> On Aug 23, 2021, at 2:57 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Sat, Aug 21, 2021 at 04:30:43PM +0000, Chuck Lever III wrote:
>>> @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
>>> 	nlmsvc_cancel_blocked(net, file, lock);
>>> 
>>> 	lock->fl.fl_type = F_UNLCK;
>>> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
>>> +	if (file->f_file[0])
>>> +		error = vfs_lock_file(file->f_file[0], F_SETLK,
>>> +					&lock->fl, NULL);
>>> +	if (file->f_file[1])
>>> +		error = vfs_lock_file(file->f_file[1], F_SETLK,
>>> +					&lock->fl, NULL);
>> 
>> Eschew raw integers :-) Should the f_file array be indexed
>> using O_ flags as the comment below suggests?
> 
> Sure, done.
> 
>>> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
>>> index f43d89e89c45..a0adaee245ae 100644
>>> --- a/fs/lockd/svcsubs.c
>>> +++ b/fs/lockd/svcsubs.c
>>> @@ -71,14 +71,38 @@ static inline unsigned int file_hash(struct nfs_fh *f)
>>> 	return tmp & (FILE_NRHASH - 1);
>>> }
>>> 
>>> +int lock_to_openmode(struct file_lock *lock)
>>> +{
>>> +	if (lock->fl_type == F_WRLCK)
>>> +		return O_WRONLY;
>>> +	else
>>> +		return O_RDONLY;
>> 
>> Style: a ternary would be more consistent with other areas
>> of the code you change in this patch.
> 
> OK.
> 
>>> +static int nlm_unlock_files(struct nlm_file *file)
>>> +{
>>> +	struct file_lock lock;
>>> +	struct file *f;
>>> +
>>> +	lock.fl_type  = F_UNLCK;
>>> +	lock.fl_start = 0;
>>> +	lock.fl_end   = OFFSET_MAX;
>>> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
>>> +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
>>> +			printk("lockd: unlock failure in %s:%d\n",
>>> +				__FILE__, __LINE__);
>> 
>> This needs a KERN_LEVEL and maybe a _once.
> 
> How about going with pr_warn for now.

Sold.


> I don't *think* there's much
> danger of spamming the logs from this one.  And I'm wondering there
> might be some causes (unresponsive server?) that could resolve
> themselves, and then come back, and that you'd still want to hear about
> the second time.
> 
>>> @@ -27,7 +27,8 @@ struct rpc_task;
>>> struct nlmsvc_binding {
>>> 	__be32			(*fopen)(struct svc_rqst *,
>>> 						struct nfs_fh *,
>>> -						struct file **);
>>> +						struct file **,
>>> +						int mode);
>> 
>> Style: "mode_t mode" might be better internal documentation.
> 
> It's confusing that we use the word "mode" both for "access mode" (O_
> flags) and "mode bits" (permission bits).  This is the former, and I
> thought mode_t was the for the latter.

Fair enough.


> 
>>> @@ -154,7 +156,8 @@ struct nlm_rqst {
>>> struct nlm_file {
>>> 	struct hlist_node	f_list;		/* linked list */
>>> 	struct nfs_fh		f_handle;	/* NFS file handle */
>>> -	struct file *		f_file;		/* VFS file pointer */
>>> +	struct file *		f_file[2];	/* VFS file pointers,
>>> +						   indexed by O_ flags */
>> 
>> Right, except that the new code in this patch always indexes
>> f_file with raw integers, making the comment misleading. My
>> preference is to keep the comment and change the new code to
>> index f_file symbolically.
> 
> OK.
> 
> --b.

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-23 18:59       ` Chuck Lever III
@ 2021-08-23 20:44         ` Bruce Fields
  2021-08-23 21:54           ` Chuck Lever III
  0 siblings, 1 reply; 28+ messages in thread
From: Bruce Fields @ 2021-08-23 20:44 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

From: "J. Bruce Fields" <bfields@redhat.com>
Subject: [PATCH v4] Keep read and write fds with each nlm_file

We shouldn't really be using a read-only file descriptor to take a write
lock.

Most filesystems will put up with it.  But NFS, for example, won't.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svc4proc.c         |   4 +-
 fs/lockd/svclock.c          |  25 ++++++---
 fs/lockd/svcproc.c          |   4 +-
 fs/lockd/svcsubs.c          | 102 +++++++++++++++++++++++++-----------
 fs/nfsd/lockd.c             |   8 ++-
 include/linux/lockd/bind.h  |   3 +-
 include/linux/lockd/lockd.h |   9 +++-
 7 files changed, 111 insertions(+), 44 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index bc496bbd696b..e10ae2c41279 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -40,13 +40,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 	/* Obtain file pointer. Not used by FREE_ALL call. */
 	if (filp != NULL) {
+		int mode = lock_to_openmode(&lock->fl);
+
 		error = nlm_lookup_file(rqstp, &file, lock);
 		if (error)
 			goto no_locks;
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
-		lock->fl.fl_file  = file->f_file;
+		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index bc1cf31f3cce..6abbc92b88e3 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -471,6 +471,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 {
 	struct nlm_block	*block = NULL;
 	int			error;
+	int			mode;
 	__be32			ret;
 
 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
@@ -524,7 +525,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 
 	if (!wait)
 		lock->fl.fl_flags &= ~FL_SLEEP;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
 	lock->fl.fl_flags &= ~FL_SLEEP;
 
 	dprintk("lockd: vfs_lock_file returned %d\n", error);
@@ -577,6 +579,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		struct nlm_lock *conflock, struct nlm_cookie *cookie)
 {
 	int			error;
+	int			mode;
 	__be32			ret;
 	struct nlm_lockowner	*test_owner;
 
@@ -595,7 +598,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	/* If there's a conflicting lock, remember to clean up the test lock */
 	test_owner = (struct nlm_lockowner *)lock->fl.fl_owner;
 
-	error = vfs_test_lock(file->f_file, &lock->fl);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_test_lock(file->f_file[mode], &lock->fl);
 	if (error) {
 		/* We can't currently deal with deferred test requests */
 		if (error == FILE_LOCK_DEFERRED)
@@ -641,7 +645,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 __be32
 nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 {
-	int	error;
+	int	error = 0;
 
 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
 				nlmsvc_file_inode(file)->i_sb->s_id,
@@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 	nlmsvc_cancel_blocked(net, file, lock);
 
 	lock->fl.fl_type = F_UNLCK;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	if (file->f_file[O_RDONLY])
+		error = vfs_lock_file(file->f_file[O_RDONLY], F_SETLK,
+					&lock->fl, NULL);
+	if (file->f_file[O_WRONLY])
+		error = vfs_lock_file(file->f_file[O_WRONLY], F_SETLK,
+					&lock->fl, NULL);
 
 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
 }
@@ -671,6 +680,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 {
 	struct nlm_block	*block;
 	int status = 0;
+	int mode;
 
 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
 				nlmsvc_file_inode(file)->i_sb->s_id,
@@ -686,7 +696,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 	block = nlmsvc_lookup_block(file, lock);
 	mutex_unlock(&file->f_mutex);
 	if (block != NULL) {
-		vfs_cancel_lock(block->b_file->f_file,
+		mode = lock_to_openmode(&lock->fl);
+		vfs_cancel_lock(block->b_file->f_file[mode],
 				&block->b_call->a_args.lock.fl);
 		status = nlmsvc_unlink_block(block);
 		nlmsvc_release_block(block);
@@ -803,6 +814,7 @@ nlmsvc_grant_blocked(struct nlm_block *block)
 {
 	struct nlm_file		*file = block->b_file;
 	struct nlm_lock		*lock = &block->b_call->a_args.lock;
+	int			mode;
 	int			error;
 	loff_t			fl_start, fl_end;
 
@@ -828,7 +840,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
 	lock->fl.fl_flags |= FL_SLEEP;
 	fl_start = lock->fl.fl_start;
 	fl_end = lock->fl.fl_end;
-	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
+	mode = lock_to_openmode(&lock->fl);
+	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
 	lock->fl.fl_flags &= ~FL_SLEEP;
 	lock->fl.fl_start = fl_start;
 	lock->fl.fl_end = fl_end;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index f4e5e0eb30fd..99696d3f6dd6 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -55,6 +55,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 	struct nlm_host		*host = NULL;
 	struct nlm_file		*file = NULL;
 	struct nlm_lock		*lock = &argp->lock;
+	int			mode;
 	__be32			error = 0;
 
 	/* nfsd callbacks must have been installed for this procedure */
@@ -75,7 +76,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
-		lock->fl.fl_file  = file->f_file;
+		mode = lock_to_openmode(&lock->fl);
+		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 13e6ffc219ec..cb3a7512c33e 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -71,14 +71,35 @@ static inline unsigned int file_hash(struct nfs_fh *f)
 	return tmp & (FILE_NRHASH - 1);
 }
 
+int lock_to_openmode(struct file_lock *lock)
+{
+	return (lock->fl_type == F_WRLCK) ? O_WRONLY : O_RDONLY;
+}
+
+/*
+ * Open the file. Note that if we're reexporting, for example,
+ * this could block the lockd thread for a while.
+ *
+ * We have to make sure we have the right credential to open
+ * the file.
+ */
+static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
+			   struct nlm_file *file, int mode)
+{
+	struct file **fp = &file->f_file[mode];
+	__be32	nfserr;
+
+	if (*fp)
+		return 0;
+	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
+	if (nfserr)
+		dprintk("lockd: open failed (error %d)\n", nfserr);
+	return nfserr;
+}
+
 /*
  * Lookup file info. If it doesn't exist, create a file info struct
  * and open a (VFS) file for the given inode.
- *
- * FIXME:
- * Note that we open the file O_RDONLY even when creating write locks.
- * This is not quite right, but for now, we assume the client performs
- * the proper R/W checking.
  */
 __be32
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
@@ -87,42 +108,38 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	struct nlm_file	*file;
 	unsigned int	hash;
 	__be32		nfserr;
+	int		mode;
 
 	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
 
 	hash = file_hash(&lock->fh);
+	mode = lock_to_openmode(&lock->fl);
 
 	/* Lock file table */
 	mutex_lock(&nlm_file_mutex);
 
 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
-		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
+		if (!nfs_compare_fh(&file->f_handle, &lock->fh)) {
+			mutex_lock(&file->f_mutex);
+			nfserr = nlm_do_fopen(rqstp, file, mode);
+			mutex_unlock(&file->f_mutex);
 			goto found;
-
+		}
 	nlm_debug_print_fh("creating file for", &lock->fh);
 
 	nfserr = nlm_lck_denied_nolocks;
 	file = kzalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
-		goto out_unlock;
+		goto out_free;
 
 	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
 	mutex_init(&file->f_mutex);
 	INIT_HLIST_NODE(&file->f_list);
 	INIT_LIST_HEAD(&file->f_blocks);
 
-	/*
-	 * Open the file. Note that if we're reexporting, for example,
-	 * this could block the lockd thread for a while.
-	 *
-	 * We have to make sure we have the right credential to open
-	 * the file.
-	 */
-	nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file);
-	if (nfserr) {
-		dprintk("lockd: open failed (error %d)\n", nfserr);
-		goto out_free;
-	}
+	nfserr = nlm_do_fopen(rqstp, file, mode);
+	if (nfserr)
+		goto out_unlock;
 
 	hlist_add_head(&file->f_list, &nlm_files[hash]);
 
@@ -130,7 +147,6 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
 	*result = file;
 	file->f_count++;
-	nfserr = 0;
 
 out_unlock:
 	mutex_unlock(&nlm_file_mutex);
@@ -150,13 +166,34 @@ nlm_delete_file(struct nlm_file *file)
 	nlm_debug_print_file("closing file", file);
 	if (!hlist_unhashed(&file->f_list)) {
 		hlist_del(&file->f_list);
-		nlmsvc_ops->fclose(file->f_file);
+		if (file->f_file[O_RDONLY])
+			nlmsvc_ops->fclose(file->f_file[O_RDONLY]);
+		if (file->f_file[O_WRONLY])
+			nlmsvc_ops->fclose(file->f_file[O_WRONLY]);
 		kfree(file);
 	} else {
 		printk(KERN_WARNING "lockd: attempt to release unknown file!\n");
 	}
 }
 
+static int nlm_unlock_files(struct nlm_file *file)
+{
+	struct file_lock lock;
+	struct file *f;
+
+	lock.fl_type  = F_UNLCK;
+	lock.fl_start = 0;
+	lock.fl_end   = OFFSET_MAX;
+	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
+		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
+			pr_warn("lockd: unlock failure in %s:%d\n",
+				__FILE__, __LINE__);
+			return 1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Loop over all locks on the given file and perform the specified
  * action.
@@ -184,17 +221,10 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
 
 		lockhost = ((struct nlm_lockowner *)fl->fl_owner)->host;
 		if (match(lockhost, host)) {
-			struct file_lock lock = *fl;
 
 			spin_unlock(&flctx->flc_lock);
-			lock.fl_type  = F_UNLCK;
-			lock.fl_start = 0;
-			lock.fl_end   = OFFSET_MAX;
-			if (vfs_lock_file(file->f_file, F_SETLK, &lock, NULL) < 0) {
-				printk("lockd: unlock failure in %s:%d\n",
-						__FILE__, __LINE__);
+			if (nlm_unlock_files(file))
 				return 1;
-			}
 			goto again;
 		}
 	}
@@ -248,6 +278,15 @@ nlm_file_inuse(struct nlm_file *file)
 	return 0;
 }
 
+static void nlm_close_files(struct nlm_file *file)
+{
+	struct file *f;
+
+	for (f = file->f_file[0]; f <= file->f_file[1]; f++)
+		if (f)
+			nlmsvc_ops->fclose(f);
+}
+
 /*
  * Loop over all files in the file table.
  */
@@ -278,7 +317,7 @@ nlm_traverse_files(void *data, nlm_host_match_fn_t match,
 			if (list_empty(&file->f_blocks) && !file->f_locks
 			 && !file->f_shares && !file->f_count) {
 				hlist_del(&file->f_list);
-				nlmsvc_ops->fclose(file->f_file);
+				nlm_close_files(file);
 				kfree(file);
 			}
 		}
@@ -412,6 +451,7 @@ nlmsvc_invalidate_all(void)
 	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
 }
 
+
 static int
 nlmsvc_match_sb(void *datap, struct nlm_file *file)
 {
diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
index 3f5b3d7b62b7..606fa155c28a 100644
--- a/fs/nfsd/lockd.c
+++ b/fs/nfsd/lockd.c
@@ -25,9 +25,11 @@
  * Note: we hold the dentry use count while the file is open.
  */
 static __be32
-nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
+nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
+		int mode)
 {
 	__be32		nfserr;
+	int		access;
 	struct svc_fh	fh;
 
 	/* must initialize before using! but maxsize doesn't matter */
@@ -36,7 +38,9 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
 	memcpy((char*)&fh.fh_handle.fh_base, f->data, f->size);
 	fh.fh_export = NULL;
 
-	nfserr = nfsd_open(rqstp, &fh, S_IFREG, NFSD_MAY_LOCK, filp);
+	access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
+	access |= NFSD_MAY_LOCK;
+	nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
 	fh_put(&fh);
  	/* We return nlm error codes as nlm doesn't know
 	 * about nfsd, but nfsd does know about nlm..
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 0520c0cd73f4..3bc9f7410e21 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -27,7 +27,8 @@ struct rpc_task;
 struct nlmsvc_binding {
 	__be32			(*fopen)(struct svc_rqst *,
 						struct nfs_fh *,
-						struct file **);
+						struct file **,
+						int mode);
 	void			(*fclose)(struct file *);
 };
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 81b71ad2040a..da319de7e557 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -10,6 +10,8 @@
 #ifndef LINUX_LOCKD_LOCKD_H
 #define LINUX_LOCKD_LOCKD_H
 
+/* XXX: a lot of this should really be under fs/lockd. */
+
 #include <linux/in.h>
 #include <linux/in6.h>
 #include <net/ipv6.h>
@@ -154,7 +156,8 @@ struct nlm_rqst {
 struct nlm_file {
 	struct hlist_node	f_list;		/* linked list */
 	struct nfs_fh		f_handle;	/* NFS file handle */
-	struct file *		f_file;		/* VFS file pointer */
+	struct file *		f_file[2];	/* VFS file pointers,
+						   indexed by O_ flags */
 	struct nlm_share *	f_shares;	/* DOS shares */
 	struct list_head	f_blocks;	/* blocked locks */
 	unsigned int		f_locks;	/* guesstimate # of locks */
@@ -267,6 +270,7 @@ typedef int	  (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
 /*
  * Server-side lock handling
  */
+int		  lock_to_openmode(struct file_lock *);
 __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
 			      struct nlm_host *, struct nlm_lock *, int,
 			      struct nlm_cookie *, int);
@@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
 
 static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
 {
-	return locks_inode(file->f_file);
+	return locks_inode(file->f_file[0] ?
+				file->f_file[0] : file->f_file[1]);
 }
 
 static inline int __nlm_privileged_request4(const struct sockaddr *sap)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-23 20:44         ` Bruce Fields
@ 2021-08-23 21:54           ` Chuck Lever III
  2021-08-23 21:58             ` Bruce Fields
  0 siblings, 1 reply; 28+ messages in thread
From: Chuck Lever III @ 2021-08-23 21:54 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List



> On Aug 23, 2021, at 4:44 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> Subject: [PATCH v4] Keep read and write fds with each nlm_file
> 
> We shouldn't really be using a read-only file descriptor to take a write
> lock.
> 
> Most filesystems will put up with it.  But NFS, for example, won't.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/lockd/svc4proc.c         |   4 +-
> fs/lockd/svclock.c          |  25 ++++++---
> fs/lockd/svcproc.c          |   4 +-
> fs/lockd/svcsubs.c          | 102 +++++++++++++++++++++++++-----------
> fs/nfsd/lockd.c             |   8 ++-
> include/linux/lockd/bind.h  |   3 +-
> include/linux/lockd/lockd.h |   9 +++-
> 7 files changed, 111 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index bc496bbd696b..e10ae2c41279 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -40,13 +40,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 
> 	/* Obtain file pointer. Not used by FREE_ALL call. */
> 	if (filp != NULL) {
> +		int mode = lock_to_openmode(&lock->fl);
> +
> 		error = nlm_lookup_file(rqstp, &file, lock);
> 		if (error)
> 			goto no_locks;
> 		*filp = file;
> 
> 		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.fl_file  = file->f_file;
> +		lock->fl.fl_file  = file->f_file[mode];
> 		lock->fl.fl_pid = current->tgid;
> 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index bc1cf31f3cce..6abbc92b88e3 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -471,6 +471,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> {
> 	struct nlm_block	*block = NULL;
> 	int			error;
> +	int			mode;
> 	__be32			ret;
> 
> 	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
> @@ -524,7 +525,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> 
> 	if (!wait)
> 		lock->fl.fl_flags &= ~FL_SLEEP;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> 	lock->fl.fl_flags &= ~FL_SLEEP;
> 
> 	dprintk("lockd: vfs_lock_file returned %d\n", error);
> @@ -577,6 +579,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 		struct nlm_lock *conflock, struct nlm_cookie *cookie)
> {
> 	int			error;
> +	int			mode;
> 	__be32			ret;
> 	struct nlm_lockowner	*test_owner;
> 
> @@ -595,7 +598,8 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> 	/* If there's a conflicting lock, remember to clean up the test lock */
> 	test_owner = (struct nlm_lockowner *)lock->fl.fl_owner;
> 
> -	error = vfs_test_lock(file->f_file, &lock->fl);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_test_lock(file->f_file[mode], &lock->fl);
> 	if (error) {
> 		/* We can't currently deal with deferred test requests */
> 		if (error == FILE_LOCK_DEFERRED)
> @@ -641,7 +645,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> __be32
> nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> {
> -	int	error;
> +	int	error = 0;
> 
> 	dprintk("lockd: nlmsvc_unlock(%s/%ld, pi=%d, %Ld-%Ld)\n",
> 				nlmsvc_file_inode(file)->i_sb->s_id,
> @@ -654,7 +658,12 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
> 	nlmsvc_cancel_blocked(net, file, lock);
> 
> 	lock->fl.fl_type = F_UNLCK;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	if (file->f_file[O_RDONLY])
> +		error = vfs_lock_file(file->f_file[O_RDONLY], F_SETLK,
> +					&lock->fl, NULL);
> +	if (file->f_file[O_WRONLY])
> +		error = vfs_lock_file(file->f_file[O_WRONLY], F_SETLK,
> +					&lock->fl, NULL);
> 
> 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
> }
> @@ -671,6 +680,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> {
> 	struct nlm_block	*block;
> 	int status = 0;
> +	int mode;
> 
> 	dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
> 				nlmsvc_file_inode(file)->i_sb->s_id,
> @@ -686,7 +696,8 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> 	block = nlmsvc_lookup_block(file, lock);
> 	mutex_unlock(&file->f_mutex);
> 	if (block != NULL) {
> -		vfs_cancel_lock(block->b_file->f_file,
> +		mode = lock_to_openmode(&lock->fl);
> +		vfs_cancel_lock(block->b_file->f_file[mode],
> 				&block->b_call->a_args.lock.fl);
> 		status = nlmsvc_unlink_block(block);
> 		nlmsvc_release_block(block);
> @@ -803,6 +814,7 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> {
> 	struct nlm_file		*file = block->b_file;
> 	struct nlm_lock		*lock = &block->b_call->a_args.lock;
> +	int			mode;
> 	int			error;
> 	loff_t			fl_start, fl_end;
> 
> @@ -828,7 +840,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
> 	lock->fl.fl_flags |= FL_SLEEP;
> 	fl_start = lock->fl.fl_start;
> 	fl_end = lock->fl.fl_end;
> -	error = vfs_lock_file(file->f_file, F_SETLK, &lock->fl, NULL);
> +	mode = lock_to_openmode(&lock->fl);
> +	error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> 	lock->fl.fl_flags &= ~FL_SLEEP;
> 	lock->fl.fl_start = fl_start;
> 	lock->fl.fl_end = fl_end;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index f4e5e0eb30fd..99696d3f6dd6 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -55,6 +55,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 	struct nlm_host		*host = NULL;
> 	struct nlm_file		*file = NULL;
> 	struct nlm_lock		*lock = &argp->lock;
> +	int			mode;
> 	__be32			error = 0;
> 
> 	/* nfsd callbacks must have been installed for this procedure */
> @@ -75,7 +76,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> 		*filp = file;
> 
> 		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.fl_file  = file->f_file;
> +		mode = lock_to_openmode(&lock->fl);
> +		lock->fl.fl_file  = file->f_file[mode];
> 		lock->fl.fl_pid = current->tgid;
> 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
> 		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index 13e6ffc219ec..cb3a7512c33e 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -71,14 +71,35 @@ static inline unsigned int file_hash(struct nfs_fh *f)
> 	return tmp & (FILE_NRHASH - 1);
> }
> 
> +int lock_to_openmode(struct file_lock *lock)
> +{
> +	return (lock->fl_type == F_WRLCK) ? O_WRONLY : O_RDONLY;
> +}
> +
> +/*
> + * Open the file. Note that if we're reexporting, for example,
> + * this could block the lockd thread for a while.
> + *
> + * We have to make sure we have the right credential to open
> + * the file.
> + */
> +static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
> +			   struct nlm_file *file, int mode)
> +{
> +	struct file **fp = &file->f_file[mode];
> +	__be32	nfserr;
> +
> +	if (*fp)
> +		return 0;
> +	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
> +	if (nfserr)
> +		dprintk("lockd: open failed (error %d)\n", nfserr);
> +	return nfserr;
> +}
> +
> /*
>  * Lookup file info. If it doesn't exist, create a file info struct
>  * and open a (VFS) file for the given inode.
> - *
> - * FIXME:
> - * Note that we open the file O_RDONLY even when creating write locks.
> - * This is not quite right, but for now, we assume the client performs
> - * the proper R/W checking.
>  */
> __be32
> nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> @@ -87,42 +108,38 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	struct nlm_file	*file;
> 	unsigned int	hash;
> 	__be32		nfserr;
> +	int		mode;
> 
> 	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
> 
> 	hash = file_hash(&lock->fh);
> +	mode = lock_to_openmode(&lock->fl);
> 
> 	/* Lock file table */
> 	mutex_lock(&nlm_file_mutex);
> 
> 	hlist_for_each_entry(file, &nlm_files[hash], f_list)
> -		if (!nfs_compare_fh(&file->f_handle, &lock->fh))
> +		if (!nfs_compare_fh(&file->f_handle, &lock->fh)) {
> +			mutex_lock(&file->f_mutex);
> +			nfserr = nlm_do_fopen(rqstp, file, mode);
> +			mutex_unlock(&file->f_mutex);
> 			goto found;
> -
> +		}
> 	nlm_debug_print_fh("creating file for", &lock->fh);
> 
> 	nfserr = nlm_lck_denied_nolocks;
> 	file = kzalloc(sizeof(*file), GFP_KERNEL);
> 	if (!file)
> -		goto out_unlock;
> +		goto out_free;
> 
> 	memcpy(&file->f_handle, &lock->fh, sizeof(struct nfs_fh));
> 	mutex_init(&file->f_mutex);
> 	INIT_HLIST_NODE(&file->f_list);
> 	INIT_LIST_HEAD(&file->f_blocks);
> 
> -	/*
> -	 * Open the file. Note that if we're reexporting, for example,
> -	 * this could block the lockd thread for a while.
> -	 *
> -	 * We have to make sure we have the right credential to open
> -	 * the file.
> -	 */
> -	nfserr = nlmsvc_ops->fopen(rqstp, &lock->fh, &file->f_file);
> -	if (nfserr) {
> -		dprintk("lockd: open failed (error %d)\n", nfserr);
> -		goto out_free;
> -	}
> +	nfserr = nlm_do_fopen(rqstp, file, mode);
> +	if (nfserr)
> +		goto out_unlock;
> 
> 	hlist_add_head(&file->f_list, &nlm_files[hash]);
> 
> @@ -130,7 +147,6 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> 	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
> 	*result = file;
> 	file->f_count++;
> -	nfserr = 0;
> 
> out_unlock:
> 	mutex_unlock(&nlm_file_mutex);
> @@ -150,13 +166,34 @@ nlm_delete_file(struct nlm_file *file)
> 	nlm_debug_print_file("closing file", file);
> 	if (!hlist_unhashed(&file->f_list)) {
> 		hlist_del(&file->f_list);
> -		nlmsvc_ops->fclose(file->f_file);
> +		if (file->f_file[O_RDONLY])
> +			nlmsvc_ops->fclose(file->f_file[O_RDONLY]);
> +		if (file->f_file[O_WRONLY])
> +			nlmsvc_ops->fclose(file->f_file[O_WRONLY]);
> 		kfree(file);
> 	} else {
> 		printk(KERN_WARNING "lockd: attempt to release unknown file!\n");
> 	}
> }
> 
> +static int nlm_unlock_files(struct nlm_file *file)
> +{
> +	struct file_lock lock;
> +	struct file *f;
> +
> +	lock.fl_type  = F_UNLCK;
> +	lock.fl_start = 0;
> +	lock.fl_end   = OFFSET_MAX;
> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {

O_RDONLY and O_WRONLY ?


> +		if (f && vfs_lock_file(f, F_SETLK, &lock, NULL) < 0) {
> +			pr_warn("lockd: unlock failure in %s:%d\n",
> +				__FILE__, __LINE__);
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> /*
>  * Loop over all locks on the given file and perform the specified
>  * action.
> @@ -184,17 +221,10 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
> 
> 		lockhost = ((struct nlm_lockowner *)fl->fl_owner)->host;
> 		if (match(lockhost, host)) {
> -			struct file_lock lock = *fl;
> 
> 			spin_unlock(&flctx->flc_lock);
> -			lock.fl_type  = F_UNLCK;
> -			lock.fl_start = 0;
> -			lock.fl_end   = OFFSET_MAX;
> -			if (vfs_lock_file(file->f_file, F_SETLK, &lock, NULL) < 0) {
> -				printk("lockd: unlock failure in %s:%d\n",
> -						__FILE__, __LINE__);
> +			if (nlm_unlock_files(file))
> 				return 1;
> -			}
> 			goto again;
> 		}
> 	}
> @@ -248,6 +278,15 @@ nlm_file_inuse(struct nlm_file *file)
> 	return 0;
> }
> 
> +static void nlm_close_files(struct nlm_file *file)
> +{
> +	struct file *f;
> +
> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++)

O_RDONLY and O_WRONLY ?


> +		if (f)
> +			nlmsvc_ops->fclose(f);
> +}
> +
> /*
>  * Loop over all files in the file table.
>  */
> @@ -278,7 +317,7 @@ nlm_traverse_files(void *data, nlm_host_match_fn_t match,
> 			if (list_empty(&file->f_blocks) && !file->f_locks
> 			 && !file->f_shares && !file->f_count) {
> 				hlist_del(&file->f_list);
> -				nlmsvc_ops->fclose(file->f_file);
> +				nlm_close_files(file);
> 				kfree(file);
> 			}
> 		}
> @@ -412,6 +451,7 @@ nlmsvc_invalidate_all(void)
> 	nlm_traverse_files(NULL, nlmsvc_is_client, NULL);
> }
> 
> +
> static int
> nlmsvc_match_sb(void *datap, struct nlm_file *file)
> {
> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> index 3f5b3d7b62b7..606fa155c28a 100644
> --- a/fs/nfsd/lockd.c
> +++ b/fs/nfsd/lockd.c
> @@ -25,9 +25,11 @@
>  * Note: we hold the dentry use count while the file is open.
>  */
> static __be32
> -nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> +nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> +		int mode)
> {
> 	__be32		nfserr;
> +	int		access;
> 	struct svc_fh	fh;
> 
> 	/* must initialize before using! but maxsize doesn't matter */
> @@ -36,7 +38,9 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp)
> 	memcpy((char*)&fh.fh_handle.fh_base, f->data, f->size);
> 	fh.fh_export = NULL;
> 
> -	nfserr = nfsd_open(rqstp, &fh, S_IFREG, NFSD_MAY_LOCK, filp);
> +	access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
> +	access |= NFSD_MAY_LOCK;
> +	nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
> 	fh_put(&fh);
>  	/* We return nlm error codes as nlm doesn't know
> 	 * about nfsd, but nfsd does know about nlm..
> diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
> index 0520c0cd73f4..3bc9f7410e21 100644
> --- a/include/linux/lockd/bind.h
> +++ b/include/linux/lockd/bind.h
> @@ -27,7 +27,8 @@ struct rpc_task;
> struct nlmsvc_binding {
> 	__be32			(*fopen)(struct svc_rqst *,
> 						struct nfs_fh *,
> -						struct file **);
> +						struct file **,
> +						int mode);
> 	void			(*fclose)(struct file *);
> };
> 
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 81b71ad2040a..da319de7e557 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -10,6 +10,8 @@
> #ifndef LINUX_LOCKD_LOCKD_H
> #define LINUX_LOCKD_LOCKD_H
> 
> +/* XXX: a lot of this should really be under fs/lockd. */
> +
> #include <linux/in.h>
> #include <linux/in6.h>
> #include <net/ipv6.h>
> @@ -154,7 +156,8 @@ struct nlm_rqst {
> struct nlm_file {
> 	struct hlist_node	f_list;		/* linked list */
> 	struct nfs_fh		f_handle;	/* NFS file handle */
> -	struct file *		f_file;		/* VFS file pointer */
> +	struct file *		f_file[2];	/* VFS file pointers,
> +						   indexed by O_ flags */
> 	struct nlm_share *	f_shares;	/* DOS shares */
> 	struct list_head	f_blocks;	/* blocked locks */
> 	unsigned int		f_locks;	/* guesstimate # of locks */
> @@ -267,6 +270,7 @@ typedef int	  (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
> /*
>  * Server-side lock handling
>  */
> +int		  lock_to_openmode(struct file_lock *);
> __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
> 			      struct nlm_host *, struct nlm_lock *, int,
> 			      struct nlm_cookie *, int);
> @@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
> 
> static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
> {
> -	return locks_inode(file->f_file);
> +	return locks_inode(file->f_file[0] ?
> +				file->f_file[0] : file->f_file[1]);

O_RDONLY and O_WRONLY ?


> }
> 
> static inline int __nlm_privileged_request4(const struct sockaddr *sap)
> -- 
> 2.31.1
> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-23 21:54           ` Chuck Lever III
@ 2021-08-23 21:58             ` Bruce Fields
  2021-08-23 22:00               ` Chuck Lever III
  0 siblings, 1 reply; 28+ messages in thread
From: Bruce Fields @ 2021-08-23 21:58 UTC (permalink / raw)
  To: Chuck Lever III
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List

On Mon, Aug 23, 2021 at 09:54:20PM +0000, Chuck Lever III wrote:
> > On Aug 23, 2021, at 4:44 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > +static int nlm_unlock_files(struct nlm_file *file)
> > +{
> > +	struct file_lock lock;
> > +	struct file *f;
> > +
> > +	lock.fl_type  = F_UNLCK;
> > +	lock.fl_start = 0;
> > +	lock.fl_end   = OFFSET_MAX;
> > +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
> 
> O_RDONLY and O_WRONLY ?

I thought they looked weird as loop boundaries.

> > @@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
> > 
> > static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
> > {
> > -	return locks_inode(file->f_file);
> > +	return locks_inode(file->f_file[0] ?
> > +				file->f_file[0] : file->f_file[1]);
> 
> O_RDONLY and O_WRONLY ?

A little less weird.

OK, I admit, "looks weird" isn't much of an argument.

I say we leave it to whoever cares enough to make the change.

--b.

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] Keep read and write fds with each nlm_file
  2021-08-23 21:58             ` Bruce Fields
@ 2021-08-23 22:00               ` Chuck Lever III
  0 siblings, 0 replies; 28+ messages in thread
From: Chuck Lever III @ 2021-08-23 22:00 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Bruce Fields, Trond Myklebust, Anna Schumaker, daire,
	Linux NFS Mailing List



> On Aug 23, 2021, at 5:58 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Mon, Aug 23, 2021 at 09:54:20PM +0000, Chuck Lever III wrote:
>>> On Aug 23, 2021, at 4:44 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>> +static int nlm_unlock_files(struct nlm_file *file)
>>> +{
>>> +	struct file_lock lock;
>>> +	struct file *f;
>>> +
>>> +	lock.fl_type  = F_UNLCK;
>>> +	lock.fl_start = 0;
>>> +	lock.fl_end   = OFFSET_MAX;
>>> +	for (f = file->f_file[0]; f <= file->f_file[1]; f++) {
>> 
>> O_RDONLY and O_WRONLY ?
> 
> I thought they looked weird as loop boundaries.
> 
>>> @@ -301,7 +305,8 @@ int           nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr);
>>> 
>>> static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
>>> {
>>> -	return locks_inode(file->f_file);
>>> +	return locks_inode(file->f_file[0] ?
>>> +				file->f_file[0] : file->f_file[1]);
>> 
>> O_RDONLY and O_WRONLY ?
> 
> A little less weird.
> 
> OK, I admit, "looks weird" isn't much of an argument.
> 
> I say we leave it to whoever cares enough to make the change.

I can update nlmsvc_file_inode() when I apply this version
of the patch, and leave the loops alone.


--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 0/8] reexport lock fixes v3
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (7 preceding siblings ...)
  2021-08-20 21:02 ` [PATCH 8/8] nfs: don't allow reexport reclaims J. Bruce Fields
@ 2021-08-25  2:35 ` J. Bruce Fields
  2021-08-25  2:36   ` [PATCH 9/8] nfsd: fix crash on LOCKT on reexported NFSv3 J. Bruce Fields
  2021-08-26 19:05 ` [PATCH 0/8] reexport lock fixes v3 Anna Schumaker
  9 siblings, 1 reply; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-25  2:35 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Chuck Lever, Trond Myklebust, Anna Schumaker, daire, linux-nfs

On Fri, Aug 20, 2021 at 05:01:58PM -0400, J. Bruce Fields wrote:
> With those changes I'm passing connecthon tests for NFSv3-4.2 reexports
> of an NFSv4.0 filesystem.

But I hadn't tested reexports of an NFSv3 filesystem.  With the
following server-side patch I also pass connectathon on NFSv3-4.2
reexports of an NFSv3 filesystem.--b.

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 9/8] nfsd: fix crash on LOCKT on reexported NFSv3
  2021-08-25  2:35 ` [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
@ 2021-08-25  2:36   ` J. Bruce Fields
  0 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-25  2:36 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Chuck Lever, Trond Myklebust, Anna Schumaker, daire, linux-nfs

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

Unlike other filesystems, NFSv3 tries to use fl_file in the GETLK case.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1b6a7f48982e..4b6d60b46b0a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7043,8 +7043,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 /*
  * The NFSv4 spec allows a client to do a LOCKT without holding an OPEN,
  * so we do a temporary open here just to get an open file to pass to
- * vfs_test_lock.  (Arguably perhaps test_lock should be done with an
- * inode operation.)
+ * vfs_test_lock.
  */
 static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
 {
@@ -7059,7 +7058,9 @@ static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct
 							NFSD_MAY_READ));
 	if (err)
 		goto out;
+	lock->fl_file = nf->nf_file;
 	err = nfserrno(vfs_test_lock(nf->nf_file, lock));
+	lock->fl_file = NULL;
 out:
 	fh_unlock(fhp);
 	nfsd_file_put(nf);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 0/8] reexport lock fixes v3
  2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
                   ` (8 preceding siblings ...)
  2021-08-25  2:35 ` [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
@ 2021-08-26 19:05 ` Anna Schumaker
  2021-08-26 19:38   ` Chuck Lever III
  9 siblings, 1 reply; 28+ messages in thread
From: Anna Schumaker @ 2021-08-26 19:05 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Chuck Lever, Trond Myklebust, daire, Linux NFS Mailing List

On Fri, Aug 20, 2021 at 5:02 PM J. Bruce Fields <bfields@redhat.com> wrote:
>
> 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.
>
> Note:
>         - patches 1-5 are server side
>         - patches 6-7 are client side
>         - patch 8 affects both
>
> Simplest might be for Trond or Anna to ACK 6-8, if they look OK, and

They look okay to me. You can add:
        Acked-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
to all three.

Anna

> then submit them all through the server.  But those three sets of
> patches are all independent if you'd rather split them up.
>
> 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.
>
> Change since v2:
>         - keep nlmsvc_file_inode a static inline to address build
>           failure identified by the kernel test robot
> Changes since v1:
>         - Use ENOGRACE instead of returning NFS-specific error from vfs lock
>           method.
>         - Take write opens for write locks in the NLM case (as we always
>           have in the NFSv4 case).
>         - Don't block NLM threads waiting for blocking locks.
>
> With those changes I'm passing connecthon tests for NFSv3-4.2 reexports
> of an NFSv4.0 filesystem.
>
> --b.
>
> J. Bruce Fields (8):
>   lockd: lockd server-side shouldn't set fl_ops
>   nlm: minor nlm_lookup_file argument change
>   nlm: minor refactoring
>   lockd: update nlm_lookup_file reexport comment
>   Keep read and write fds with each nlm_file
>   nfs: don't atempt blocking locks on nfs reexports
>   lockd: don't attempt blocking locks on nfs reexports
>   nfs: don't allow reexport reclaims
>
>  fs/lockd/svc4proc.c         |   6 +-
>  fs/lockd/svclock.c          |  80 ++++++++++++++----------
>  fs/lockd/svcproc.c          |   6 +-
>  fs/lockd/svcsubs.c          | 117 +++++++++++++++++++++++++-----------
>  fs/nfs/export.c             |   2 +-
>  fs/nfs/file.c               |   3 +
>  fs/nfsd/lockd.c             |   8 ++-
>  fs/nfsd/nfs4state.c         |  11 +++-
>  fs/nfsd/nfsproc.c           |   1 +
>  include/linux/errno.h       |   1 +
>  include/linux/exportfs.h    |   2 +
>  include/linux/fs.h          |   1 +
>  include/linux/lockd/bind.h  |   3 +-
>  include/linux/lockd/lockd.h |  11 +++-
>  14 files changed, 170 insertions(+), 82 deletions(-)
>
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 0/8] reexport lock fixes v3
  2021-08-26 19:05 ` [PATCH 0/8] reexport lock fixes v3 Anna Schumaker
@ 2021-08-26 19:38   ` Chuck Lever III
  0 siblings, 0 replies; 28+ messages in thread
From: Chuck Lever III @ 2021-08-26 19:38 UTC (permalink / raw)
  To: Anna Schumaker, Bruce Fields
  Cc: Trond Myklebust, Daire Byrne, Linux NFS Mailing List



> On Aug 26, 2021, at 3:05 PM, Anna Schumaker <schumakeranna@gmail.com> wrote:
> 
> On Fri, Aug 20, 2021 at 5:02 PM J. Bruce Fields <bfields@redhat.com> wrote:
>> 
>> 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.
>> 
>> Note:
>>        - patches 1-5 are server side
>>        - patches 6-7 are client side
>>        - patch 8 affects both
>> 
>> Simplest might be for Trond or Anna to ACK 6-8, if they look OK, and
> 
> They look okay to me. You can add:
>        Acked-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> to all three.
> 
> Anna

Thanks. I've captured Anna's Acks and included 9/8 (posted a few
days ago). See:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next



>> then submit them all through the server.  But those three sets of
>> patches are all independent if you'd rather split them up.
>> 
>> 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.
>> 
>> Change since v2:
>>        - keep nlmsvc_file_inode a static inline to address build
>>          failure identified by the kernel test robot
>> Changes since v1:
>>        - Use ENOGRACE instead of returning NFS-specific error from vfs lock
>>          method.
>>        - Take write opens for write locks in the NLM case (as we always
>>          have in the NFSv4 case).
>>        - Don't block NLM threads waiting for blocking locks.
>> 
>> With those changes I'm passing connecthon tests for NFSv3-4.2 reexports
>> of an NFSv4.0 filesystem.
>> 
>> --b.
>> 
>> J. Bruce Fields (8):
>>  lockd: lockd server-side shouldn't set fl_ops
>>  nlm: minor nlm_lookup_file argument change
>>  nlm: minor refactoring
>>  lockd: update nlm_lookup_file reexport comment
>>  Keep read and write fds with each nlm_file
>>  nfs: don't atempt blocking locks on nfs reexports
>>  lockd: don't attempt blocking locks on nfs reexports
>>  nfs: don't allow reexport reclaims
>> 
>> fs/lockd/svc4proc.c         |   6 +-
>> fs/lockd/svclock.c          |  80 ++++++++++++++----------
>> fs/lockd/svcproc.c          |   6 +-
>> fs/lockd/svcsubs.c          | 117 +++++++++++++++++++++++++-----------
>> fs/nfs/export.c             |   2 +-
>> fs/nfs/file.c               |   3 +
>> fs/nfsd/lockd.c             |   8 ++-
>> fs/nfsd/nfs4state.c         |  11 +++-
>> fs/nfsd/nfsproc.c           |   1 +
>> include/linux/errno.h       |   1 +
>> include/linux/exportfs.h    |   2 +
>> include/linux/fs.h          |   1 +
>> include/linux/lockd/bind.h  |   3 +-
>> include/linux/lockd/lockd.h |  11 +++-
>> 14 files changed, 170 insertions(+), 82 deletions(-)
>> 
>> --
>> 2.31.1
>> 

--
Chuck Lever




^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 4/8] lockd: update nlm_lookup_file reexport comment
  2021-08-16 13:59 [PATCH 0/8] reexport lock fixes v2 J. Bruce Fields
@ 2021-08-16 13:59 ` J. Bruce Fields
  0 siblings, 0 replies; 28+ messages in thread
From: J. Bruce Fields @ 2021-08-16 13:59 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker; +Cc: daire, linux-nfs, J. Bruce Fields

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

Update comment to reflect that we *do* allow reexport, whether it's a
good idea or not....

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svcsubs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 2558598610a9..f43d89e89c45 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -111,8 +111,9 @@ nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 	INIT_HLIST_NODE(&file->f_list);
 	INIT_LIST_HEAD(&file->f_blocks);
 
-	/* Open the file. Note that this must not sleep for too long, else
-	 * we would lock up lockd:-) So no NFS re-exports, folks.
+	/*
+	 * Open the file. Note that if we're reexporting, for example,
+	 * this could block the lockd thread for a while.
 	 *
 	 * We have to make sure we have the right credential to open
 	 * the file.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-08-26 19:38 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 21:01 [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
2021-08-20 21:01 ` [PATCH 1/8] lockd: lockd server-side shouldn't set fl_ops J. Bruce Fields
2021-08-20 21:02 ` [PATCH 2/8] nlm: minor nlm_lookup_file argument change J. Bruce Fields
2021-08-21 16:30   ` Chuck Lever III
2021-08-23 16:01     ` J. Bruce Fields
2021-08-23 17:02       ` Chuck Lever III
2021-08-23 17:21         ` Bruce Fields
2021-08-20 21:02 ` [PATCH 3/8] nlm: minor refactoring J. Bruce Fields
2021-08-21 16:30   ` Chuck Lever III
2021-08-23 15:26     ` J. Bruce Fields
2021-08-20 21:02 ` [PATCH 4/8] lockd: update nlm_lookup_file reexport comment J. Bruce Fields
2021-08-20 21:02 ` [PATCH 5/8] Keep read and write fds with each nlm_file J. Bruce Fields
2021-08-21 16:30   ` Chuck Lever III
2021-08-23 16:08     ` J. Bruce Fields
2021-08-23 18:57     ` J. Bruce Fields
2021-08-23 18:59       ` Chuck Lever III
2021-08-23 20:44         ` Bruce Fields
2021-08-23 21:54           ` Chuck Lever III
2021-08-23 21:58             ` Bruce Fields
2021-08-23 22:00               ` Chuck Lever III
2021-08-20 21:02 ` [PATCH 6/8] nfs: don't atempt blocking locks on nfs reexports J. Bruce Fields
2021-08-20 21:02 ` [PATCH 7/8] lockd: don't attempt " J. Bruce Fields
2021-08-20 21:02 ` [PATCH 8/8] nfs: don't allow reexport reclaims J. Bruce Fields
2021-08-25  2:35 ` [PATCH 0/8] reexport lock fixes v3 J. Bruce Fields
2021-08-25  2:36   ` [PATCH 9/8] nfsd: fix crash on LOCKT on reexported NFSv3 J. Bruce Fields
2021-08-26 19:05 ` [PATCH 0/8] reexport lock fixes v3 Anna Schumaker
2021-08-26 19:38   ` Chuck Lever III
  -- strict thread matches above, loose matches on Subject: below --
2021-08-16 13:59 [PATCH 0/8] reexport lock fixes v2 J. Bruce Fields
2021-08-16 13:59 ` [PATCH 4/8] lockd: update nlm_lookup_file reexport comment J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.