locking fixups for NFS

flock64_to_posix_lock() is already doing this check

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/nfs4proc.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

fs/nfs/nfs4proc.c +0 -0

index 8981803..2231d7d 100644

We only need to check lock exclusive/shared types against open mode when flock() is used on NFS, so move it into the flock-specific path instead of checking it for all locks.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c     |   16 ++++++++++++++++
 fs/nfs/nfs4proc.c |   13 -------------
 2 files changed, 16 insertions(+), 13 deletions(-)

fs/nfs/file.c +0 -0

index 93e2364..1e4804b 100644

fs/nfs/file.c +15 -0

+	/*
+	 * VFS doesn't require the open mode to match a flock() lock's type.
+	 * NFS, however, may simulate flock() locking with posix locking which
+	 * requires the open mode to match the lock type.
+	 */
+	switch (fl->fl_type) {
+	case F_RDLCK:
+		if (!(filp->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+
 	return do_setlk(filp, cmd, fl, is_local);
 }
 EXPORT_SYMBOL_GPL(nfs_flock);

fs/nfs/nfs4proc.c +0 -6

index 2231d7d..05ea1e1 100644
@@ -6125,19 +6125,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 
 	if (state == NULL)
 		return -ENOLCK;
-	/*
-	 * Don't rely on the VFS having checked the file open mode,
-	 * since it won't do this for flock() locks.
-	 */
-	switch (request->fl_type) {
-	case F_RDLCK:

We only use the file struct pointer to obtain the nfs_open_context (or the inode for v2/v3), so just pass that instead. This allows us to use the lock procedure without a reference to a struct file which may not be available while releasing locks after a file close.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c           |    9 ++++++---
 fs/nfs/nfs3proc.c       |    4 ++--
 fs/nfs/nfs4proc.c       |    4 +---
 fs/nfs/proc.c           |    4 ++--
 include/linux/nfs_xdr.h |    2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

fs/nfs/file.c +6 -3

index 1e4804b..5dec0fb 100644
@@ -711,6 +711,7 @@ static int
 do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status = 0;
 	unsigned int saved_type = fl->fl_type;
 

@@ -728,7 +729,7 @@ do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (is_local)
 		goto out_noconflict;
 
-	status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 out:
 	return status;
 out_noconflict:

@@ -745,6 +746,7 @@ static int
 do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	struct nfs_lock_context *l_ctx;
 	int status;
 

@@ -771,7 +773,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	return status;

@@ -786,6 +788,7 @@ static int
 do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status;
 
 	/*

@@ -801,7 +804,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	if (status < 0)

fs/nfs/nfs3proc.c +2 -2

index cb28cce..a10e147 100644
@@ -866,9 +866,9 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
 }
 
 static int
-nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

fs/nfs/nfs4proc.c +1 -3

index 05ea1e1..784ba4e 100644
@@ -6097,15 +6097,13 @@ static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *
 }
 
 static int
-nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
+nfs4_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *request)
 {
-	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
 	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
 	int status;
 
 	/* verify open state */
-	ctx = nfs_file_open_context(filp);
 	state = ctx->state;
 
 	if (IS_GETLK(cmd)) {

fs/nfs/proc.c +2 -2

index b417bbc..185deb9 100644
@@ -634,9 +634,9 @@ nfs_proc_commit_setup(struct nfs_commit_data *data, struct rpc_message *msg)
 }
 
 static int
-nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

include/linux/nfs_xdr.h +1 -1

index 11bbae4..58c4682 100644
@@ -1555,7 +1555,7 @@ struct nfs_rpc_ops {
 	void	(*commit_setup) (struct nfs_commit_data *, struct rpc_message *);
 	void	(*commit_rpc_prepare)(struct rpc_task *, struct nfs_commit_data *);
 	int	(*commit_done) (struct rpc_task *, struct nfs_commit_data *);
-	int	(*lock)(struct file *, int, struct file_lock *);
+	int	(*lock)(struct nfs_open_context *, int, struct file_lock *);
 	int	(*lock_check_bounds)(const struct file_lock *);
 	void	(*clear_acl_cache)(struct inode *);
 	void	(*close_context)(struct nfs_open_context *ctx, int);

Quoted 6 lines · On Mon, 28 Dec 2015 11:27:59 -0500 Benjamin Coddington <[email protected]>…

We only use the file struct pointer to obtain the nfs_open_context (or the inode for v2/v3), so just pass that instead. This allows us to use the lock procedure without a reference to a struct file which may not be available while releasing locks after a file close.

Signed-off-by Benjamin Coddington <[email protected]>

Hmm...

So what pins the nfs_open_context in place after the filp is gone?

AFAICT, that gets zapped when the struct file goes away. Is there any chance that the ctx pointers you're passing around here might outlive the structure to which they point?

Quoted 121 lines

---
 fs/nfs/file.c           |    9 ++++++---
 fs/nfs/nfs3proc.c       |    4 ++--
 fs/nfs/nfs4proc.c       |    4 +---
 fs/nfs/proc.c           |    4 ++--
 include/linux/nfs_xdr.h |    2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

fs/nfs/file.c +6 -3

index 1e4804b..5dec0fb 100644
@@ -711,6 +711,7 @@ static int
 do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status = 0;
 	unsigned int saved_type = fl->fl_type;
 

@@ -728,7 +729,7 @@ do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (is_local)
 		goto out_noconflict;
 
-	status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 out:
 	return status;
 out_noconflict:

@@ -745,6 +746,7 @@ static int
 do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	struct nfs_lock_context *l_ctx;
 	int status;
 

@@ -771,7 +773,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	return status;

@@ -786,6 +788,7 @@ static int
 do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status;
 
 	/*

@@ -801,7 +804,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	if (status < 0)

fs/nfs/nfs3proc.c +2 -2

index cb28cce..a10e147 100644
@@ -866,9 +866,9 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
 }
 
 static int
-nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

fs/nfs/nfs4proc.c +1 -3

index 05ea1e1..784ba4e 100644
@@ -6097,15 +6097,13 @@ static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *
 }
 
 static int
-nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
+nfs4_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *request)
 {
-	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
 	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
 	int status;
 
 	/* verify open state */
-	ctx = nfs_file_open_context(filp);
 	state = ctx->state;
 
 	if (IS_GETLK(cmd)) {

fs/nfs/proc.c +2 -2

index b417bbc..185deb9 100644
@@ -634,9 +634,9 @@ nfs_proc_commit_setup(struct nfs_commit_data *data, struct rpc_message *msg)
 }
 
 static int
-nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

include/linux/nfs_xdr.h +0 -0

index 11bbae4..58c4682 100644

include/linux/nfs_xdr.h struct nfs_rpc_ops { · line 1561

Quoted 8 lines

include/linux/nfs_xdr.h +1 -1

@@ -1555,7 +1555,7 @@ struct nfs_rpc_ops {
 	void	(*commit_setup) (struct nfs_commit_data *, struct rpc_message *);
 	void	(*commit_rpc_prepare)(struct rpc_task *, struct nfs_commit_data *);
 	int	(*commit_done) (struct rpc_task *, struct nfs_commit_data *);
-	int	(*lock)(struct file *, int, struct file_lock *);
+	int	(*lock)(struct nfs_open_context *, int, struct file_lock *);
 	int	(*lock_check_bounds)(const struct file_lock *);
 	void	(*clear_acl_cache)(struct inode *);
 	void	(*close_context)(struct nfs_open_context *ctx, int);

--
Jeff Layton <[email protected]>

Quoted 10 lines · On Mon, 28 Dec 2015 11:27:58 -0500 Benjamin Coddington <[email protected]>…

We only need to check lock exclusive/shared types against open mode when flock() is used on NFS, so move it into the flock-specific path instead of checking it for all locks.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c     |   16 ++++++++++++++++
 fs/nfs/nfs4proc.c |   13 -------------
 2 files changed, 16 insertions(+), 13 deletions(-)

fs/nfs/file.c int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) · line 896

Quoted 9 lines

fs/nfs/file.c +1 -0

index 93e2364..1e4804b 100644
@@ -893,6 +893,22 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 	/* We're simulating flock() locks using posix locks on the server */
 	if (fl->fl_type == F_UNLCK)
 		return do_unlk(filp, cmd, fl, is_local);
+

nit: maybe make the above and below part of the same switch?

Quoted 32 lines

fs/nfs/file.c +15 -0

+	/*
+	 * VFS doesn't require the open mode to match a flock() lock's type.
+	 * NFS, however, may simulate flock() locking with posix locking which
+	 * requires the open mode to match the lock type.
+	 */
+	switch (fl->fl_type) {
+	case F_RDLCK:
+		if (!(filp->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+
 	return do_setlk(filp, cmd, fl, is_local);
 }
 EXPORT_SYMBOL_GPL(nfs_flock);

fs/nfs/nfs4proc.c +0 -6

index 2231d7d..05ea1e1 100644
@@ -6125,19 +6125,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 
 	if (state == NULL)
 		return -ENOLCK;
-	/*
-	 * Don't rely on the VFS having checked the file open mode,
-	 * since it won't do this for flock() locks.
-	 */
-	switch (request->fl_type) {
-	case F_RDLCK:

fs/nfs/nfs4proc.c nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request) · line 6130

Quoted 10 lines

fs/nfs/nfs4proc.c +0 -7

-		if (!(filp->f_mode & FMODE_READ))
-			return -EBADF;
-		break;
-	case F_WRLCK:
-		if (!(filp->f_mode & FMODE_WRITE))
-			return -EBADF;
-	}
 
 	do {
 		status = nfs4_proc_setlk(state, cmd, request);

Looks correct other than the nit above:

Reviewed-by Jeff Layton <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Jeff Layton

Quoted 6 lines · On Mon, 28 Dec 2015 11:27:59 -0500 Benjamin Coddington <[email protected]>…

We only use the file struct pointer to obtain the nfs_open_context (or the inode for v2/v3), so just pass that instead. This allows us to use the lock procedure without a reference to a struct file which may not be available while releasing locks after a file close.

Signed-off-by Benjamin Coddington <[email protected]>

Quoted 7 lines

Hmm...

So what pins the nfs_open_context in place after the filp is gone?

AFAICT, that gets zapped when the struct file goes away. Is there any chance that the ctx pointers you're passing around here might outlive the structure to which they point?

Patch 9 sets up the only time we'll want to perform an unlock without a filp, and that is when that unlock has been deferred due to outstanding IO. When the IO completes, nfs_iocounter_dec() is called while holding a reference to a nfs_lock_context containing a reference to the nfs_open_context, and that final unlock is done while holding that reference.

Ben

Quoted 134 lines

Quoted 121 lines

---
 fs/nfs/file.c           |    9 ++++++---
 fs/nfs/nfs3proc.c       |    4 ++--
 fs/nfs/nfs4proc.c       |    4 +---
 fs/nfs/proc.c           |    4 ++--
 include/linux/nfs_xdr.h |    2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

fs/nfs/file.c +6 -3

index 1e4804b..5dec0fb 100644
@@ -711,6 +711,7 @@ static int
 do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status = 0;
 	unsigned int saved_type = fl->fl_type;
 

@@ -728,7 +729,7 @@ do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (is_local)
 		goto out_noconflict;
 
-	status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 out:
 	return status;
 out_noconflict:

@@ -745,6 +746,7 @@ static int
 do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	struct nfs_lock_context *l_ctx;
 	int status;
 

@@ -771,7 +773,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	return status;

@@ -786,6 +788,7 @@ static int
 do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 {
 	struct inode *inode = filp->f_mapping->host;
+	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 	int status;
 
 	/*

@@ -801,7 +804,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	 * "-olocal_lock="
 	 */
 	if (!is_local)
-		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+		status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
 	else
 		status = do_vfs_lock(filp, fl);
 	if (status < 0)

fs/nfs/nfs3proc.c +2 -2

index cb28cce..a10e147 100644
@@ -866,9 +866,9 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
 }
 
 static int
-nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

fs/nfs/nfs4proc.c +1 -3

index 05ea1e1..784ba4e 100644
@@ -6097,15 +6097,13 @@ static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *
 }
 
 static int
-nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
+nfs4_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *request)
 {
-	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
 	unsigned long timeout = NFS4_LOCK_MINTIMEOUT;
 	int status;
 
 	/* verify open state */
-	ctx = nfs_file_open_context(filp);
 	state = ctx->state;
 
 	if (IS_GETLK(cmd)) {

fs/nfs/proc.c +2 -2

index b417bbc..185deb9 100644
@@ -634,9 +634,9 @@ nfs_proc_commit_setup(struct nfs_commit_data *data, struct rpc_message *msg)
 }
 
 static int
-nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl)
+nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
-	struct inode *inode = file_inode(filp);
+	struct inode *inode = d_inode(ctx->dentry);
 
 	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
 }

include/linux/nfs_xdr.h +0 -0

index 11bbae4..58c4682 100644

include/linux/nfs_xdr.h struct nfs_rpc_ops { · line 1561

Quoted 8 lines

include/linux/nfs_xdr.h +1 -1

@@ -1555,7 +1555,7 @@ struct nfs_rpc_ops {
 	void	(*commit_setup) (struct nfs_commit_data *, struct rpc_message *);
 	void	(*commit_rpc_prepare)(struct rpc_task *, struct nfs_commit_data *);
 	int	(*commit_done) (struct rpc_task *, struct nfs_commit_data *);
-	int	(*lock)(struct file *, int, struct file_lock *);
+	int	(*lock)(struct nfs_open_context *, int, struct file_lock *);
 	int	(*lock_check_bounds)(const struct file_lock *);
 	void	(*clear_acl_cache)(struct inode *);
 	void	(*close_context)(struct nfs_open_context *ctx, int);

--
Jeff Layton <[email protected]>

nfs4_proc_unlk() acquires the nfs_open_context from the file_lock's fl_file->private, but we will not always have a referenced fl_file in this path. Instead, pass along the nfs_open_context from the call to nfs4_proc_lock(). This allows us to use nfs4_proc_unlck() without a valid reference in fl_file.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/nfs4proc.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

fs/nfs/nfs4proc.c +4 -2

index 784ba4e..279c8b3 100644
@@ -5656,8 +5656,10 @@ static struct rpc_task *nfs4_do_unlck(struct file_lock *fl,
 	return rpc_run_task(&task_setup_data);
 }
 
-static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *request)
+static int nfs4_proc_unlck(struct nfs_open_context *ctx,
+		int cmd, struct file_lock *request)
 {
+	struct nfs4_state *state = ctx->state;
 	struct inode *inode = state->inode;
 	struct nfs4_state_owner *sp = state->owner;
 	struct nfs_inode *nfsi = NFS_I(inode);

@@ -5693,7 +5695,7 @@ static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *
 	status = -ENOMEM;
 	if (IS_ERR(seqid))
 		goto out;
-	task = nfs4_do_unlck(request, nfs_file_open_context(request->fl_file), lsp, seqid);
+	task = nfs4_do_unlck(request, ctx, lsp, seqid);
 	status = PTR_ERR(task);
 	if (IS_ERR(task))
 		goto out;

We cannot depend on a file_lock->fl_file still being around while performing unlocks for NFSv2 and NFSv3. Pass along the nfs_open_context to provide the necessary inode and credential to complete the unlock.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/lockd/clntproc.c        |   16 ++++++++++------
 fs/nfs/nfs3proc.c          |    4 +---
 fs/nfs/proc.c              |    4 +---
 include/linux/lockd/bind.h |    3 ++-
 4 files changed, 14 insertions(+), 13 deletions(-)

fs/lockd/clntproc.c +0 -0

index 1129520..72b65c4 100644

fs/lockd/clntproc.c +7 -4

 static int	nlm_stat_to_errno(__be32 stat);
 static void	nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host);
 static int	nlmclnt_cancel(struct nlm_host *, int , struct file_lock *);

@@ -147,13 +148,15 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
 
 /**
  * nlmclnt_proc - Perform a single client-side lock request
- * @host: address of a valid nlm_host context representing the NLM server
+ * @ctx: address of the nfs_open_context for lock to operate upon
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
+	struct inode		*inode = d_inode(ctx->dentry);
+	struct nlm_host		*host = NFS_SERVER(inode)->nlm_host;
 	struct nlm_rqst		*call;
 	int			status;
 

@@ -175,7 +178,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 			call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
 			status = nlmclnt_lock(call, fl);
 		} else
-			status = nlmclnt_unlock(call, fl);
+			status = nlmclnt_unlock(ctx, call, fl);
 	} else if (IS_GETLK(cmd))
 		status = nlmclnt_test(call, fl);
 	else

@@ -646,7 +649,8 @@ nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
  * UNLOCK: remove an existing lock
  */
 static int
-nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
+nlmclnt_unlock(struct nfs_open_context *ctx,
+		struct nlm_rqst *req, struct file_lock *fl)
 {
 	struct nlm_host	*host = req->a_host;
 	struct nlm_res	*resp = &req->a_res;

fs/lockd/clntproc.c +0 -0

 	if (status < 0)
 		goto out;

fs/nfs/nfs3proc.c +0 -0

index a10e147..2941ab7 100644
@@ -868,9 +868,7 @@ static void nfs3_proc_commit_setup(struct

nfs_commit_data *data, struct rpc_mess static int
nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct
file_lock *fl) {

fs/nfs/nfs3proc.c +1 -3

-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)

fs/nfs/proc.c +1 -3

index 185deb9..17d892e 100644
@@ -636,9 +636,7 @@ nfs_proc_commit_setup(struct nfs_commit_data
 *data, struct rpc_message *msg) static int
 nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct
 file_lock *fl) {
-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 /* Helper functions for NFS lock bounds checking */

include/linux/lockd/bind.h +2 -1

index 4d24d64..1d62e85 100644
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct nfs_open_context;
 
 /*
  * This is the set of functions for lockd->nfsd communication

@@ -52,7 +53,7 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata
 *nlm_init); extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
+extern int	nlmclnt_proc(struct nfs_open_context *ctx, int cmd,
 					struct file_lock *fl);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);

Quoted 18 lines · On Mon, 28 Dec 2015 11:28:01 -0500 Benjamin Coddington <[email protected]>…

We cannot depend on a file_lock->fl_file still being around while performing unlocks for NFSv2 and NFSv3. Pass along the nfs_open_context to provide the necessary inode and credential to complete the unlock.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/lockd/clntproc.c        |   16 ++++++++++------
 fs/nfs/nfs3proc.c          |    4 +---
 fs/nfs/proc.c              |    4 +---
 include/linux/lockd/bind.h |    3 ++-
 4 files changed, 14 insertions(+), 13 deletions(-)

fs/lockd/clntproc.c +0 -0

index 1129520..72b65c4 100644
@@ -25,7 +25,8 @@
 

fs/lockd/clntproc.c line 29

Quoted 5 lines

fs/lockd/clntproc.c +2 -1

 static int	nlmclnt_test(struct nlm_rqst *, struct file_lock *);
 static int	nlmclnt_lock(struct nlm_rqst *, struct file_lock *);
-static int	nlmclnt_unlock(struct nlm_rqst *, struct file_lock *);
+static int	nlmclnt_unlock(struct nfs_open_context *nfs,
+		struct nlm_rqst *, struct file_lock *);

This smells a bit like a layering violation. The NLM APIs really shouldn't require stuff that's NFS specific.

Quoted 43 lines

fs/lockd/clntproc.c +7 -4

 static int	nlm_stat_to_errno(__be32 stat);
 static void	nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host);
 static int	nlmclnt_cancel(struct nlm_host *, int , struct file_lock *);

@@ -147,13 +148,15 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
 
 /**
  * nlmclnt_proc - Perform a single client-side lock request
- * @host: address of a valid nlm_host context representing the NLM server
+ * @ctx: address of the nfs_open_context for lock to operate upon
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
+	struct inode		*inode = d_inode(ctx->dentry);
+	struct nlm_host		*host = NFS_SERVER(inode)->nlm_host;
 	struct nlm_rqst		*call;
 	int			status;
 

@@ -175,7 +178,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 			call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
 			status = nlmclnt_lock(call, fl);
 		} else
-			status = nlmclnt_unlock(call, fl);
+			status = nlmclnt_unlock(ctx, call, fl);
 	} else if (IS_GETLK(cmd))
 		status = nlmclnt_test(call, fl);
 	else

@@ -646,7 +649,8 @@ nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
  * UNLOCK: remove an existing lock
  */
 static int
-nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
+nlmclnt_unlock(struct nfs_open_context *ctx,
+		struct nlm_rqst *req, struct file_lock *fl)
 {
 	struct nlm_host	*host = req->a_host;
 	struct nlm_res	*resp = &req->a_res;

@@ -669,7 +673,7 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
 	}
 

fs/lockd/clntproc.c nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl) · line 677

Quoted 4 lines

fs/lockd/clntproc.c +1 -1

 	atomic_inc(&req->a_count);
-	status = nlmclnt_async_call(nfs_file_cred(fl->fl_file), req,
+	status = nlmclnt_async_call(ctx->cred, req,
 			NLMPROC_UNLOCK, &nlmclnt_unlock_ops);

Since this is the only place you're going to fetch anything out of the ctx, maybe it'd be best to just pass down a rpc_cred pointer instead?

Quoted 45 lines

fs/lockd/clntproc.c +0 -0

 	if (status < 0)
 		goto out;

fs/nfs/nfs3proc.c +0 -0

index a10e147..2941ab7 100644
@@ -868,9 +868,7 @@ static void nfs3_proc_commit_setup(struct

nfs_commit_data *data, struct rpc_mess static int
nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct
file_lock *fl) {

fs/nfs/nfs3proc.c +1 -3

-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)

fs/nfs/proc.c +1 -3

index 185deb9..17d892e 100644
@@ -636,9 +636,7 @@ nfs_proc_commit_setup(struct nfs_commit_data
 *data, struct rpc_message *msg) static int
 nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct
 file_lock *fl) {
-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 /* Helper functions for NFS lock bounds checking */

include/linux/lockd/bind.h +1 -0

index 4d24d64..1d62e85 100644
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct nfs_open_context;
 
 /*
  * This is the set of functions for lockd->nfsd communication

include/linux/lockd/bind.h struct nlmclnt_initdata { · line 59

Quoted 8 lines

include/linux/lockd/bind.h +1 -1

@@ -52,7 +53,7 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata
 *nlm_init); extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
+extern int	nlmclnt_proc(struct nfs_open_context *ctx, int cmd,
 					struct file_lock *fl);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);

--
Jeff Layton <[email protected]>

Quoted 21 lines

Quoted 18 lines · On Mon, 28 Dec 2015 11:28:01 -0500 Benjamin Coddington <[email protected]>…

We cannot depend on a file_lock->fl_file still being around while performing unlocks for NFSv2 and NFSv3. Pass along the nfs_open_context to provide the necessary inode and credential to complete the unlock.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/lockd/clntproc.c        |   16 ++++++++++------
 fs/nfs/nfs3proc.c          |    4 +---
 fs/nfs/proc.c              |    4 +---
 include/linux/lockd/bind.h |    3 ++-
 4 files changed, 14 insertions(+), 13 deletions(-)

fs/lockd/clntproc.c +0 -0

index 1129520..72b65c4 100644
@@ -25,7 +25,8 @@
 

Quoted 8 lines

fs/lockd/clntproc.c line 29

Quoted 5 lines

fs/lockd/clntproc.c +2 -1

 static int	nlmclnt_test(struct nlm_rqst *, struct file_lock *);
 static int	nlmclnt_lock(struct nlm_rqst *, struct file_lock *);
-static int	nlmclnt_unlock(struct nlm_rqst *, struct file_lock *);
+static int	nlmclnt_unlock(struct nfs_open_context *nfs,
+		struct nlm_rqst *, struct file_lock *);

This smells a bit like a layering violation. The NLM APIs really shouldn't require stuff that's NFS specific.

I had second thoughts about this too, but it didn't make sense to keep the separation if we're just going to use nfs_file_cred() in NLM anyway..

Do you think we should find a way to clean that out too?

Quoted 43 lines

Quoted 43 lines

fs/lockd/clntproc.c +7 -4

 static int	nlm_stat_to_errno(__be32 stat);
 static void	nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host);
 static int	nlmclnt_cancel(struct nlm_host *, int , struct file_lock *);

@@ -147,13 +148,15 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
 
 /**
  * nlmclnt_proc - Perform a single client-side lock request
- * @host: address of a valid nlm_host context representing the NLM server
+ * @ctx: address of the nfs_open_context for lock to operate upon
  * @cmd: fcntl-style file lock operation to perform
  * @fl: address of arguments for the lock operation
  *
  */
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
 {
+	struct inode		*inode = d_inode(ctx->dentry);
+	struct nlm_host		*host = NFS_SERVER(inode)->nlm_host;
 	struct nlm_rqst		*call;
 	int			status;
 

@@ -175,7 +178,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
 			call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
 			status = nlmclnt_lock(call, fl);
 		} else
-			status = nlmclnt_unlock(call, fl);
+			status = nlmclnt_unlock(ctx, call, fl);
 	} else if (IS_GETLK(cmd))
 		status = nlmclnt_test(call, fl);
 	else

@@ -646,7 +649,8 @@ nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
  * UNLOCK: remove an existing lock
  */
 static int
-nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
+nlmclnt_unlock(struct nfs_open_context *ctx,
+		struct nlm_rqst *req, struct file_lock *fl)
 {
 	struct nlm_host	*host = req->a_host;
 	struct nlm_res	*resp = &req->a_res;

@@ -669,7 +673,7 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
 	}
 

Quoted 7 lines

fs/lockd/clntproc.c nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl) · line 677

Quoted 4 lines

fs/lockd/clntproc.c +1 -1

 	atomic_inc(&req->a_count);
-	status = nlmclnt_async_call(nfs_file_cred(fl->fl_file), req,
+	status = nlmclnt_async_call(ctx->cred, req,
 			NLMPROC_UNLOCK, &nlmclnt_unlock_ops);

Since this is the only place you're going to fetch anything out of the ctx, maybe it'd be best to just pass down a rpc_cred pointer instead?

Patch 7 changes do_vfs_lock() to take the inode instead of the file, so the ctx is useful again to obtain the inode.

Ben

Quoted 58 lines

Quoted 45 lines

fs/lockd/clntproc.c +0 -0

 	if (status < 0)
 		goto out;

fs/nfs/nfs3proc.c +0 -0

index a10e147..2941ab7 100644
@@ -868,9 +868,7 @@ static void nfs3_proc_commit_setup(struct

nfs_commit_data *data, struct rpc_mess static int
nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct
file_lock *fl) {

fs/nfs/nfs3proc.c +1 -3

-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 static int nfs3_have_delegation(struct inode *inode, fmode_t flags)

fs/nfs/proc.c +1 -3

index 185deb9..17d892e 100644
@@ -636,9 +636,7 @@ nfs_proc_commit_setup(struct nfs_commit_data
 *data, struct rpc_message *msg) static int
 nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct
 file_lock *fl) {
-	struct inode *inode = d_inode(ctx->dentry);
-
-	return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+	return nlmclnt_proc(ctx, cmd, fl);
 }
 
 /* Helper functions for NFS lock bounds checking */

include/linux/lockd/bind.h +1 -0

index 4d24d64..1d62e85 100644
@@ -18,6 +18,7 @@
 
 /* Dummy declarations */
 struct svc_rqst;
+struct nfs_open_context;
 
 /*
  * This is the set of functions for lockd->nfsd communication

include/linux/lockd/bind.h struct nlmclnt_initdata { · line 59

Quoted 8 lines

include/linux/lockd/bind.h +1 -1

@@ -52,7 +53,7 @@ struct nlmclnt_initdata {
 extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata
 *nlm_init); extern void	nlmclnt_done(struct nlm_host *host);
 
-extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
+extern int	nlmclnt_proc(struct nfs_open_context *ctx, int cmd,
 					struct file_lock *fl);
 extern int	lockd_up(struct net *net);
 extern void	lockd_down(struct net *net);

--
Jeff Layton <[email protected]>