linux-kernel - Re: [PATCH 2/2] fs/locks: Remove fl_nspid and use fs-specific l

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: <9037C362-D6F7-4545-80C2-08B072005055@redhat.com>
Date:   Tue, 20 Jun 2017 10:03:42 -0400
From:   "Benjamin Coddington" <bcodding@...hat.com>
To:     "Jeff Layton" <jlayton@...chiereds.net>
Cc:     bfields@...ldses.org, "Alexander Viro" <viro@...iv.linux.org.uk>,
        linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        "open list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] fs/locks: Remove fl_nspid and use fs-specific l_pid
 for remote locks


On 19 Jun 2017, at 13:32, Jeff Layton wrote:

> On Mon, 2017-06-19 at 09:24 -0400, Benjamin Coddington wrote:
>> @@ -2041,16 +2034,46 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, 
>> unsigned int, cmd)
>>   */
>>  int vfs_test_lock(struct file *filp, struct file_lock *fl)
>>  {
>> -	if (filp->f_op->lock && is_remote_lock(filp))
>> +	if (filp->f_op->lock && is_remote_lock(filp)) {
>> +		fl->fl_flags |= FL_PID_PRIV;
>>  		return filp->f_op->lock(filp, F_GETLK, fl);
>> +	}
>>  	posix_test_lock(filp, fl);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(vfs_test_lock);
>>
>
> I think this looks wrong for NFS.

Oh yes, this is completely wrong..  It should be looking for fl_ops, 
which
would set the flag for lock managers.

> There are really two cases we're concerned with here:
>
> 1) the lock is held by a task on the client itself, in which case we
> probably want to report the pid as we would on a local fs.
>
> ...or...
>
> 2) the lock is held by another host entirely in which case the pid
> doesn't have any meaning. We probably ought to return something like 
> '-
> 1' as the pid (like we would for OFD locks).

I don't think we have f_op->lock() users that only set remote locks.  
For
NFS, the remote lock is always matched by a local lock.

> The problem for NFS is that you're setting the flag unconditionally
> there. It may very well be the case that we _want_ to translate the
> fl_pid according to the local namespace (i.e. if the lock is held by a
> task on the same host).
>
> I think what you want to do here is have the fs ->lock operation set
> that flag if the fl_pid should be used "as-is" instead of being
> translated.
>
> Most of the current lock operations can just set it early (to preserve
> the existing behavior), but NFS could be set up to set that flag if 
> the
> lock request goes to the server.

I think this is just a mistake.. I think we want to always translate all
local locks, unless the lock is placed by a lock manager.

I'll send a corrected version.

Ben