linux-kernel - Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible processes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Jun 2018 07:13:22 -0400
From:   "Benjamin Coddington" <bcodding@...hat.com>
To:     "Konstantin Khorenko" <khorenko@...tuozzo.com>,
        "Jeff Layton" <jlayton@...nel.org>
Cc:     "Kirill Gorkunov" <kgorkunov@...tuozzo.com>,
        "Andrey Vagin" <avagin@...tuozzo.com>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        "Alexander Viro" <viro@...iv.linux.org.uk>,
        "Vasily Averin" <vvs@...tuozzo.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] fs/lock: show locks info owned by dead/invisible
 processes



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@...hat.com>

to these two.

Ben