Re: [PATCH] proc: show locks in /proc/pid/fdinfo/X

From: Andrew Vagin <avagin@parallels.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrey Vagin <avagin@openvz.org>, <linux-kernel@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Jeff Layton <jlayton@poochiereds.net>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	"Cyrill Gorcunov" <gorcunov@openvz.org>,
	Pavel Emelyanov <xemul@parallels.com>,
	"Joe Perches" <joe@perches.com>
Subject: Re: [PATCH] proc: show locks in /proc/pid/fdinfo/X
Date: Thu, 12 Mar 2015 18:54:42 +0300	[thread overview]
Message-ID: <20150312155441.GA19139@paralelels.com> (raw)
In-Reply-To: <20150311150853.492fee52def529e86506976b@linux-foundation.org>

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Wed, Mar 11, 2015 at 03:08:53PM -0700, Andrew Morton wrote:
> On Thu,  5 Mar 2015 18:37:18 +0300 Andrey Vagin <avagin@openvz.org> wrote:
> 
> > Let's show locks which are associated with a file descriptor in
> > its fdinfo file.
> > 
> > Currently we don't have a reliable way to determine who holds a lock.
> > We can find some information in /proc/locks, but PID which is reported
> > there can be wrong. For example, a process takes a lock, then forks a
> > child and dies. In this case /proc/locks contains the parent pid, which
> > can be reused by another process.
> > 
> > $ cat /proc/locks
> > ...
> > 6: FLOCK  ADVISORY  WRITE 324 00:13:13431 0 EOF
> > ...
> > 
> > $ ps -C rpcbind
> >   PID TTY          TIME CMD
> >   332 ?        00:00:00 rpcbind
> > 
> > $ cat /proc/332/fdinfo/4
> > pos:	0
> > flags:	0100000
> > mnt_id:	22
> > lock:	1: FLOCK  ADVISORY  WRITE 324 00:13:13431 0 EOF
> > 
> > $ ls -l /proc/332/fd/4
> > lr-x------ 1 root root 64 Mar  5 14:43 /proc/332/fd/4 -> /run/rpcbind.lock
> > 
> > $ ls -l /proc/324/fd/
> > total 0
> > lrwx------ 1 root root 64 Feb 27 14:50 0 -> /dev/pts/0
> > lrwx------ 1 root root 64 Feb 27 14:50 1 -> /dev/pts/0
> > lrwx------ 1 root root 64 Feb 27 14:49 2 -> /dev/pts/0
> > 
> > You can see that the process with the 324 pid doesn't hold the lock.
> > 
> > This information is required for proper dumping and restoring file
> > locks.
> > 
> > ...
> >
> > --- a/fs/proc/fd.c
> > +++ b/fs/proc/fd.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/security.h>
> >  #include <linux/file.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/fs.h>
> >  
> >  #include <linux/proc_fs.h>
> >  
> > @@ -48,16 +49,24 @@ static int seq_show(struct seq_file *m, void *v)
> >  		put_files_struct(files);
> >  	}
> >  
> > -	if (!ret) {
> > -		seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
> > -			   (long long)file->f_pos, f_flags,
> > -			   real_mount(file->f_path.mnt)->mnt_id);
> > -		if (file->f_op->show_fdinfo)
> > -			file->f_op->show_fdinfo(m, file);
> > -		ret = seq_has_overflowed(m);
> > -		fput(file);
> > -	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
> > +		   (long long)file->f_pos, f_flags,
> > +		   real_mount(file->f_path.mnt)->mnt_id);
> > +
> > +	show_fd_locks(m, file, files);
> > +	ret = seq_has_overflowed(m);
> > +	if (ret)
> > +		goto out;
> 
> seq_has_overflowed() returns a boolean, but fs/seq_file.c:traverse() is
> looking for a -ve errno from ->show().
> 
> Documentation/filesystems/seq_file.txt says
> 
> If all is well, the show() function should return zero.  A negative error
> code in the usual manner indicates that something went wrong; it will be
> passed back to user space.  This function can also return SEQ_SKIP, which
> causes the current item to be skipped; if the show() function has already
> generated output before returning SEQ_SKIP, that output will be dropped.

You are right. The updated version of this patch is attached. Thank you
for the review.

> 
> 
> > +	if (file->f_op->show_fdinfo)
> > +		file->f_op->show_fdinfo(m, file);
> > +	ret = seq_has_overflowed(m);
> >  
> > +out:
> > +	fput(file);
> >  	return ret;
> >  }
> 

[-- Attachment #2: 0001-proc-show-locks-in-proc-pid-fdinfo-X-v2.patch --]
[-- Type: text/plain, Size: 5864 bytes --]

>From 29fcabc8a9d9c8bea4a7ec6202a7c0f6e98a518e Mon Sep 17 00:00:00 2001
From: Andrey Vagin <avagin@openvz.org>
Date: Thu, 26 Feb 2015 15:46:02 +0300
Subject: [PATCH] proc: show locks in /proc/pid/fdinfo/X (v2)

Let's show locks which are associated with a file descriptor in
its fdinfo file.

Currently we don't have a reliable way to determine who holds a lock.
We can find some information in /proc/locks, but PID which is reported
there can be wrong. For example, a process takes a lock, then forks a
child and dies. In this case /proc/locks contains the parent pid, which
can be reused by another process.

$ cat /proc/locks
...
6: FLOCK  ADVISORY  WRITE 324 00:13:13431 0 EOF
...

$ ps -C rpcbind
  PID TTY          TIME CMD
  332 ?        00:00:00 rpcbind

$ cat /proc/332/fdinfo/4
pos:	0
flags:	0100000
mnt_id:	22
lock:	1: FLOCK  ADVISORY  WRITE 324 00:13:13431 0 EOF

$ ls -l /proc/332/fd/4
lr-x------ 1 root root 64 Mar  5 14:43 /proc/332/fd/4 -> /run/rpcbind.lock

$ ls -l /proc/324/fd/
total 0
lrwx------ 1 root root 64 Feb 27 14:50 0 -> /dev/pts/0
lrwx------ 1 root root 64 Feb 27 14:50 1 -> /dev/pts/0
lrwx------ 1 root root 64 Feb 27 14:49 2 -> /dev/pts/0

You can see that the process with the 324 pid doesn't hold the lock.

This information is required for proper dumping and restoring file
locks.

v1:
Acked-by: Jeff Layton <jlayton@poochiereds.net>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: "J. Bruce Fields" <bfields@fieldses.org>

v2: use seq_has_overflowed() properly

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 Documentation/filesystems/proc.txt |  4 ++++
 fs/locks.c                         | 38 ++++++++++++++++++++++++++++++++++++++
 fs/proc/fd.c                       | 27 +++++++++++++++++----------
 include/linux/fs.h                 |  7 +++++++
 4 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index a07ba61..6331623 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -1704,6 +1704,10 @@ A typical output is
 	flags:	0100002
 	mnt_id:	19
 
+All locks associated with a file descriptor are shown in its fdinfo too.
+
+lock:       1: FLOCK  ADVISORY  WRITE 359 00:13:11691 0 EOF
+
 The files such as eventfd, fsnotify, signalfd, epoll among the regular pos/flags
 pair provide additional information particular to the objects they represent.
 
diff --git a/fs/locks.c b/fs/locks.c
index f1bad68..89a0540 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2593,6 +2593,44 @@ static int locks_show(struct seq_file *f, void *v)
 	return 0;
 }
 
+static void __show_fd_locks(struct seq_file *f,
+			struct list_head *head, int *id,
+			struct file *filp, struct files_struct *files)
+{
+	struct file_lock *fl;
+
+	list_for_each_entry(fl, head, fl_list) {
+
+		if (filp != fl->fl_file)
+			continue;
+		if (fl->fl_owner != files &&
+		    fl->fl_owner != filp)
+			continue;
+
+		(*id)++;
+		seq_puts(f, "lock:\t");
+		lock_get_status(f, fl, *id, "");
+	}
+}
+
+void show_fd_locks(struct seq_file *f,
+		  struct file *filp, struct files_struct *files)
+{
+	struct inode *inode = file_inode(filp);
+	struct file_lock_context *ctx;
+	int id = 0;
+
+	ctx = inode->i_flctx;
+	if (!ctx)
+		return;
+
+	spin_lock(&ctx->flc_lock);
+	__show_fd_locks(f, &ctx->flc_flock, &id, filp, files);
+	__show_fd_locks(f, &ctx->flc_posix, &id, filp, files);
+	__show_fd_locks(f, &ctx->flc_lease, &id, filp, files);
+	spin_unlock(&ctx->flc_lock);
+}
+
 static void *locks_start(struct seq_file *f, loff_t *pos)
 	__acquires(&blocked_lock_lock)
 {
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 8e5ad83..af84ad0 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -8,6 +8,7 @@
 #include <linux/security.h>
 #include <linux/file.h>
 #include <linux/seq_file.h>
+#include <linux/fs.h>
 
 #include <linux/proc_fs.h>
 
@@ -48,17 +49,23 @@ static int seq_show(struct seq_file *m, void *v)
 		put_files_struct(files);
 	}
 
-	if (!ret) {
-		seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
-			   (long long)file->f_pos, f_flags,
-			   real_mount(file->f_path.mnt)->mnt_id);
-		if (file->f_op->show_fdinfo)
-			file->f_op->show_fdinfo(m, file);
-		ret = seq_has_overflowed(m);
-		fput(file);
-	}
+	if (ret)
+		return ret;
 
-	return ret;
+	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
+		   (long long)file->f_pos, f_flags,
+		   real_mount(file->f_path.mnt)->mnt_id);
+
+	show_fd_locks(m, file, files);
+	if (seq_has_overflowed(m))
+		goto out;
+
+	if (file->f_op->show_fdinfo)
+		file->f_op->show_fdinfo(m, file);
+
+out:
+	fput(file);
+	return 0;
 }
 
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b4d71b5..ba0bd2a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1018,6 +1018,9 @@ extern void lease_get_mtime(struct inode *, struct timespec *time);
 extern int generic_setlease(struct file *, long, struct file_lock **, void **priv);
 extern int vfs_setlease(struct file *, long, struct file_lock **, void **);
 extern int lease_modify(struct file_lock *, int, struct list_head *);
+struct files_struct;
+extern void show_fd_locks(struct seq_file *f,
+			 struct file *filp, struct files_struct *files);
 #else /* !CONFIG_FILE_LOCKING */
 static inline int fcntl_getlk(struct file *file, unsigned int cmd,
 			      struct flock __user *user)
@@ -1154,6 +1157,10 @@ static inline int lease_modify(struct file_lock *fl, int arg,
 {
 	return -EINVAL;
 }
+
+struct files_struct;
+static inline void show_fd_locks(struct seq_file *f,
+			struct file *filp, struct files_struct *files) {}
 #endif /* !CONFIG_FILE_LOCKING */
 
 
-- 
2.1.0

