[PATCH 0/3] Fixups for l_pid

[PATCH 0/3] Fixups for l_pid

[Benjamin Coddington]

LTP fcntl tests (fcntl11 fcntl14 fcntl17 fcntl19 fcntl20 fcntl21) have been
failing for NFSv4 mounts due to an unexpected l_pid.  What follows are some
fixups:

Benjamin Coddington (3):
  fs/locks: Alloc file_lock where practical
  fs/locks: Set fl_nspid at file_lock allocation
  fs/locks: Use fs-specific l_pid for remote locks

 fs/locks.c         | 137 +++++++++++++++++++++++++++++++++--------------------
 include/linux/fs.h |   1 +
 2 files changed, 86 insertions(+), 52 deletions(-)

-- 
2.9.3

[PATCH 1/3] fs/locks: Alloc file_lock where practical

[Benjamin Coddington]

Use an allocation it where makes sense to do so.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/locks.c | 83 ++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index af2031a1fcff..54aeacf8dc46 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1293,36 +1293,39 @@ int locks_mandatory_locked(struct file *file)
 int locks_mandatory_area(struct inode *inode, struct file *filp, loff_t start,
 			 loff_t end, unsigned char type)
 {
-	struct file_lock fl;
+	struct file_lock *fl;
 	int error;
 	bool sleep = false;
 
-	locks_init_lock(&fl);
-	fl.fl_pid = current->tgid;
-	fl.fl_file = filp;
-	fl.fl_flags = FL_POSIX | FL_ACCESS;
+	fl = locks_alloc_lock();
+	if (fl == NULL)
+		return -ENOMEM;
+
+	fl->fl_pid = current->tgid;
+	fl->fl_file = filp;
+	fl->fl_flags = FL_POSIX | FL_ACCESS;
 	if (filp && !(filp->f_flags & O_NONBLOCK))
 		sleep = true;
-	fl.fl_type = type;
-	fl.fl_start = start;
-	fl.fl_end = end;
+	fl->fl_type = type;
+	fl->fl_start = start;
+	fl->fl_end = end;
 
 	for (;;) {
 		if (filp) {
-			fl.fl_owner = filp;
-			fl.fl_flags &= ~FL_SLEEP;
-			error = posix_lock_inode(inode, &fl, NULL);
+			fl->fl_owner = filp;
+			fl->fl_flags &= ~FL_SLEEP;
+			error = posix_lock_inode(inode, fl, NULL);
 			if (!error)
 				break;
 		}
 
 		if (sleep)
-			fl.fl_flags |= FL_SLEEP;
-		fl.fl_owner = current->files;
-		error = posix_lock_inode(inode, &fl, NULL);
+			fl->fl_flags |= FL_SLEEP;
+		fl->fl_owner = current->files;
+		error = posix_lock_inode(inode, fl, NULL);
 		if (error != FILE_LOCK_DEFERRED)
 			break;
-		error = wait_event_interruptible(fl.fl_wait, !fl.fl_next);
+		error = wait_event_interruptible(fl->fl_wait, !fl->fl_next);
 		if (!error) {
 			/*
 			 * If we've been sleeping someone might have
@@ -1332,10 +1335,10 @@ int locks_mandatory_area(struct inode *inode, struct file *filp, loff_t start,
 				continue;
 		}
 
-		locks_delete_block(&fl);
+		locks_delete_block(fl);
 		break;
 	}
-
+	locks_free_lock(fl);
 	return error;
 }
 
@@ -2088,10 +2091,13 @@ static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl)
  */
 int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
 {
-	struct file_lock file_lock;
+	struct file_lock *fl;
 	struct flock flock;
 	int error;
 
+	fl = locks_alloc_lock();
+	if (fl == NULL)
+		return -ENOMEM;
 	error = -EFAULT;
 	if (copy_from_user(&flock, l, sizeof(flock)))
 		goto out;
@@ -2099,7 +2105,7 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
 	if ((flock.l_type != F_RDLCK) && (flock.l_type != F_WRLCK))
 		goto out;
 
-	error = flock_to_posix_lock(filp, &file_lock, &flock);
+	error = flock_to_posix_lock(filp, fl, &flock);
 	if (error)
 		goto out;
 
@@ -2109,26 +2115,25 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
 			goto out;
 
 		cmd = F_GETLK;
-		file_lock.fl_flags |= FL_OFDLCK;
-		file_lock.fl_owner = filp;
+		fl->fl_flags |= FL_OFDLCK;
+		fl->fl_owner = filp;
 	}
 
-	error = vfs_test_lock(filp, &file_lock);
+	error = vfs_test_lock(filp, fl);
 	if (error)
 		goto out;
  
-	flock.l_type = file_lock.fl_type;
-	if (file_lock.fl_type != F_UNLCK) {
-		error = posix_lock_to_flock(&flock, &file_lock);
+	flock.l_type = fl->fl_type;
+	if (fl->fl_type != F_UNLCK) {
+		error = posix_lock_to_flock(&flock, fl);
 		if (error)
-			goto rel_priv;
+			goto out;
 	}
 	error = -EFAULT;
 	if (!copy_to_user(l, &flock, sizeof(flock)))
 		error = 0;
-rel_priv:
-	locks_release_private(&file_lock);
 out:
+	locks_free_lock(fl);
 	return error;
 }
 
@@ -2317,10 +2322,14 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
  */
 int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
 {
-	struct file_lock file_lock;
+	struct file_lock *fl;
 	struct flock64 flock;
 	int error;
 
+	fl = locks_alloc_lock();
+	if (fl == NULL)
+		return -ENOMEM;
+
 	error = -EFAULT;
 	if (copy_from_user(&flock, l, sizeof(flock)))
 		goto out;
@@ -2328,7 +2337,7 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
 	if ((flock.l_type != F_RDLCK) && (flock.l_type != F_WRLCK))
 		goto out;
 
-	error = flock64_to_posix_lock(filp, &file_lock, &flock);
+	error = flock64_to_posix_lock(filp, fl, &flock);
 	if (error)
 		goto out;
 
@@ -2338,24 +2347,24 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
 			goto out;
 
 		cmd = F_GETLK64;
-		file_lock.fl_flags |= FL_OFDLCK;
-		file_lock.fl_owner = filp;
+		fl->fl_flags |= FL_OFDLCK;
+		fl->fl_owner = filp;
 	}
 
-	error = vfs_test_lock(filp, &file_lock);
+	error = vfs_test_lock(filp, fl);
 	if (error)
 		goto out;
 
-	flock.l_type = file_lock.fl_type;
-	if (file_lock.fl_type != F_UNLCK)
-		posix_lock_to_flock64(&flock, &file_lock);
+	flock.l_type = fl->fl_type;
+	if (fl->fl_type != F_UNLCK)
+		posix_lock_to_flock64(&flock, fl);
 
 	error = -EFAULT;
 	if (!copy_to_user(l, &flock, sizeof(flock)))
 		error = 0;
 
-	locks_release_private(&file_lock);
 out:
+	locks_free_lock(fl);
 	return error;
 }
 
-- 
2.9.3

[PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation

[Benjamin Coddington]

Since commit c69899a17ca4 "NFSv4: Update of VFS byte range lock must be
atomic with the stateid update", NFSv4 has been inserting locks in rpciod
worker context.  The result is that the file_lock's fl_nspid is the
kworker's pid instead of the original userspace pid.  We can fix that up by
setting fl_nspid in locks_allocate_lock, and tranfer it to the file_lock
that's eventually recorded.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/locks.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 54aeacf8dc46..270ae50247db 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -249,7 +249,9 @@ locks_dump_ctx_list(struct list_head *list, char *list_type)
 	struct file_lock *fl;
 
 	list_for_each_entry(fl, list, fl_list) {
-		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n", list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, fl->fl_pid);
+		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u fl_nspid=%u\n",
+			list_type, fl->fl_owner, fl->fl_flags, fl->fl_type,
+			fl->fl_pid, pid_vnr(fl->fl_nspid));
 	}
 }
 
@@ -294,8 +296,10 @@ struct file_lock *locks_alloc_lock(void)
 {
 	struct file_lock *fl = kmem_cache_zalloc(filelock_cache, GFP_KERNEL);
 
-	if (fl)
+	if (fl) {
 		locks_init_lock_heads(fl);
+		fl->fl_nspid = get_pid(task_tgid(current));
+	}
 
 	return fl;
 }
@@ -328,6 +332,8 @@ void locks_free_lock(struct file_lock *fl)
 	BUG_ON(!hlist_unhashed(&fl->fl_link));
 
 	locks_release_private(fl);
+	if (fl->fl_nspid)
+		put_pid(fl->fl_nspid);
 	kmem_cache_free(filelock_cache, fl);
 }
 EXPORT_SYMBOL(locks_free_lock);
@@ -357,8 +363,15 @@ EXPORT_SYMBOL(locks_init_lock);
  */
 void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
 {
+	struct pid *replace_pid = new->fl_nspid;
+
 	new->fl_owner = fl->fl_owner;
 	new->fl_pid = fl->fl_pid;
+	if (fl->fl_nspid) {
+		new->fl_nspid = get_pid(fl->fl_nspid);
+		if (replace_pid)
+			put_pid(replace_pid);
+	}
 	new->fl_file = NULL;
 	new->fl_flags = fl->fl_flags;
 	new->fl_type = fl->fl_type;
@@ -733,7 +746,6 @@ static void locks_wake_up_blocks(struct file_lock *blocker)
 static void
 locks_insert_lock_ctx(struct file_lock *fl, struct list_head *before)
 {
-	fl->fl_nspid = get_pid(task_tgid(current));
 	list_add_tail(&fl->fl_list, before);
 	locks_insert_global_locks(fl);
 }
@@ -743,10 +755,6 @@ locks_unlink_lock_ctx(struct file_lock *fl)
 {
 	locks_delete_global_locks(fl);
 	list_del_init(&fl->fl_list);
-	if (fl->fl_nspid) {
-		put_pid(fl->fl_nspid);
-		fl->fl_nspid = NULL;
-	}
 	locks_wake_up_blocks(fl);
 }
 
@@ -823,8 +831,6 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
 	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
 		if (posix_locks_conflict(fl, cfl)) {
 			locks_copy_conflock(fl, cfl);
-			if (cfl->fl_nspid)
-				fl->fl_pid = pid_vnr(cfl->fl_nspid);
 			goto out;
 		}
 	}
@@ -2492,6 +2498,7 @@ void locks_remove_posix(struct file *filp, fl_owner_t owner)
 	lock.fl_end = OFFSET_MAX;
 	lock.fl_owner = owner;
 	lock.fl_pid = current->tgid;
+	lock.fl_nspid = get_pid(task_tgid(current));
 	lock.fl_file = filp;
 	lock.fl_ops = NULL;
 	lock.fl_lmops = NULL;
@@ -2500,6 +2507,7 @@ void locks_remove_posix(struct file *filp, fl_owner_t owner)
 
 	if (lock.fl_ops && lock.fl_ops->fl_release_private)
 		lock.fl_ops->fl_release_private(&lock);
+	put_pid(lock.fl_nspid);
 	trace_locks_remove_posix(inode, &lock, error);
 }
 
@@ -2522,6 +2530,8 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 	if (list_empty(&flctx->flc_flock))
 		return;
 
+	fl.fl_nspid = get_pid(task_tgid(current));
+
 	if (filp->f_op->flock && is_remote_lock(filp))
 		filp->f_op->flock(filp, F_SETLKW, &fl);
 	else
@@ -2529,6 +2539,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 
 	if (fl.fl_ops && fl.fl_ops->fl_release_private)
 		fl.fl_ops->fl_release_private(&fl);
+	put_pid(fl.fl_nspid);
 }
 
 /* The i_flctx must be valid when calling into here */
-- 
2.9.3

[PATCH 3/3] fs/locks: Use fs-specific l_pid for remote locks

[Benjamin Coddington]

Now that we're setting fl_nspid at file_lock allocation, we need to handle
the case where a remote filesystem directly sets fl_pid.  If the filesystem
implements the lock operation, set a flag to return the lock's fl_pid,
rather than fl_nspid.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/locks.c         | 25 +++++++++++++++++++------
 include/linux/fs.h |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 270ae50247db..f1aac1680394 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2052,16 +2052,28 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
  */
 int vfs_test_lock(struct file *filp, struct file_lock *fl)
 {
-	if (filp->f_op->lock && is_remote_lock(filp))
+	if (filp->f_op->lock && is_remote_lock(filp)) {
+		fl->fl_flags |= FL_PID_PRIV;
 		return filp->f_op->lock(filp, F_GETLK, fl);
+	}
 	posix_test_lock(filp, fl);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vfs_test_lock);
 
+static inline int flock_translate_pid(struct file_lock *fl)
+{
+	if (IS_OFDLCK(fl))
+		return -1;
+	if (fl->fl_flags & FL_PID_PRIV)
+		return fl->fl_pid;
+	return pid_vnr(fl->fl_nspid);
+}
+
+
 static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl)
 {
-	flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid;
+	flock->l_pid = flock_translate_pid(fl);
 #if BITS_PER_LONG == 32
 	/*
 	 * Make sure we can represent the posix lock via
@@ -2083,7 +2095,7 @@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl)
 #if BITS_PER_LONG == 32
 static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl)
 {
-	flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid;
+	flock->l_pid = flock_translate_pid(fl);
 	flock->l_start = fl->fl_start;
 	flock->l_len = fl->fl_end == OFFSET_MAX ? 0 :
 		fl->fl_end - fl->fl_start + 1;
@@ -2636,7 +2648,9 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
 
-	if (fl->fl_nspid) {
+	if (fl->fl_flags & FL_PID_PRIV) {
+		fl_pid = fl->fl_pid;
+	} else {
 		struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
 
 		/* Don't let fl_pid change based on who is reading the file */
@@ -2649,8 +2663,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 		 */
 		if (fl_pid == 0)
 			return;
-	} else
-		fl_pid = fl->fl_pid;
+	}
 
 	if (fl->fl_file != NULL)
 		inode = locks_inode(fl->fl_file);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0ad325ed71e8..8d945b86b9f3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -908,6 +908,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_PID_PRIV	4096	/* F_GETLK should report fl_pid */
 
 #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
 
-- 
2.9.3

Re: [PATCH 0/3] Fixups for l_pid

[Benjamin Coddington]

On 26 May 2017, at 16:14, Benjamin Coddington wrote:

> LTP fcntl tests (fcntl11 fcntl14 fcntl17 fcntl19 fcntl20 fcntl21) have 
> been
> failing for NFSv4 mounts due to an unexpected l_pid.  What follows are 
> some
> fixups:
>
> Benjamin Coddington (3):
>   fs/locks: Alloc file_lock where practical
>   fs/locks: Set fl_nspid at file_lock allocation
>   fs/locks: Use fs-specific l_pid for remote locks
>
>  fs/locks.c         | 137 
> +++++++++++++++++++++++++++++++++--------------------
>  include/linux/fs.h |   1 +
>  2 files changed, 86 insertions(+), 52 deletions(-)

Ah, Jeff, I just saw your other email asking to add comments documenting 
the behavior for the four cases.  Would you like another patch for 
those, or should I send another version rolling them into one of these?

Ben

Re: [PATCH 1/3] fs/locks: Alloc file_lock where practical

[Jeff Layton]

On Fri, 2017-05-26 at 16:14 -0400, Benjamin Coddington wrote:
> Use an allocation it where makes sense to do so.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/locks.c | 83 ++++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index af2031a1fcff..54aeacf8dc46 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1293,36 +1293,39 @@ int locks_mandatory_locked(struct file *file)
>  int locks_mandatory_area(struct inode *inode, struct file *filp, loff_t start,
>  			 loff_t end, unsigned char type)
>  {
> -	struct file_lock fl;
> +	struct file_lock *fl;
>  	int error;
>  	bool sleep = false;
>  
> -	locks_init_lock(&fl);
> -	fl.fl_pid = current->tgid;
> -	fl.fl_file = filp;
> -	fl.fl_flags = FL_POSIX | FL_ACCESS;
> +	fl = locks_alloc_lock();
> +	if (fl == NULL)
> +		return -ENOMEM;
> +

I'm fine with allocating these for GETLK, but locks_mandatory_area will
end up getting called on read/write operations if mandatory locking is
in effect.

OTOH, we have been (slowly!) moving away from supporting mandatory
locks, so maybe it's OK to do an extra alloc/free in that case. Meh,
ok...

> +	fl->fl_pid = current->tgid;
> +	fl->fl_file = filp;
> +	fl->fl_flags = FL_POSIX | FL_ACCESS;
>  	if (filp && !(filp->f_flags & O_NONBLOCK))
>  		sleep = true;
> -	fl.fl_type = type;
> -	fl.fl_start = start;
> -	fl.fl_end = end;
> +	fl->fl_type = type;
> +	fl->fl_start = start;
> +	fl->fl_end = end;
>  
>  	for (;;) {
>  		if (filp) {
> -			fl.fl_owner = filp;
> -			fl.fl_flags &= ~FL_SLEEP;
> -			error = posix_lock_inode(inode, &fl, NULL);
> +			fl->fl_owner = filp;
> +			fl->fl_flags &= ~FL_SLEEP;
> +			error = posix_lock_inode(inode, fl, NULL);
>  			if (!error)
>  				break;
>  		}
>  
>  		if (sleep)
> -			fl.fl_flags |= FL_SLEEP;
> -		fl.fl_owner = current->files;
> -		error = posix_lock_inode(inode, &fl, NULL);
> +			fl->fl_flags |= FL_SLEEP;
> +		fl->fl_owner = current->files;
> +		error = posix_lock_inode(inode, fl, NULL);
>  		if (error != FILE_LOCK_DEFERRED)
>  			break;
> -		error = wait_event_interruptible(fl.fl_wait, !fl.fl_next);
> +		error = wait_event_interruptible(fl->fl_wait, !fl->fl_next);
>  		if (!error) {
>  			/*
>  			 * If we've been sleeping someone might have
> @@ -1332,10 +1335,10 @@ int locks_mandatory_area(struct inode *inode, struct file *filp, loff_t start,
>  				continue;
>  		}
>  
> -		locks_delete_block(&fl);
> +		locks_delete_block(fl);
>  		break;
>  	}
> -
> +	locks_free_lock(fl);
>  	return error;
>  }
>  
> @@ -2088,10 +2091,13 @@ static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl)
>   */
>  int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
>  {
> -	struct file_lock file_lock;
> +	struct file_lock *fl;
>  	struct flock flock;
>  	int error;
>  
> +	fl = locks_alloc_lock();
> +	if (fl == NULL)
> +		return -ENOMEM;
>  	error = -EFAULT;
>  	if (copy_from_user(&flock, l, sizeof(flock)))
>  		goto out;
> @@ -2099,7 +2105,7 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
>  	if ((flock.l_type != F_RDLCK) && (flock.l_type != F_WRLCK))
>  		goto out;
>  
> -	error = flock_to_posix_lock(filp, &file_lock, &flock);
> +	error = flock_to_posix_lock(filp, fl, &flock);
>  	if (error)
>  		goto out;
>  
> @@ -2109,26 +2115,25 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
>  			goto out;
>  
>  		cmd = F_GETLK;
> -		file_lock.fl_flags |= FL_OFDLCK;
> -		file_lock.fl_owner = filp;
> +		fl->fl_flags |= FL_OFDLCK;
> +		fl->fl_owner = filp;
>  	}
>  
> -	error = vfs_test_lock(filp, &file_lock);
> +	error = vfs_test_lock(filp, fl);
>  	if (error)
>  		goto out;
>   
> -	flock.l_type = file_lock.fl_type;
> -	if (file_lock.fl_type != F_UNLCK) {
> -		error = posix_lock_to_flock(&flock, &file_lock);
> +	flock.l_type = fl->fl_type;
> +	if (fl->fl_type != F_UNLCK) {
> +		error = posix_lock_to_flock(&flock, fl);
>  		if (error)
> -			goto rel_priv;
> +			goto out;
>  	}
>  	error = -EFAULT;
>  	if (!copy_to_user(l, &flock, sizeof(flock)))
>  		error = 0;
> -rel_priv:
> -	locks_release_private(&file_lock);
>  out:
> +	locks_free_lock(fl);
>  	return error;
>  }
>  
> @@ -2317,10 +2322,14 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
>   */
>  int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
>  {
> -	struct file_lock file_lock;
> +	struct file_lock *fl;
>  	struct flock64 flock;
>  	int error;
>  
> +	fl = locks_alloc_lock();
> +	if (fl == NULL)
> +		return -ENOMEM;
> +
>  	error = -EFAULT;
>  	if (copy_from_user(&flock, l, sizeof(flock)))
>  		goto out;
> @@ -2328,7 +2337,7 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
>  	if ((flock.l_type != F_RDLCK) && (flock.l_type != F_WRLCK))
>  		goto out;
>  
> -	error = flock64_to_posix_lock(filp, &file_lock, &flock);
> +	error = flock64_to_posix_lock(filp, fl, &flock);
>  	if (error)
>  		goto out;
>  
> @@ -2338,24 +2347,24 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
>  			goto out;
>  
>  		cmd = F_GETLK64;
> -		file_lock.fl_flags |= FL_OFDLCK;
> -		file_lock.fl_owner = filp;
> +		fl->fl_flags |= FL_OFDLCK;
> +		fl->fl_owner = filp;
>  	}
>  
> -	error = vfs_test_lock(filp, &file_lock);
> +	error = vfs_test_lock(filp, fl);
>  	if (error)
>  		goto out;
>  
> -	flock.l_type = file_lock.fl_type;
> -	if (file_lock.fl_type != F_UNLCK)
> -		posix_lock_to_flock64(&flock, &file_lock);
> +	flock.l_type = fl->fl_type;
> +	if (fl->fl_type != F_UNLCK)
> +		posix_lock_to_flock64(&flock, fl);
>  
>  	error = -EFAULT;
>  	if (!copy_to_user(l, &flock, sizeof(flock)))
>  		error = 0;
>  
> -	locks_release_private(&file_lock);
>  out:
> +	locks_free_lock(fl);
>  	return error;
>  }
>  

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation

[Jeff Layton]

On Fri, 2017-05-26 at 16:14 -0400, Benjamin Coddington wrote:
> Since commit c69899a17ca4 "NFSv4: Update of VFS byte range lock must be
> atomic with the stateid update", NFSv4 has been inserting locks in rpciod
> worker context.  The result is that the file_lock's fl_nspid is the
> kworker's pid instead of the original userspace pid.  We can fix that up by
> setting fl_nspid in locks_allocate_lock, and tranfer it to the file_lock
> that's eventually recorded.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/locks.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 54aeacf8dc46..270ae50247db 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -249,7 +249,9 @@ locks_dump_ctx_list(struct list_head *list, char *list_type)
>  	struct file_lock *fl;
>  
>  	list_for_each_entry(fl, list, fl_list) {
> -		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n", list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, fl->fl_pid);
> +		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u fl_nspid=%u\n",
> +			list_type, fl->fl_owner, fl->fl_flags, fl->fl_type,
> +			fl->fl_pid, pid_vnr(fl->fl_nspid));
>  	}
>  }
>  
> @@ -294,8 +296,10 @@ struct file_lock *locks_alloc_lock(void)
>  {
>  	struct file_lock *fl = kmem_cache_zalloc(filelock_cache, GFP_KERNEL);
>  
> -	if (fl)
> +	if (fl) {
>  		locks_init_lock_heads(fl);
> +		fl->fl_nspid = get_pid(task_tgid(current));
> +	}
>  
>  	return fl;
>  }
> @@ -328,6 +332,8 @@ void locks_free_lock(struct file_lock *fl)
>  	BUG_ON(!hlist_unhashed(&fl->fl_link));
>  
>  	locks_release_private(fl);
> +	if (fl->fl_nspid)
> +		put_pid(fl->fl_nspid);
>  	kmem_cache_free(filelock_cache, fl);
>  }
>  EXPORT_SYMBOL(locks_free_lock);
> @@ -357,8 +363,15 @@ EXPORT_SYMBOL(locks_init_lock);
>   */
>  void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
>  {
> +	struct pid *replace_pid = new->fl_nspid;
> +
>  	new->fl_owner = fl->fl_owner;
>  	new->fl_pid = fl->fl_pid;
> +	if (fl->fl_nspid) {
> +		new->fl_nspid = get_pid(fl->fl_nspid);
> +		if (replace_pid)
> +			put_pid(replace_pid);
> +	}
>  	new->fl_file = NULL;
>  	new->fl_flags = fl->fl_flags;
>  	new->fl_type = fl->fl_type;
> @@ -733,7 +746,6 @@ static void locks_wake_up_blocks(struct file_lock *blocker)
>  static void
>  locks_insert_lock_ctx(struct file_lock *fl, struct list_head *before)
>  {
> -	fl->fl_nspid = get_pid(task_tgid(current));
>  	list_add_tail(&fl->fl_list, before);
>  	locks_insert_global_locks(fl);
>  }
> @@ -743,10 +755,6 @@ locks_unlink_lock_ctx(struct file_lock *fl)
>  {
>  	locks_delete_global_locks(fl);
>  	list_del_init(&fl->fl_list);
> -	if (fl->fl_nspid) {
> -		put_pid(fl->fl_nspid);
> -		fl->fl_nspid = NULL;
> -	}
>  	locks_wake_up_blocks(fl);
>  }
>  
> @@ -823,8 +831,6 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
>  		if (posix_locks_conflict(fl, cfl)) {
>  			locks_copy_conflock(fl, cfl);
> -			if (cfl->fl_nspid)
> -				fl->fl_pid = pid_vnr(cfl->fl_nspid);
>  			goto out;
>  		}
>  	}
> @@ -2492,6 +2498,7 @@ void locks_remove_posix(struct file *filp, fl_owner_t owner)
>  	lock.fl_end = OFFSET_MAX;
>  	lock.fl_owner = owner;
>  	lock.fl_pid = current->tgid;
> +	lock.fl_nspid = get_pid(task_tgid(current));
>  	lock.fl_file = filp;
>  	lock.fl_ops = NULL;
>  	lock.fl_lmops = NULL;
> @@ -2500,6 +2507,7 @@ void locks_remove_posix(struct file *filp, fl_owner_t owner)
>  
>  	if (lock.fl_ops && lock.fl_ops->fl_release_private)
>  		lock.fl_ops->fl_release_private(&lock);
> +	put_pid(lock.fl_nspid);
>  	trace_locks_remove_posix(inode, &lock, error);
>  }
>  
> @@ -2522,6 +2530,8 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>  	if (list_empty(&flctx->flc_flock))
>  		return;
>  
> +	fl.fl_nspid = get_pid(task_tgid(current));
> +
>  	if (filp->f_op->flock && is_remote_lock(filp))
>  		filp->f_op->flock(filp, F_SETLKW, &fl);
>  	else
> @@ -2529,6 +2539,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>  
>  	if (fl.fl_ops && fl.fl_ops->fl_release_private)
>  		fl.fl_ops->fl_release_private(&fl);
> +	put_pid(fl.fl_nspid);

Maybe we should make a new helper function that does fl_release_private
and put_pid? That can be done in a later cleanup patch though. This
looks fine in the meantime.

>  }
>  
>  /* The i_flctx must be valid when calling into here */

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 0/3] Fixups for l_pid

[Jeff Layton]

On Fri, 2017-05-26 at 16:14 -0400, Benjamin Coddington wrote:
> LTP fcntl tests (fcntl11 fcntl14 fcntl17 fcntl19 fcntl20 fcntl21) have been
> failing for NFSv4 mounts due to an unexpected l_pid.  What follows are some
> fixups:
> 
> Benjamin Coddington (3):
>   fs/locks: Alloc file_lock where practical
>   fs/locks: Set fl_nspid at file_lock allocation
>   fs/locks: Use fs-specific l_pid for remote locks
> 
>  fs/locks.c         | 137 +++++++++++++++++++++++++++++++++--------------------
>  include/linux/fs.h |   1 +
>  2 files changed, 86 insertions(+), 52 deletions(-)
> 

Thanks Ben. These all look fine to me.

Could you rebase them on top of current linux-next? Christoph sent some
patches last week that are generating conflicts here and this doesn't
quite merge in correctly.

Thanks,
-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 1/3] fs/locks: Alloc file_lock where practical

[Christoph Hellwig]

On Fri, May 26, 2017 at 04:14:49PM -0400, Benjamin Coddington wrote:
> Use an allocation it where makes sense to do so.

Care to expain why it makes sense?