linux-kernel - Re: [PATCH] fcntl: make F_GETOWN(EX) return 0 on dead owner task

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]

Message-ID: <88739f26-63b0-be1d-4e6d-def01633323e@virtuozzo.com>
Date:   Thu, 4 Feb 2021 00:35:42 +0300
From:   Pavel Tikhomirov <ptikhomirov@...tuozzo.com>
To:     Cyrill Gorcunov <gorcunov@...il.com>
Cc:     Jeff Layton <jlayton@...chiereds.net>,
        Jeff Layton <jlayton@...nel.org>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andrei Vagin <avagin@...il.com>
Subject: Re: [PATCH] fcntl: make F_GETOWN(EX) return 0 on dead owner task

On 2/3/21 10:32 PM, Cyrill Gorcunov wrote:
> On Wed, Feb 03, 2021 at 03:41:56PM +0300, Pavel Tikhomirov wrote:
>> Currently there is no way to differentiate the file with alive owner
>> from the file with dead owner but pid of the owner reused. That's why
>> CRIU can't actually know if it needs to restore file owner or not,
>> because if it restores owner but actual owner was dead, this can
>> introduce unexpected signals to the "false"-owner (which reused the
>> pid).
> 
> Hi! Thanks for the patch. You know I manage to forget the fowner internals.
> Could you please enlighten me -- when owner is set with some pid we do
> 
> f_setown_ex
>    __f_setown
>      f_modown
>        filp->f_owner.pid = get_pid(pid);
> 
> Thus pid get refcount incremented.

Hi, and yes you are right about refcount is held.

  Then the owner exits but refcounter
> should be still up and running and pid should not be reused, no? Or
> I miss something obvious?

AFAICS if pid is held only by 1) fowner refcount and by 2) single 
process (without threads, group and session for simplicity), on process 
exit we go through:

do_exit
   exit_notify
     release_task
       __exit_signal
         __unhash_process
           detach_pid
             __change_pid
               free_pid
                 idr_remove

So pid is removed from idr, and after that alloc_pid can reuse pid 
numbers even if old pid structure is still alive and is still held by 
fowner.

Also I've added criu-zdtm test which reproduces the problem:

https://src.openvz.org/projects/OVZ/repos/criu/commits/e25904c35dbc535f6837e55da58ca0f5a5caf4b3#test/zdtm/static/file_fown_reuse.c

Hope this answers your question, Thanks!

> 
> The patch itself looks ok on a first glance.
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.