* [PATCH 1/3] fs/locks: Alloc file_lock where practical
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
@ 2017-05-26 20:14 ` Benjamin Coddington
2017-05-27 9:56 ` Jeff Layton
2017-05-28 6:35 ` Christoph Hellwig
2017-05-26 20:14 ` [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation Benjamin Coddington
` (3 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Benjamin Coddington @ 2017-05-26 20:14 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, bfields; +Cc: linux-fsdevel, linux-nfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] fs/locks: Alloc file_lock where practical
2017-05-26 20:14 ` [PATCH 1/3] fs/locks: Alloc file_lock where practical Benjamin Coddington
@ 2017-05-27 9:56 ` Jeff Layton
2017-05-28 6:35 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2017-05-27 9:56 UTC (permalink / raw)
To: Benjamin Coddington, Alexander Viro, bfields; +Cc: linux-fsdevel, linux-nfs
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] fs/locks: Alloc file_lock where practical
2017-05-26 20:14 ` [PATCH 1/3] fs/locks: Alloc file_lock where practical Benjamin Coddington
2017-05-27 9:56 ` Jeff Layton
@ 2017-05-28 6:35 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2017-05-28 6:35 UTC (permalink / raw)
To: Benjamin Coddington
Cc: Alexander Viro, Jeff Layton, bfields, linux-fsdevel, linux-nfs
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
2017-05-26 20:14 ` [PATCH 1/3] fs/locks: Alloc file_lock where practical Benjamin Coddington
@ 2017-05-26 20:14 ` Benjamin Coddington
2017-05-27 10:00 ` Jeff Layton
2017-05-26 20:14 ` [PATCH 3/3] fs/locks: Use fs-specific l_pid for remote locks Benjamin Coddington
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Benjamin Coddington @ 2017-05-26 20:14 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, bfields; +Cc: linux-fsdevel, linux-nfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation
2017-05-26 20:14 ` [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation Benjamin Coddington
@ 2017-05-27 10:00 ` Jeff Layton
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2017-05-27 10:00 UTC (permalink / raw)
To: Benjamin Coddington, Alexander Viro, bfields; +Cc: linux-fsdevel, linux-nfs
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] fs/locks: Use fs-specific l_pid for remote locks
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
2017-05-26 20:14 ` [PATCH 1/3] fs/locks: Alloc file_lock where practical Benjamin Coddington
2017-05-26 20:14 ` [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation Benjamin Coddington
@ 2017-05-26 20:14 ` Benjamin Coddington
2017-05-26 20:26 ` [PATCH 0/3] Fixups for l_pid Benjamin Coddington
2017-05-27 10:11 ` Jeff Layton
4 siblings, 0 replies; 9+ messages in thread
From: Benjamin Coddington @ 2017-05-26 20:14 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, bfields; +Cc: linux-fsdevel, linux-nfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/3] Fixups for l_pid
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
` (2 preceding siblings ...)
2017-05-26 20:14 ` [PATCH 3/3] fs/locks: Use fs-specific l_pid for remote locks Benjamin Coddington
@ 2017-05-26 20:26 ` Benjamin Coddington
2017-05-27 10:11 ` Jeff Layton
4 siblings, 0 replies; 9+ messages in thread
From: Benjamin Coddington @ 2017-05-26 20:26 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-fsdevel, linux-nfs, bfields, Alexander Viro
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/3] Fixups for l_pid
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
` (3 preceding siblings ...)
2017-05-26 20:26 ` [PATCH 0/3] Fixups for l_pid Benjamin Coddington
@ 2017-05-27 10:11 ` Jeff Layton
4 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2017-05-27 10:11 UTC (permalink / raw)
To: Benjamin Coddington, Alexander Viro, bfields; +Cc: linux-fsdevel, linux-nfs
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>
^ permalink raw reply [flat|nested] 9+ messages in thread