Re: [PATCH] locks: Set fl_nspid at file_lock allocation

From: "Benjamin Coddington" <bcodding@redhat.com>
To: "Jeff Layton" <jlayton@poochiereds.net>
Cc: "Alexander Viro" <viro@zeniv.linux.org.uk>,
	bfields@fieldses.org, linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org
Subject: Re: [PATCH] locks: Set fl_nspid at file_lock allocation
Date: Fri, 19 May 2017 08:35:07 -0400	[thread overview]
Message-ID: <6ED42F9C-F290-451C-89A9-4A7C3FFDEFA6@redhat.com> (raw)
In-Reply-To: <1495140075.3956.13.camel@poochiereds.net>

On 18 May 2017, at 16:41, Jeff Layton wrote:

> On Thu, 2017-05-18 at 13:36 -0400, Benjamin Coddington wrote:
>>> On Thu, 2017-05-18 at 12:02 -0400, Benjamin Coddington wrote:
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index af2031a1fcff..959b3f93f4bd 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -249,7 +249,7 @@ locks_dump_ctx_list(struct list_head *list, char
>>>> *list_type)
>>>>  	struct file_lock *fl;
>>>>
>>>>  	list_for_each_entry(fl, list, fl_list) {
>>>> -		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n",
>>>> list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, fl->fl_pid);
>>>> +		pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n",
>>>> list_type, fl->fl_owner, fl->fl_flags, fl->fl_type,
>>>> pid_vnr(fl->fl_nspid));
>>>>  	}
>>>>  }
>>>>
>>>
>>> Probably should change the format to say fl_nspid=%u here, just to be
>>> clear. Might also want to keep fl_pid in there since the lock manager
>>> could set it.
>>
>> Yes, I thought about just spitting out both.  Let's do that.
>>
>>>> @@ -2074,7 +2075,7 @@ static int posix_lock_to_flock(struct flock
>>>> *flock, struct file_lock *fl)
>>>>  #if BITS_PER_LONG == 32
>>>>  static void posix_lock_to_flock64(struct flock64 *flock, struct
>>>> file_lock *fl)
>>>>  {
>>>> -	flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid;
>>>> +	flock->l_pid = IS_OFDLCK(fl) ? -1 : pid_vnr(fl->fl_nspid);
>>>
>>> What about the lock managers that _do_ set fl->fl_pid. With nfsv2/3,
>>> this is always going to give you back the pid of lockd, AFAICT.
>>
>> But isn't this really what you want?  If a local process wants to know
>> who
>> holds a conflicting lock, the fl_pid of a remote system is really pretty
>> useless.  Not only that, but there's no way for the local process to
>> know
>> when the pid is local or remote.  Better to be consistent and always
>> return
>> something that's useful.
>>
>
> The l_pid field in struct flock (and by extension fl_pid) is pretty
> poorly defined in the spec(s), especially when there is a remote host
> involved. Programs that rely on it are insane, of course...but Linux has
> always behaved this way.

But if it is completely useless today, then we can change it without
worrying about breaking it because it is already broken.  There's no
documentation anywhere that informs users of F_GETLK or /proc/locks that
l_pid is completely unreliable.

Do you know why linux hasn't picked up l_sysid?  I can't seem to find any
previous discussion about it.

> In the absence of a compelling reason to change it, I think we should
> keep the behavior in this respect as close as possible to what we have
> now.

I think the reason would be l_pid should at least have some consistent
meaning.  I think now that this patch probably shouldn't change it, but it
should be changed.

>>> Do we want to present the pid value that the client sent here instead
>>> in
>>> that case? Maybe the lm could set a fl_flag to indicate that the pid
>>> should be taken directly from fl_pid here? Then you could move the
>>> above
>>> logic to a static inline or something.
>>>
>>> Alternately, you could add a new lm_present_pid operation to lock
>>> managers to format the pid as they see fit.
>>
>> Either works to solve the problem, but I still think that F_GETLK and
>> /proc/locks should only return local pids.
>>
>>>> @@ -2322,6 +2325,7 @@ int fcntl_getlk64(struct file *filp, unsigned
>>>> int cmd, struct flock64 __user *l)
>>>>  	int error;
>>>>
>>>>  	error = -EFAULT;
>>>> +	file_lock.fl_nspid = get_pid(task_tgid(current));
>>>
>>> Might it be cleaner to create a FILE_LOCK(name) macro that does this
>>> on
>>> the stack (like LIST_HEAD()) ?
>>
>> Yes, it would.  I'll do it.
>>
>
> In some of those places it might not hurt to consider allocating and
> freeing a file_lock instead. file_lock is not exactly small (208 bytes
> on my latest build)...

Do you want that wrapped up with this patch?

Ben