[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?

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

[Benjamin Coddington]

Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
handle the case where a remote filesystem directly sets fl_pid.  In that
case, the fl_pid should not be translated into a local pid namespace.  If
the filesystem implements the lock operation, set a flag to return the
lock's fl_pid value directly, rather translate it.

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

diff --git a/fs/locks.c b/fs/locks.c
index 104398ccc9b9..6858ec9802d2 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2034,8 +2034,10 @@ 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;
 }
@@ -2066,9 +2068,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns)
 	return vnr;
 }
 
+static pid_t 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 locks_translate_pid(fl->fl_pid,  task_active_pid_ns(current));
+}
+
 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
@@ -2090,7 +2101,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;
@@ -2604,7 +2615,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	unsigned int fl_pid;
 	struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
 
-	fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
+	if (fl->fl_flags & FL_PID_PRIV)
+		fl_pid = fl->fl_pid;
+	else
+		fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
 	/*
 	 * If there isn't a fl_pid don't display who is waiting on
 	 * the lock if we are called from locks_show, or if we are
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b013fac515f7..179496a9719d 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

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

[Benjamin Coddington]

Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
handle the case where a remote filesystem directly sets fl_pid.  In that
case, the fl_pid should not be translated into a local pid namespace.  If
the filesystem implements the lock operation, set a flag to return the
lock's fl_pid value directly, rather translate it.

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

diff --git a/fs/locks.c b/fs/locks.c
index 8d48e4c42ed3..206a46d28bbd 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2034,8 +2034,10 @@ 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;
 }
@@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns)
 	return vnr;
 }
 
+static pid_t 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 locks_translate_pid(fl->fl_pid,  task_active_pid_ns(current));
+}
+
 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
@@ -2084,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;
@@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	unsigned int fl_pid;
 	struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
 
-	fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
+	if (fl->fl_flags & FL_PID_PRIV)
+		fl_pid = fl->fl_pid;
+	else
+		fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
 	/*
 	 * If there isn't a fl_pid don't display who is waiting on
 	 * the lock if we are called from locks_show, or if we are
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b013fac515f7..179496a9719d 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 3/3] fs/locks: Use fs-specific l_pid for remote locks

[Jeff Layton]

On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote:
> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
> handle the case where a remote filesystem directly sets fl_pid.  In that
> case, the fl_pid should not be translated into a local pid namespace.  If
> the filesystem implements the lock operation, set a flag to return the
> lock's fl_pid value directly, rather translate it.
> 

Actually, you're not translating anything for F_GETLK until we get to
this patch. Patch #2 in this series removes the fl_nspid field, but the
pid translation isn't fixed until here. That does mean a nominal
regression here in how fl_pid is reported between the two.

Would it be best to squash #2 and #3 together? Or maybe just go ahead
and universally translate the fl_pid field until you add the flag in
this patch?

Also to make sure I understand: task->tgid will always represent the
task's pid in the init_pid_ns, right?

Other than the minor bisectability concern, I think this looks good.
Nice work!

> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/locks.c         | 22 ++++++++++++++++++----
>  include/linux/fs.h |  1 +
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 8d48e4c42ed3..206a46d28bbd 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2034,8 +2034,10 @@ 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;
>  }
> @@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns)
>  	return vnr;
>  }
>  
> +static pid_t 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 locks_translate_pid(fl->fl_pid,  task_active_pid_ns(current));
> +}
> +
>  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
> @@ -2084,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;
> @@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	unsigned int fl_pid;
>  	struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
>  
> -	fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
> +	if (fl->fl_flags & FL_PID_PRIV)
> +		fl_pid = fl->fl_pid;
> +	else
> +		fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
>  	/*
>  	 * If there isn't a fl_pid don't display who is waiting on
>  	 * the lock if we are called from locks_show, or if we are
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b013fac515f7..179496a9719d 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)
>  

-- 
Jeff Layton <jlayton@poochiereds.net>

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

[Jeff Layton]

On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote:
> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
> handle the case where a remote filesystem directly sets fl_pid.  In that
> case, the fl_pid should not be translated into a local pid namespace.  If
> the filesystem implements the lock operation, set a flag to return the
> lock's fl_pid value directly, rather translate it.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/locks.c         | 22 ++++++++++++++++++----
>  include/linux/fs.h |  1 +
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 8d48e4c42ed3..206a46d28bbd 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2034,8 +2034,10 @@ 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;
>  }

I do have one concern here with setting the flag at this point. It's
possible with NFS that we'll end up setting a lock locally if we have a
delegation (also true for CIFS and probably Ceph -- maybe others too).

Here though, you're setting FL_PID_PRIV so those locks will always show
a l_pid of current->tgid, even when you're querying it from a different
pid namespace.

If we want to go this route, then you'll also need to fix NFS to clear
that flag when it sets the lock locally, and audit other fs' that have a
->lock operation for the same thing.

However, I think it might be cleaner to have the filesystems set that
flag to opt out of the default pid translation.

Thoughts?
 
> @@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns)
>  	return vnr;
>  }
>  
> +static pid_t 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 locks_translate_pid(fl->fl_pid,  task_active_pid_ns(current));
> +}
> +
>  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
> @@ -2084,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;
> @@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	unsigned int fl_pid;
>  	struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
>  
> -	fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
> +	if (fl->fl_flags & FL_PID_PRIV)
> +		fl_pid = fl->fl_pid;
> +	else
> +		fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns);
>  	/*
>  	 * If there isn't a fl_pid don't display who is waiting on
>  	 * the lock if we are called from locks_show, or if we are
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b013fac515f7..179496a9719d 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)
>  

-- 
Jeff Layton <jlayton@poochiereds.net>

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

[Benjamin Coddington]

Apologies for the delayed response..

On 7 Jun 2017, at 7:40, Jeff Layton wrote:

> On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote:
>> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
>> handle the case where a remote filesystem directly sets fl_pid.  In that
>> case, the fl_pid should not be translated into a local pid namespace.  If
>> the filesystem implements the lock operation, set a flag to return the
>> lock's fl_pid value directly, rather translate it.
>>
>
> Actually, you're not translating anything for F_GETLK until we get to
> this patch. Patch #2 in this series removes the fl_nspid field, but the
> pid translation isn't fixed until here. That does mean a nominal
> regression here in how fl_pid is reported between the two.

Good catch.

> Would it be best to squash #2 and #3 together? Or maybe just go ahead
> and universally translate the fl_pid field until you add the flag in
> this patch?

I'll send a v4 that universally translates the fl_pid field until this
patch.  I think the first two patches should be separate.

> Also to make sure I understand: task->tgid will always represent the
> task's pid in the init_pid_ns, right?

Yes.  (I was incorrect on IRC last week), as is seen in kernel/fork.c:
1748     if (pid != &init_struct_pid) {
1749         pid = alloc_pid(p->nsproxy->pid_ns_for_children);
...
1754     }
...
1785     p->pid = pid_nr(pid);
1786     if (clone_flags & CLONE_THREAD) {
...
1789         p->tgid = current->tgid;
1790     } else {
...
1796         p->tgid = p->pid;
1797     }

.. and include/linux/pid.h:
153 /*
154  * the helpers to get the pid's id seen from different namespaces
...
156  * pid_nr()    : global id, i.e. the id seen from the init namespace;
...
162  */
163
164 static inline pid_t pid_nr(struct pid *pid)
165 {
166     pid_t nr = 0;
167     if (pid)
168         nr = pid->numbers[0].nr;
169     return nr;
170 }

Ben

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

[Benjamin Coddington]

On 19 Jun 2017, at 8:37, Benjamin Coddington wrote:

> Apologies for the delayed response..
>
> On 7 Jun 2017, at 7:40, Jeff Layton wrote:
>
>> On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote:
>>> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to
>>> handle the case where a remote filesystem directly sets fl_pid.  In that
>>> case, the fl_pid should not be translated into a local pid namespace.  If
>>> the filesystem implements the lock operation, set a flag to return the
>>> lock's fl_pid value directly, rather translate it.
>>>
>>
>> Actually, you're not translating anything for F_GETLK until we get to
>> this patch. Patch #2 in this series removes the fl_nspid field, but the
>> pid translation isn't fixed until here. That does mean a nominal
>> regression here in how fl_pid is reported between the two.
>
> Good catch.
>
>> Would it be best to squash #2 and #3 together? Or maybe just go ahead
>> and universally translate the fl_pid field until you add the flag in
>> this patch?
>
> I'll send a v4 that universally translates the fl_pid field until this
> patch.  I think the first two patches should be separate.

Ah, but /2 and 3/ should just be squashed, yes I agree with that.

Ben