linux-kernel - Re: [PATCH 2/3] fs/locks: Remove fl

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:   Tue, 06 Jun 2017 16:41:29 -0400
From:   "Benjamin Coddington" <bcodding@...hat.com>
To:     "Jeff Layton" <jlayton@...hat.com>
Cc:     bfields@...ldses.org, "Alexander Viro" <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] fs/locks: Remove fl_nspid

On 6 Jun 2017, at 14:57, Benjamin Coddington wrote:

> On 6 Jun 2017, at 14:25, Jeff Layton wrote:
>
>> On Tue, 2017-06-06 at 14:00 -0400, Jeff Layton wrote:
>>> On Tue, 2017-06-06 at 13:19 -0400, Benjamin Coddington wrote:
>>>> Since commit c69899a17ca4 "NFSv4: Update of VFS byte range lock 
>>>> must be
>>>> atomic with the stateid update", NFSv4 has been inserting locks in 
>>>> rpciod
>>>> worker context.  The result is that the file_lock's fl_nspid is the
>>>> kworker's pid instead of the original userspace pid.
>>>>
>>>> The fl_nspid is only used to represent the namespaced virtual pid 
>>>> number
>>>> when displaying locks or returning from F_GETLK.  There's no reason 
>>>> to set
>>>> it for every inserted lock, since we can usually just look it up 
>>>> from
>>>> fl_pid.  So, instead of looking up and holding struct pid for every 
>>>> lock,
>>>> let's just look up the virtual pid number from fl_pid when it is 
>>>> needed.
>>>> That means we can remove fl_nspid entirely.
>>>>
>>>
>>> With this set, I think we ought to codify that the stored pid must 
>>> be
>>> relative
>>
>> ...to the init_pid_ns. Let's make that clear in the comments for
>> filesystem authors.
>
> OK, but I think you mean fl_pid should always be current->tgid or the 
> pid as
> it is in init_pid_ns.  We translate that pid into the virtual pid of 
> the
> process doing F_GETLK or reading /proc/locks.

And in that sense, we shouldn't have to add a comment, because the 
expected
behavior is the same as it is today -- namely that fl_pid is 
current->tgid,
or the real pid number, which is the pid number in init_pid_ns.

So, unless you feel strongly that this should be explained in a comment, 
I
think I'll resend this without additional comments after making the 
change
to use find_pid_ns().

Ben