linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
@ 2018-06-08 14:27 Konstantin Khorenko
  2018-06-08 14:27 ` [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns Konstantin Khorenko
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Konstantin Khorenko @ 2018-06-08 14:27 UTC (permalink / raw)
  To: Kirill Gorkunov, Andrey Vagin, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro
  Cc: Konstantin Khorenko, Vasily Averin, linux-fsdevel, linux-kernel

The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove fl_nspid
and use fs-specific l_pid for remote locks")
and now /proc/$PID/fdinfo/$FD does not show the info about the lock
* if the flock owner process is dead and its pid has been already freed
or
* if the lock owner is not visible in current pidns.

CRIU uses this interface to store locks info during dump and thus can break
on v4.13 and newer.

So let's show info about locks anyway in described cases (like it was before
9d5b86ac13c5), but show pid number saved in file_lock struct if we are in
init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with SID.

Reproducer:
process A       process A1      process A2
fork()--------->
exit()          open()
                flock()
                fork()--------->
                exit()          sleep()

Before the patch:
================
(root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
pos:    4
flags:  02100002
mnt_id: 257
lock:   (root@vz7)/:

After the patch:
===============
(root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
pos:    4
flags:  02100002
mnt_id: 295
lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF

===============
# cat flock1.c

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/file.h>
#include <fcntl.h>

int main(void)
{
        int fd;
        int err;
        pid_t child_pid;

        child_pid = fork();
        if (child_pid == -1)
                perror("fork failed");
        if (child_pid) {
                exit(0);
        }

        fd = open("/tmp/a", O_CREAT | O_RDWR);
        if (fd == -1)
                perror("Failed to open the file");

        err = flock(fd, LOCK_EX);
        if (err == -1)
                perror("flock failed");

        child_pid = fork();
        if (child_pid == -1)
                perror("fork failed");
        if (child_pid)
                exit(0);

        sleep(10000);

        return 0;
}

Konstantin Khorenko (2):
  fs/lock: skip lock owner pid translation in case we are in init_pid_ns
  fs/lock: show locks taken by processes from another pidns

 fs/locks.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.15.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
@ 2018-06-08 14:27 ` Konstantin Khorenko
  2018-06-12  4:35   ` Andrey Vagin
  2018-06-08 14:27 ` [PATCH 2/2] fs/lock: show locks taken by processes from another pidns Konstantin Khorenko
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Konstantin Khorenko @ 2018-06-08 14:27 UTC (permalink / raw)
  To: Kirill Gorkunov, Andrey Vagin, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro
  Cc: Konstantin Khorenko, Vasily Averin, linux-fsdevel, linux-kernel

If the flock owner process is dead and its pid has been already freed,
pid translation won't work, but we still want to show flock owner pid
number when expecting /proc/$PID/fdinfo/$FD in init pidns.

Reproducer:
process A	process A1	process A2
fork()--------->
exit()		open()
		flock()
		fork()--------->
		exit()		sleep()

Before the patch:
================
(root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
pos:    4
flags:  02100002
mnt_id: 257
lock:   (root@vz7)/:

After the patch:
===============
(root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
pos:    4
flags:  02100002
mnt_id: 295
lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF

Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
 fs/locks.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/locks.c b/fs/locks.c
index 05e211be8684..bfee5b7f2862 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2072,6 +2072,13 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
 		return -1;
 	if (IS_REMOTELCK(fl))
 		return fl->fl_pid;
+	/*
+	 * If the flock owner process is dead and its pid has been already
+	 * freed, the translation below won't work, but we still want to show
+	 * flock owner pid number in init pidns.
+	 */
+	if (ns == &init_pid_ns)
+		return (pid_t)fl->fl_pid;
 
 	rcu_read_lock();
 	pid = find_pid_ns(fl->fl_pid, &init_pid_ns);
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
  2018-06-08 14:27 ` [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns Konstantin Khorenko
@ 2018-06-08 14:27 ` Konstantin Khorenko
  2018-06-12  4:53   ` Andrey Vagin
  2018-06-14 11:00   ` Jeff Layton
  2018-06-08 16:02 ` [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Cyrill Gorcunov
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 12+ messages in thread
From: Konstantin Khorenko @ 2018-06-08 14:27 UTC (permalink / raw)
  To: Kirill Gorkunov, Andrey Vagin, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro
  Cc: Konstantin Khorenko, Vasily Averin, linux-fsdevel, linux-kernel

Currently if we face a lock taken by a process invisible in the current
pidns we skip the lock completely, but this

1) makes the output not that nice
    (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
    pos:    4
    flags:  02100002
    mnt_id: 257
    lock:   (root@vz7)/:

2) makes it more difficult to debug issues with leaked flocks
   if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file

Let's show information about such locks again as previously, but
show zero in the owner pid field.

After the patch:
===============
(root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
pos:    4
flags:  02100002
mnt_id: 295
lock:   1: FLOCK  ADVISORY  WRITE 0 b6:f8a61:529946 0 EOF

Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
 fs/locks.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index bfee5b7f2862..e533623e2e99 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 
 	fl_pid = locks_translate_pid(fl, 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
-	 * called from __show_fd_info - skip lock entirely
+	 * If lock owner is dead (and pid is freed) or not visible in current
+	 * pidns, zero is shown as a pid value. Check lock info from
+	 * init_pid_ns to get saved lock pid value.
 	 */
-	if (fl_pid == 0)
-		return;
 
 	if (fl->fl_file != NULL)
 		inode = locks_inode(fl->fl_file);
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
  2018-06-08 14:27 ` [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns Konstantin Khorenko
  2018-06-08 14:27 ` [PATCH 2/2] fs/lock: show locks taken by processes from another pidns Konstantin Khorenko
@ 2018-06-08 16:02 ` Cyrill Gorcunov
  2018-06-14 10:41 ` Jeff Layton
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Cyrill Gorcunov @ 2018-06-08 16:02 UTC (permalink / raw)
  To: Konstantin Khorenko
  Cc: Andrey Vagin, Benjamin Coddington, Jeff Layton, J. Bruce Fields,
	Alexander Viro, Vasily Averin, linux-fsdevel, linux-kernel

On Fri, Jun 08, 2018 at 05:27:10PM +0300, Konstantin Khorenko wrote:
> The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove fl_nspid
> and use fs-specific l_pid for remote locks")
> and now /proc/$PID/fdinfo/$FD does not show the info about the lock
> * if the flock owner process is dead and its pid has been already freed
> or
> * if the lock owner is not visible in current pidns.
> 
> CRIU uses this interface to store locks info during dump and thus can break
> on v4.13 and newer.

Indeed, it should cause problems with procfs parsing. We should add a test
for such scenarions, and since we're running tests by linux-next all the time
we would have this issue catched already.

The series looks ok to me, but I'm not flock expert so better wait for more
trustworthy opinion.

> 
> So let's show info about locks anyway in described cases (like it was before
> 9d5b86ac13c5), but show pid number saved in file_lock struct if we are in
> init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with SID.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns
  2018-06-08 14:27 ` [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns Konstantin Khorenko
@ 2018-06-12  4:35   ` Andrey Vagin
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Vagin @ 2018-06-12  4:35 UTC (permalink / raw)
  To: Konstantin Khorenko
  Cc: Kirill Gorkunov, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro, Vasily Averin, linux-fsdevel,
	linux-kernel

On Fri, Jun 08, 2018 at 05:27:11PM +0300, Konstantin Khorenko wrote:
> If the flock owner process is dead and its pid has been already freed,
> pid translation won't work, but we still want to show flock owner pid
> number when expecting /proc/$PID/fdinfo/$FD in init pidns.
> 
> Reproducer:
> process A	process A1	process A2
> fork()--------->
> exit()		open()
> 		flock()
> 		fork()--------->
> 		exit()		sleep()
> 
> Before the patch:
> ================
> (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 257
> lock:   (root@vz7)/:
> 
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF
> 

Acked-by: Andrey Vagin <avagin@openvz.org>

> Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
>  fs/locks.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 05e211be8684..bfee5b7f2862 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2072,6 +2072,13 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
>  		return -1;
>  	if (IS_REMOTELCK(fl))
>  		return fl->fl_pid;
> +	/*
> +	 * If the flock owner process is dead and its pid has been already
> +	 * freed, the translation below won't work, but we still want to show
> +	 * flock owner pid number in init pidns.
> +	 */
> +	if (ns == &init_pid_ns)
> +		return (pid_t)fl->fl_pid;
>  
>  	rcu_read_lock();
>  	pid = find_pid_ns(fl->fl_pid, &init_pid_ns);
> -- 
> 2.15.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
  2018-06-08 14:27 ` [PATCH 2/2] fs/lock: show locks taken by processes from another pidns Konstantin Khorenko
@ 2018-06-12  4:53   ` Andrey Vagin
  2018-06-14 11:00   ` Jeff Layton
  1 sibling, 0 replies; 12+ messages in thread
From: Andrey Vagin @ 2018-06-12  4:53 UTC (permalink / raw)
  To: Konstantin Khorenko
  Cc: Kirill Gorkunov, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro, Vasily Averin, linux-fsdevel,
	linux-kernel

On Fri, Jun 08, 2018 at 05:27:12PM +0300, Konstantin Khorenko wrote:
> Currently if we face a lock taken by a process invisible in the current
> pidns we skip the lock completely, but this
> 
> 1) makes the output not that nice
>     (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
>     pos:    4
>     flags:  02100002
>     mnt_id: 257
>     lock:   (root@vz7)/:
> 
> 2) makes it more difficult to debug issues with leaked flocks
>    if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file

3) breaks the CRIU project. criu reads fdinfo to dump file locks.

> 
> Let's show information about such locks again as previously, but
> show zero in the owner pid field.
> 
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE 0 b6:f8a61:529946 0 EOF
>

I think initially this was broken in this commit:

Fixes: d67fd44f697d ("locks: Filter /proc/locks output on proc pid ns")


Acked-by: Andrei Vagin <avagin@openvz.org>

Thanks,
Andrei

> Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
>  fs/locks.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index bfee5b7f2862..e533623e2e99 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  
>  	fl_pid = locks_translate_pid(fl, 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
> -	 * called from __show_fd_info - skip lock entirely
> +	 * If lock owner is dead (and pid is freed) or not visible in current
> +	 * pidns, zero is shown as a pid value. Check lock info from
> +	 * init_pid_ns to get saved lock pid value.
>  	 */
> -	if (fl_pid == 0)
> -		return;
>  
>  	if (fl->fl_file != NULL)
>  		inode = locks_inode(fl->fl_file);
> -- 
> 2.15.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
                   ` (2 preceding siblings ...)
  2018-06-08 16:02 ` [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Cyrill Gorcunov
@ 2018-06-14 10:41 ` Jeff Layton
  2018-06-14 11:13 ` Benjamin Coddington
  2018-08-09  7:16 ` Murphy Zhou
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2018-06-14 10:41 UTC (permalink / raw)
  To: Konstantin Khorenko, Kirill Gorkunov, Andrey Vagin,
	Benjamin Coddington, J. Bruce Fields, Alexander Viro
  Cc: Vasily Averin, linux-fsdevel, linux-kernel

On Fri, 2018-06-08 at 17:27 +0300, Konstantin Khorenko wrote:
> The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove fl_nspid
> and use fs-specific l_pid for remote locks")
> and now /proc/$PID/fdinfo/$FD does not show the info about the lock
> * if the flock owner process is dead and its pid has been already freed
> or
> * if the lock owner is not visible in current pidns.
> 
> CRIU uses this interface to store locks info during dump and thus can break
> on v4.13 and newer.
> 
> So let's show info about locks anyway in described cases (like it was before
> 9d5b86ac13c5), but show pid number saved in file_lock struct if we are in
> init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with SID.
> 
> Reproducer:
> process A       process A1      process A2
> fork()--------->
> exit()          open()
>                 flock()
>                 fork()--------->
>                 exit()          sleep()
> 
> Before the patch:
> ================
> (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 257
> lock:   (root@vz7)/:
> 
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF
> 
> ===============
> # cat flock1.c
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/file.h>
> #include <fcntl.h>
> 
> int main(void)
> {
>         int fd;
>         int err;
>         pid_t child_pid;
> 
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid) {
>                 exit(0);
>         }
> 
>         fd = open("/tmp/a", O_CREAT | O_RDWR);
>         if (fd == -1)
>                 perror("Failed to open the file");
> 
>         err = flock(fd, LOCK_EX);
>         if (err == -1)
>                 perror("flock failed");
> 
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid)
>                 exit(0);
> 
>         sleep(10000);
> 
>         return 0;
> }
> 
> Konstantin Khorenko (2):
>   fs/lock: skip lock owner pid translation in case we are in init_pid_ns
>   fs/lock: show locks taken by processes from another pidns
> 
>  fs/locks.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

These look fine to me. I'll plan to pick them up for v4.19 unless anyone
has objections.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
  2018-06-08 14:27 ` [PATCH 2/2] fs/lock: show locks taken by processes from another pidns Konstantin Khorenko
  2018-06-12  4:53   ` Andrey Vagin
@ 2018-06-14 11:00   ` Jeff Layton
  2018-06-19 20:25     ` Andrey Vagin
  1 sibling, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2018-06-14 11:00 UTC (permalink / raw)
  To: Konstantin Khorenko, Kirill Gorkunov, Andrey Vagin,
	Benjamin Coddington, J. Bruce Fields, Alexander Viro
  Cc: Vasily Averin, linux-fsdevel, linux-kernel, Nikolay Borisov

On Fri, 2018-06-08 at 17:27 +0300, Konstantin Khorenko wrote:
> Currently if we face a lock taken by a process invisible in the current
> pidns we skip the lock completely, but this
> 
> 1) makes the output not that nice
>     (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
>     pos:    4
>     flags:  02100002
>     mnt_id: 257
>     lock:   (root@vz7)/:
> 
> 2) makes it more difficult to debug issues with leaked flocks
>    if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file
> 
> Let's show information about such locks again as previously, but
> show zero in the owner pid field.
> 
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE 0 b6:f8a61:529946 0 EOF
> 
> Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
>  fs/locks.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index bfee5b7f2862..e533623e2e99 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  
>  	fl_pid = locks_translate_pid(fl, 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
> -	 * called from __show_fd_info - skip lock entirely
> +	 * If lock owner is dead (and pid is freed) or not visible in current
> +	 * pidns, zero is shown as a pid value. Check lock info from
> +	 * init_pid_ns to get saved lock pid value.
>  	 */
> -	if (fl_pid == 0)
> -		return;
>  
>  	if (fl->fl_file != NULL)
>  		inode = locks_inode(fl->fl_file);

(cc'ing Nickolay)

As Andrey points out, this behavior was originally added in commit
d67fd44f697d to address performance issues when there are a lot of locks
held by tasks in other namespaces.

Will allowing this code to show these again cause a problem there?
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
                   ` (3 preceding siblings ...)
  2018-06-14 10:41 ` Jeff Layton
@ 2018-06-14 11:13 ` Benjamin Coddington
  2018-08-09  7:16 ` Murphy Zhou
  5 siblings, 0 replies; 12+ messages in thread
From: Benjamin Coddington @ 2018-06-14 11:13 UTC (permalink / raw)
  To: Konstantin Khorenko, Jeff Layton
  Cc: Kirill Gorkunov, Andrey Vagin, J. Bruce Fields, Alexander Viro,
	Vasily Averin, linux-fsdevel, linux-kernel



On 8 Jun 2018, at 10:27, Konstantin Khorenko wrote:

> The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove 
> fl_nspid
> and use fs-specific l_pid for remote locks")
> and now /proc/$PID/fdinfo/$FD does not show the info about the lock
> * if the flock owner process is dead and its pid has been already 
> freed
> or
> * if the lock owner is not visible in current pidns.
>
> CRIU uses this interface to store locks info during dump and thus can 
> break
> on v4.13 and newer.
>
> So let's show info about locks anyway in described cases (like it was 
> before
> 9d5b86ac13c5), but show pid number saved in file_lock struct if we are 
> in
> init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with 
> SID.
>
> Reproducer:
> process A       process A1      process A2
> fork()--------->
> exit()          open()
>                 flock()
>                 fork()--------->
>                 exit()          sleep()
>
> Before the patch:
> ================
> (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 257
> lock:   (root@vz7)/:
>
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF
>
> ===============
> # cat flock1.c
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/file.h>
> #include <fcntl.h>
>
> int main(void)
> {
>         int fd;
>         int err;
>         pid_t child_pid;
>
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid) {
>                 exit(0);
>         }
>
>         fd = open("/tmp/a", O_CREAT | O_RDWR);
>         if (fd == -1)
>                 perror("Failed to open the file");
>
>         err = flock(fd, LOCK_EX);
>         if (err == -1)
>                 perror("flock failed");
>
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid)
>                 exit(0);
>
>         sleep(10000);
>
>         return 0;
> }
>
> Konstantin Khorenko (2):
>   fs/lock: skip lock owner pid translation in case we are in 
> init_pid_ns
>   fs/lock: show locks taken by processes from another pidns


These look good to me too.  It makes sense that we treat pid numbers out 
of
our namespace in the same way we'd treat remote pids, by returning 0 
instead
of skipping their display altogether.  You can add my

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

to these two.

Ben

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] fs/lock: show locks taken by processes from another pidns
  2018-06-14 11:00   ` Jeff Layton
@ 2018-06-19 20:25     ` Andrey Vagin
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Vagin @ 2018-06-19 20:25 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Konstantin Khorenko, Kirill Gorkunov, Benjamin Coddington,
	J. Bruce Fields, Alexander Viro, Vasily Averin, linux-fsdevel,
	linux-kernel, Nikolay Borisov

On Thu, Jun 14, 2018 at 07:00:07AM -0400, Jeff Layton wrote:
> On Fri, 2018-06-08 at 17:27 +0300, Konstantin Khorenko wrote:
> > Currently if we face a lock taken by a process invisible in the current
> > pidns we skip the lock completely, but this
> > 
> > 1) makes the output not that nice
> >     (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
> >     pos:    4
> >     flags:  02100002
> >     mnt_id: 257
> >     lock:   (root@vz7)/:
> > 
> > 2) makes it more difficult to debug issues with leaked flocks
> >    if you get error on lock, but don't see any locks in /proc/$id/fdinfo/$file
> > 
> > Let's show information about such locks again as previously, but
> > show zero in the owner pid field.
> > 
> > After the patch:
> > ===============
> > (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> > pos:    4
> > flags:  02100002
> > mnt_id: 295
> > lock:   1: FLOCK  ADVISORY  WRITE 0 b6:f8a61:529946 0 EOF
> > 
> > Fixes: 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks")
> > Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> > ---
> >  fs/locks.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index bfee5b7f2862..e533623e2e99 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2633,12 +2633,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> >  
> >  	fl_pid = locks_translate_pid(fl, 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
> > -	 * called from __show_fd_info - skip lock entirely
> > +	 * If lock owner is dead (and pid is freed) or not visible in current
> > +	 * pidns, zero is shown as a pid value. Check lock info from
> > +	 * init_pid_ns to get saved lock pid value.
> >  	 */
> > -	if (fl_pid == 0)
> > -		return;
> >  
> >  	if (fl->fl_file != NULL)
> >  		inode = locks_inode(fl->fl_file);
> 
> (cc'ing Nickolay)
> 
> As Andrey points out, this behavior was originally added in commit
> d67fd44f697d to address performance issues when there are a lot of locks
> held by tasks in other namespaces.
> 
> Will allowing this code to show these again cause a problem there?

No, it will not. The content of /proc/locks is still filtered. As for
fdinfo, it shows locks for one file descriptor, there will not be a lot of
locks. And fdinfo was designed to show all locks for a file descriptor,
it doesn't matter in what pidns they were taken.

Thanks,
Andrei

> -- 
> Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
  2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
                   ` (4 preceding siblings ...)
  2018-06-14 11:13 ` Benjamin Coddington
@ 2018-08-09  7:16 ` Murphy Zhou
  2018-08-09  8:20   ` Konstantin Khorenko
  5 siblings, 1 reply; 12+ messages in thread
From: Murphy Zhou @ 2018-08-09  7:16 UTC (permalink / raw)
  To: Konstantin Khorenko
  Cc: Kirill Gorkunov, Andrey Vagin, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro, Vasily Averin, Linux-Fsdevel,
	linux-kernel

Hi,

Looks like this missed v4.18 ?

Thanks,
Murphy

On Fri, Jun 8, 2018 at 10:27 PM, Konstantin Khorenko
<khorenko@virtuozzo.com> wrote:
> The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove fl_nspid
> and use fs-specific l_pid for remote locks")
> and now /proc/$PID/fdinfo/$FD does not show the info about the lock
> * if the flock owner process is dead and its pid has been already freed
> or
> * if the lock owner is not visible in current pidns.
>
> CRIU uses this interface to store locks info during dump and thus can break
> on v4.13 and newer.
>
> So let's show info about locks anyway in described cases (like it was before
> 9d5b86ac13c5), but show pid number saved in file_lock struct if we are in
> init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with SID.
>
> Reproducer:
> process A       process A1      process A2
> fork()--------->
> exit()          open()
>                 flock()
>                 fork()--------->
>                 exit()          sleep()
>
> Before the patch:
> ================
> (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 257
> lock:   (root@vz7)/:
>
> After the patch:
> ===============
> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
> pos:    4
> flags:  02100002
> mnt_id: 295
> lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF
>
> ===============
> # cat flock1.c
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/file.h>
> #include <fcntl.h>
>
> int main(void)
> {
>         int fd;
>         int err;
>         pid_t child_pid;
>
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid) {
>                 exit(0);
>         }
>
>         fd = open("/tmp/a", O_CREAT | O_RDWR);
>         if (fd == -1)
>                 perror("Failed to open the file");
>
>         err = flock(fd, LOCK_EX);
>         if (err == -1)
>                 perror("flock failed");
>
>         child_pid = fork();
>         if (child_pid == -1)
>                 perror("fork failed");
>         if (child_pid)
>                 exit(0);
>
>         sleep(10000);
>
>         return 0;
> }
>
> Konstantin Khorenko (2):
>   fs/lock: skip lock owner pid translation in case we are in init_pid_ns
>   fs/lock: show locks taken by processes from another pidns
>
>  fs/locks.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> --
> 2.15.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes
  2018-08-09  7:16 ` Murphy Zhou
@ 2018-08-09  8:20   ` Konstantin Khorenko
  0 siblings, 0 replies; 12+ messages in thread
From: Konstantin Khorenko @ 2018-08-09  8:20 UTC (permalink / raw)
  To: Murphy Zhou
  Cc: Kirill Gorkunov, Andrey Vagin, Benjamin Coddington, Jeff Layton,
	J. Bruce Fields, Alexander Viro, Vasily Averin, Linux-Fsdevel,
	linux-kernel

On 08/09/2018 10:16 AM, Murphy Zhou wrote:
> Hi,
>
> Looks like this missed v4.18 ?

Hi Murphy,

yes, Jeff planned to push those patches into 4.19 and they are in "linux-next" now,
but not in 4.18 "master" currently.

On 06/14/2018 01:41 PM, Jeff Layton wrote:
 > These look fine to me. I'll plan to pick them up for v4.19 unless anyone
 > has objections.

linux-next:
1cf8e5de4055 ("fs/lock: show locks taken by processes from another pidns")
826d7bc9f013 ("fs/lock: skip lock owner pid translation in case we are in init_pid_ns")

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

> On Fri, Jun 8, 2018 at 10:27 PM, Konstantin Khorenko
> <khorenko@virtuozzo.com> wrote:
>> The behavior has been changed after 9d5b86ac13c5 ("fs/locks: Remove fl_nspid
>> and use fs-specific l_pid for remote locks")
>> and now /proc/$PID/fdinfo/$FD does not show the info about the lock
>> * if the flock owner process is dead and its pid has been already freed
>> or
>> * if the lock owner is not visible in current pidns.
>>
>> CRIU uses this interface to store locks info during dump and thus can break
>> on v4.13 and newer.
>>
>> So let's show info about locks anyway in described cases (like it was before
>> 9d5b86ac13c5), but show pid number saved in file_lock struct if we are in
>> init_pid_ns (patch 1) or just zero otherwise (patch 2) like we do with SID.
>>
>> Reproducer:
>> process A       process A1      process A2
>> fork()--------->
>> exit()          open()
>>                 flock()
>>                 fork()--------->
>>                 exit()          sleep()
>>
>> Before the patch:
>> ================
>> (root@vz7)/: cat /proc/${PID_A2}/fdinfo/3
>> pos:    4
>> flags:  02100002
>> mnt_id: 257
>> lock:   (root@vz7)/:
>>
>> After the patch:
>> ===============
>> (root@vz7)/:cat /proc/${PID_A2}/fdinfo/3
>> pos:    4
>> flags:  02100002
>> mnt_id: 295
>> lock:   1: FLOCK  ADVISORY  WRITE ${PID_A1} b6:f8a61:529946 0 EOF
>>
>> ===============
>> # cat flock1.c
>>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <sys/file.h>
>> #include <fcntl.h>
>>
>> int main(void)
>> {
>>         int fd;
>>         int err;
>>         pid_t child_pid;
>>
>>         child_pid = fork();
>>         if (child_pid == -1)
>>                 perror("fork failed");
>>         if (child_pid) {
>>                 exit(0);
>>         }
>>
>>         fd = open("/tmp/a", O_CREAT | O_RDWR);
>>         if (fd == -1)
>>                 perror("Failed to open the file");
>>
>>         err = flock(fd, LOCK_EX);
>>         if (err == -1)
>>                 perror("flock failed");
>>
>>         child_pid = fork();
>>         if (child_pid == -1)
>>                 perror("fork failed");
>>         if (child_pid)
>>                 exit(0);
>>
>>         sleep(10000);
>>
>>         return 0;
>> }
>>
>> Konstantin Khorenko (2):
>>   fs/lock: skip lock owner pid translation in case we are in init_pid_ns
>>   fs/lock: show locks taken by processes from another pidns
>>
>>  fs/locks.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> --
>> 2.15.1
>>
> .
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-08-09 10:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-08 14:27 [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Konstantin Khorenko
2018-06-08 14:27 ` [PATCH 1/2] fs/lock: skip lock owner pid translation in case we are in init_pid_ns Konstantin Khorenko
2018-06-12  4:35   ` Andrey Vagin
2018-06-08 14:27 ` [PATCH 2/2] fs/lock: show locks taken by processes from another pidns Konstantin Khorenko
2018-06-12  4:53   ` Andrey Vagin
2018-06-14 11:00   ` Jeff Layton
2018-06-19 20:25     ` Andrey Vagin
2018-06-08 16:02 ` [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes Cyrill Gorcunov
2018-06-14 10:41 ` Jeff Layton
2018-06-14 11:13 ` Benjamin Coddington
2018-08-09  7:16 ` Murphy Zhou
2018-08-09  8:20   ` Konstantin Khorenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).