Re: [PATCH] locks: Set fl_nspid at file_lock allocation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2017-05-18 at 12:02 -0400, Benjamin Coddington wrote:
diff --git a/fs/locks.c b/fs/locks.c
index af2031a1fcff..959b3f93f4bd 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -249,7 +249,7 @@ 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\n", list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, pid_vnr(fl->fl_nspid));
 	}
 }


Probably should change the format to say fl_nspid=%u here, just to be
clear. Might also want to keep fl_pid in there since the lock manager
could set it.

Yes, I thought about just spitting out both.  Let's do that.

@@ -2074,7 +2075,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 = IS_OFDLCK(fl) ? -1 : pid_vnr(fl->fl_nspid);

What about the lock managers that _do_ set fl->fl_pid. With nfsv2/3,
this is always going to give you back the pid of lockd, AFAICT.

But isn't this really what you want? If a local process wants to know who
holds a conflicting lock, the fl_pid of a remote system is really pretty
useless. Not only that, but there's no way for the local process to know when the pid is local or remote. Better to be consistent and always return
something that's useful.

Do we want to present the pid value that the client sent here instead in
that case? Maybe the lm could set a fl_flag to indicate that the pid
should be taken directly from fl_pid here? Then you could move the above
logic to a static inline or something.

Alternately, you could add a new lm_present_pid operation to lock
managers to format the pid as they see fit.

Either works to solve the problem, but I still think that F_GETLK and
/proc/locks should only return local pids.

@@ -2322,6 +2325,7 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
 	int error;

 	error = -EFAULT;
+	file_lock.fl_nspid = get_pid(task_tgid(current));

Might it be cleaner to create a FILE_LOCK(name) macro that does this on
the stack (like LIST_HEAD()) ?

Yes, it would.  I'll do it.

@@ -2520,6 +2527,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);

I think we only need to take a reference for when the file_lock can
outlive the current task, so the get/put may not be necessary in these
functions.

Right.. of course.  I can clean those up.

Ben



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux