[PATCH 0/3] RFC: F_OFD_GETLK should provide more info

[PATCH 0/3] RFC: F_OFD_GETLK should provide more info

[Stas Sergeev]

This patch-set implements 2 small extensions to the current F_OFD_GETLK,
allowing it to gather more information than it currently returns.

First extension allows to use F_UNLCK on query, which currently returns
EINVAL. Instead it can be used to query the locks on a particular fd -
something that is not currently possible. The basic idea is that on
F_OFD_GETLK, F_UNLCK would "conflict" with (or query) any types of the
lock on the same fd, and ignore any locks on other fds.

Use-cases:

1. CRIU-alike scenario when you want to read the locking info from an
fd for the later reconstruction. This can now be done by setting
l_start and l_len to 0 to cover entire file range, and do F_OFD_GETLK.
In the loop you need to advance l_start past the returned lock ranges,
to eventually collect all locked ranges.

2. Implementing the lock checking/enforcing policy.
Say you want to implement an "auditor" module in your program,
that checks that the I/O is done only after the proper locking is
applied on a file region. In this case you need to know if the
particular region is locked on that fd, and if so - with what type
of the lock. If you would do that currently (without this extension)
then you can only check for the write locks, and for that you need to
probe the lock on your fd and then open the same file via nother fd and
probe there. That way you can identify the write lock on a particular
fd, but such trick is non-atomic and complex. As for finding out the
read lock on a particular fd - impossible.
This extension allows to do such queries without any extra efforts.

3. Implementing the mandatory locking policy.
Suppose you want to make a policy where the write lock inhibits any
unlocked readers and writers. Currently you need to check if the
write lock is present on some other fd, and if it is not there - allow
the I/O operation. But because the write lock can appear at any moment,
you need to do that under some global lock, which can be released only
when the I/O operation is finished.
With the proposed extension you can instead just check the write lock
on your own fd first, and if it is there - allow the I/O operation on
that fd without using any global lock. Only if there is no write lock
on this fd, then you need to take global lock and check for a write
lock on other fds.


The second patch implements another extension.
Currently F_OFD_GETLK returns -1 in the l_pid member.
This patch removes the code that writes -1 there, so that the proper
pid is returned. I am not sure why it was decided to deliberately hide
the owner's pid. It may be needed in case you want to send some
message to the offending locker, like eg SIGKILL.


The third patch adds a test-case for OFD locks.
It tests both the generic things and the proposed extensions.

Stas Sergeev (3):
  fs/locks: F_UNLCK extension for F_OFD_GETLK
  fd/locks: allow get the lock owner by F_OFD_GETLK
  selftests: add OFD lock tests

 fs/locks.c                                 |  25 +++-
 tools/testing/selftests/locking/Makefile   |   2 +
 tools/testing/selftests/locking/ofdlocks.c | 135 +++++++++++++++++++++
 3 files changed, 157 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/locking/ofdlocks.c

CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Shuah Khan <shuah@kernel.org>
CC: linux-kselftest@vger.kernel.org

-- 
2.39.2

[PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

[Stas Sergeev]

Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
The proposed extension allows to use it for getting the lock
information from the particular fd.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org

---
 fs/locks.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..210766007e63 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
 	return locks_conflict(caller_fl, sys_fl);
 }
 
+/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
+ * path so checks for additional GETLK-specific things like F_UNLCK.
+ */
+static bool posix_test_locks_conflict(struct file_lock *caller_fl,
+				      struct file_lock *sys_fl)
+{
+	/* F_UNLCK checks any locks on the same fd. */
+	if (caller_fl->fl_type == F_UNLCK) {
+		if (!posix_same_owner(caller_fl, sys_fl))
+			return false;
+		return locks_overlap(caller_fl, sys_fl);
+	}
+	return posix_locks_conflict(caller_fl, sys_fl);
+}
+
 /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
  * checking before calling the locks_conflict().
  */
@@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
 retry:
 	spin_lock(&ctx->flc_lock);
 	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
-		if (!posix_locks_conflict(fl, cfl))
+		if (!posix_test_locks_conflict(fl, cfl))
 			continue;
 		if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
 			&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
@@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
 	if (fl == NULL)
 		return -ENOMEM;
 	error = -EINVAL;
-	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+			&& flock->l_type != F_WRLCK)
 		goto out;
 
 	error = flock_to_posix_lock(filp, fl, flock);
@@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
 		return -ENOMEM;
 
 	error = -EINVAL;
-	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+			&& flock->l_type != F_WRLCK)
 		goto out;
 
 	error = flock64_to_posix_lock(filp, fl, flock);
-- 
2.39.2

[PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Stas Sergeev]

Currently F_OFD_GETLK sets the pid of the lock owner to -1.
Remove such behavior to allow getting the proper owner's pid.
This may be helpful when you want to send some message (like SIGKILL)
to the offending locker.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org

---
 fs/locks.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 210766007e63..ee265e166542 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
 	pid_t vnr;
 	struct pid *pid;
 
-	if (IS_OFDLCK(fl))
-		return -1;
 	if (IS_REMOTELCK(fl))
 		return fl->fl_pid;
 	/*
-- 
2.39.2

[PATCH 3/3] selftests: add OFD lock tests

[Stas Sergeev]

Test the basic locking stuff on 2 fds: multiple read locks,
conflicts between read and write locks, use of len==0 for queries.
Also test for pid and F_UNLCK F_OFD_GETLK extensions.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Shuah Khan <shuah@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: linux-kselftest@vger.kernel.org
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org

---
 tools/testing/selftests/locking/Makefile   |   2 +
 tools/testing/selftests/locking/ofdlocks.c | 138 +++++++++++++++++++++
 2 files changed, 140 insertions(+)
 create mode 100644 tools/testing/selftests/locking/ofdlocks.c

diff --git a/tools/testing/selftests/locking/Makefile b/tools/testing/selftests/locking/Makefile
index 6e7761ab3536..a83ced1626de 100644
--- a/tools/testing/selftests/locking/Makefile
+++ b/tools/testing/selftests/locking/Makefile
@@ -7,4 +7,6 @@ all:
 
 TEST_PROGS := ww_mutex.sh
 
+TEST_GEN_PROGS := ofdlocks
+
 include ../lib.mk
diff --git a/tools/testing/selftests/locking/ofdlocks.c b/tools/testing/selftests/locking/ofdlocks.c
new file mode 100644
index 000000000000..1cff350e2c81
--- /dev/null
+++ b/tools/testing/selftests/locking/ofdlocks.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <assert.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include "../kselftest.h"
+
+static int lock_set(int fd, struct flock *fl)
+{
+	int ret;
+
+	fl->l_pid = 0;		// needed for OFD locks
+	fl->l_whence = SEEK_SET;
+	ret = fcntl(fd, F_OFD_SETLK, fl);
+	if (ret)
+		perror("fcntl()");
+	return ret;
+}
+
+static int lock_get(int fd, struct flock *fl)
+{
+	int ret;
+
+	fl->l_pid = 0;		// needed for OFD locks
+	fl->l_whence = SEEK_SET;
+	ret = fcntl(fd, F_OFD_GETLK, fl);
+	if (ret)
+		perror("fcntl()");
+	return ret;
+}
+
+int main(void)
+{
+	int rc;
+	struct flock fl, fl2;
+	int fd = open("/tmp/aa", O_RDWR | O_CREAT | O_EXCL, 0600);
+	int fd2 = open("/tmp/aa", O_RDONLY);
+
+	unlink("aa");
+	assert(fd != -1);
+	assert(fd2 != -1);
+	ksft_print_msg("[INFO] opened fds %i %i\n", fd, fd2);
+
+	/* Set some read lock */
+	fl.l_type = F_RDLCK;
+	fl.l_start = 5;
+	fl.l_len = 3;
+	rc = lock_set(fd, &fl);
+	if (rc == 0) {
+		ksft_print_msg
+		    ("[SUCCESS] set OFD read lock on first fd\n");
+	} else {
+		ksft_print_msg("[FAIL] to set OFD read lock on first fd\n");
+		return -1;
+	}
+	/* Make sure read locks do not conflict on different fds. */
+	fl.l_type = F_RDLCK;
+	fl.l_start = 5;
+	fl.l_len = 1;
+	rc = lock_get(fd2, &fl);
+	if (rc != 0)
+		return -1;
+	if (fl.l_type != F_UNLCK) {
+		ksft_print_msg("[FAIL] read locks conflicted\n");
+		return -1;
+	}
+	/* Make sure read/write locks do conflict on different fds. */
+	fl.l_type = F_WRLCK;
+	fl.l_start = 5;
+	fl.l_len = 1;
+	rc = lock_get(fd2, &fl);
+	if (rc != 0)
+		return -1;
+	if (fl.l_type != F_UNLCK) {
+		ksft_print_msg
+		    ("[SUCCESS] read and write locks conflicted\n");
+	} else {
+		ksft_print_msg
+		    ("[SUCCESS] read and write locks not conflicted\n");
+		return -1;
+	}
+	/* Get info about the lock on first fd. */
+	fl.l_type = F_UNLCK;
+	fl.l_start = 5;
+	fl.l_len = 1;
+	rc = lock_get(fd, &fl);
+	if (rc != 0) {
+		ksft_print_msg
+		    ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
+		return -1;
+	}
+	if (fl.l_type != F_UNLCK) {
+		if (fl.l_pid != getpid()) {
+			ksft_print_msg
+			    ("[FAIL] F_OFD_GETLK does not return pid, %i\n",
+			    fl.l_pid);
+			return -1;
+		}
+		ksft_print_msg
+		    ("[SUCCESS] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
+		     fl.l_type, fl.l_pid, fl.l_len);
+	} else {
+		ksft_print_msg
+		    ("[FAIL] F_OFD_GETLK with F_UNLCK did not return lock info\n");
+		return -1;
+	}
+	/* Try the same but by locking everything by len==0. */
+	fl2.l_type = F_UNLCK;
+	fl2.l_start = 0;
+	fl2.l_len = 0;
+	rc = lock_get(fd, &fl2);
+	if (rc != 0) {
+		ksft_print_msg
+		    ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
+		return -1;
+	}
+	if (memcmp(&fl, &fl2, sizeof(fl))) {
+		ksft_print_msg
+		    ("[FAIL] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
+		     fl.l_type, fl.l_pid, fl.l_len);
+		return -1;
+	}
+	ksft_print_msg("[SUCCESS] F_UNLCK with len==0 returned the same\n");
+	/* Get info about the lock on second fd - no locks on it. */
+	fl.l_type = F_UNLCK;
+	fl.l_start = 0;
+	fl.l_len = 0;
+	lock_get(fd2, &fl);
+	if (fl.l_type != F_UNLCK) {
+		ksft_print_msg
+		    ("[FAIL] F_OFD_GETLK with F_UNLCK return lock info from another fd\n");
+		return -1;
+	}
+	return 0;
+}
-- 
2.39.2

Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
> The proposed extension allows to use it for getting the lock
> information from the particular fd.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Christian Brauner <brauner@kernel.org>
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> ---
>  fs/locks.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index df8b26a42524..210766007e63 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
>  	return locks_conflict(caller_fl, sys_fl);
>  }
>  
> +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
> + * path so checks for additional GETLK-specific things like F_UNLCK.
> + */
> +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
> +				      struct file_lock *sys_fl)
> +{
> +	/* F_UNLCK checks any locks on the same fd. */
> +	if (caller_fl->fl_type == F_UNLCK) {
> +		if (!posix_same_owner(caller_fl, sys_fl))
> +			return false;
> +		return locks_overlap(caller_fl, sys_fl);
> +	}
> +	return posix_locks_conflict(caller_fl, sys_fl);
> +}
> +
>  /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
>   * checking before calling the locks_conflict().
>   */
> @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  retry:
>  	spin_lock(&ctx->flc_lock);
>  	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
> -		if (!posix_locks_conflict(fl, cfl))
> +		if (!posix_test_locks_conflict(fl, cfl))
>  			continue;
>  		if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
>  			&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
> @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
>  	if (fl == NULL)
>  		return -ENOMEM;
>  	error = -EINVAL;
> -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> +			&& flock->l_type != F_WRLCK)
>  		goto out;
>  
>  	error = flock_to_posix_lock(filp, fl, flock);
> @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
>  		return -ENOMEM;
>  
>  	error = -EINVAL;
> -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> +			&& flock->l_type != F_WRLCK)
>  		goto out;
>  
>  	error = flock64_to_posix_lock(filp, fl, flock);

This seems like a reasonable sort of interface to add, particularly for
the CRIU case. Using F_UNLCK for this is a bit kludgey, but adding a new
constant is probably worse.

I'm willing to take this in with an eye toward v6.6. Are you also
willing to draft up some manpage patches that detail this new interface?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> Currently F_OFD_GETLK sets the pid of the lock owner to -1.
> Remove such behavior to allow getting the proper owner's pid.
> This may be helpful when you want to send some message (like SIGKILL)
> to the offending locker.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Christian Brauner <brauner@kernel.org>
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> ---
>  fs/locks.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 210766007e63..ee265e166542 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
>  	pid_t vnr;
>  	struct pid *pid;
>  
> -	if (IS_OFDLCK(fl))
> -		return -1;
>  	if (IS_REMOTELCK(fl))
>  		return fl->fl_pid;
>  	/*

NACK on this one.

OFD locks are not owned by processes. They are owned by the file
description (hence the name). Because of this, returning a pid here is
wrong.

This precedent comes from BSD, where flock() and POSIX locks can
conflict. BSD returns -1 for the pid if you call F_GETLK on a file
locked with flock(). Since OFD locks have similar ownership semantics to
flock() locks, we use the same convention here.

Cheers, 
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

Hello,

20.06.2023 15:51, Jeff Layton пишет:
> On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
>> Currently F_OFD_GETLK sets the pid of the lock owner to -1.
>> Remove such behavior to allow getting the proper owner's pid.
>> This may be helpful when you want to send some message (like SIGKILL)
>> to the offending locker.
>>
>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>
>> CC: Jeff Layton <jlayton@kernel.org>
>> CC: Chuck Lever <chuck.lever@oracle.com>
>> CC: Alexander Viro <viro@zeniv.linux.org.uk>
>> CC: Christian Brauner <brauner@kernel.org>
>> CC: linux-fsdevel@vger.kernel.org
>> CC: linux-kernel@vger.kernel.org
>>
>> ---
>>   fs/locks.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index 210766007e63..ee265e166542 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
>>   	pid_t vnr;
>>   	struct pid *pid;
>>   
>> -	if (IS_OFDLCK(fl))
>> -		return -1;
>>   	if (IS_REMOTELCK(fl))
>>   		return fl->fl_pid;
>>   	/*
> NACK on this one.
>
> OFD locks are not owned by processes. They are owned by the file
> description (hence the name). Because of this, returning a pid here is
> wrong.

But fd is owned by a process.
PID has a meaning, you can send SIGKILL
to the returned PID, and the lock is clear.
Was there any reason to hide the PID at
a first place?


> This precedent comes from BSD, where flock() and POSIX locks can
> conflict. BSD returns -1 for the pid if you call F_GETLK on a file
> locked with flock(). Since OFD locks have similar ownership semantics to
> flock() locks, we use the same convention here.
OK if you insist I can drop this one and
search the PID by some other means.
Just a bit unsure what makes it so important
to overwrite the potentially useful info
with -1.

So in case you insist on that, then should
I send a v2 or can you just drop the patch
yourself?

Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

[stsp]

Hello,

20.06.2023 15:46, Jeff Layton пишет:
> On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
>> Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
>> The proposed extension allows to use it for getting the lock
>> information from the particular fd.
>>
>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>
>> CC: Jeff Layton <jlayton@kernel.org>
>> CC: Chuck Lever <chuck.lever@oracle.com>
>> CC: Alexander Viro <viro@zeniv.linux.org.uk>
>> CC: Christian Brauner <brauner@kernel.org>
>> CC: linux-fsdevel@vger.kernel.org
>> CC: linux-kernel@vger.kernel.org
>>
>> ---
>>   fs/locks.c | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index df8b26a42524..210766007e63 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
>>   	return locks_conflict(caller_fl, sys_fl);
>>   }
>>   
>> +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
>> + * path so checks for additional GETLK-specific things like F_UNLCK.
>> + */
>> +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
>> +				      struct file_lock *sys_fl)
>> +{
>> +	/* F_UNLCK checks any locks on the same fd. */
>> +	if (caller_fl->fl_type == F_UNLCK) {
>> +		if (!posix_same_owner(caller_fl, sys_fl))
>> +			return false;
>> +		return locks_overlap(caller_fl, sys_fl);
>> +	}
>> +	return posix_locks_conflict(caller_fl, sys_fl);
>> +}
>> +
>>   /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
>>    * checking before calling the locks_conflict().
>>    */
>> @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>   retry:
>>   	spin_lock(&ctx->flc_lock);
>>   	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
>> -		if (!posix_locks_conflict(fl, cfl))
>> +		if (!posix_test_locks_conflict(fl, cfl))
>>   			continue;
>>   		if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
>>   			&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
>> @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
>>   	if (fl == NULL)
>>   		return -ENOMEM;
>>   	error = -EINVAL;
>> -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
>> +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
>> +			&& flock->l_type != F_WRLCK)
>>   		goto out;
>>   
>>   	error = flock_to_posix_lock(filp, fl, flock);
>> @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
>>   		return -ENOMEM;
>>   
>>   	error = -EINVAL;
>> -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
>> +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
>> +			&& flock->l_type != F_WRLCK)
>>   		goto out;
>>   
>>   	error = flock64_to_posix_lock(filp, fl, flock);
> This seems like a reasonable sort of interface to add, particularly for
> the CRIU case.

Just for the record: my own cases are
the remaining 2. CRIU case is not mine
and I haven't talked to CRIU people
about that.


>   Using F_UNLCK for this is a bit kludgey, but adding a new
> constant is probably worse.
>
> I'm willing to take this in with an eye toward v6.6. Are you also
> willing to draft up some manpage patches that detail this new interface?
Sure thing.
As soon as its applied, I'll prepare a man
patch, or should it be done before that point?

Re: [PATCH 3/3] selftests: add OFD lock tests

[Jeff Layton]

On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> Test the basic locking stuff on 2 fds: multiple read locks,
> conflicts between read and write locks, use of len==0 for queries.
> Also test for pid and F_UNLCK F_OFD_GETLK extensions.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Shuah Khan <shuah@kernel.org>
> CC: linux-kernel@vger.kernel.org
> CC: linux-kselftest@vger.kernel.org
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Christian Brauner <brauner@kernel.org>
> CC: linux-fsdevel@vger.kernel.org
> 
> ---
>  tools/testing/selftests/locking/Makefile   |   2 +
>  tools/testing/selftests/locking/ofdlocks.c | 138 +++++++++++++++++++++
>  2 files changed, 140 insertions(+)
>  create mode 100644 tools/testing/selftests/locking/ofdlocks.c
> 
> diff --git a/tools/testing/selftests/locking/Makefile b/tools/testing/selftests/locking/Makefile
> index 6e7761ab3536..a83ced1626de 100644
> --- a/tools/testing/selftests/locking/Makefile
> +++ b/tools/testing/selftests/locking/Makefile
> @@ -7,4 +7,6 @@ all:
>  
>  TEST_PROGS := ww_mutex.sh
>  
> +TEST_GEN_PROGS := ofdlocks
> +
>  include ../lib.mk
> diff --git a/tools/testing/selftests/locking/ofdlocks.c b/tools/testing/selftests/locking/ofdlocks.c
> new file mode 100644
> index 000000000000..1cff350e2c81
> --- /dev/null
> +++ b/tools/testing/selftests/locking/ofdlocks.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <assert.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include "../kselftest.h"
> +
> +static int lock_set(int fd, struct flock *fl)
> +{
> +	int ret;
> +
> +	fl->l_pid = 0;		// needed for OFD locks
> +	fl->l_whence = SEEK_SET;
> +	ret = fcntl(fd, F_OFD_SETLK, fl);
> +	if (ret)
> +		perror("fcntl()");
> +	return ret;
> +}
> +
> +static int lock_get(int fd, struct flock *fl)
> +{
> +	int ret;
> +
> +	fl->l_pid = 0;		// needed for OFD locks
> +	fl->l_whence = SEEK_SET;
> +	ret = fcntl(fd, F_OFD_GETLK, fl);
> +	if (ret)
> +		perror("fcntl()");
> +	return ret;
> +}
> +
> +int main(void)
> +{
> +	int rc;
> +	struct flock fl, fl2;
> +	int fd = open("/tmp/aa", O_RDWR | O_CREAT | O_EXCL, 0600);
> +	int fd2 = open("/tmp/aa", O_RDONLY);
> +
> +	unlink("aa");
> +	assert(fd != -1);
> +	assert(fd2 != -1);
> +	ksft_print_msg("[INFO] opened fds %i %i\n", fd, fd2);
> +
> +	/* Set some read lock */
> +	fl.l_type = F_RDLCK;
> +	fl.l_start = 5;
> +	fl.l_len = 3;
> +	rc = lock_set(fd, &fl);
> +	if (rc == 0) {
> +		ksft_print_msg
> +		    ("[SUCCESS] set OFD read lock on first fd\n");
> +	} else {
> +		ksft_print_msg("[FAIL] to set OFD read lock on first fd\n");
> +		return -1;
> +	}
> +	/* Make sure read locks do not conflict on different fds. */
> +	fl.l_type = F_RDLCK;
> +	fl.l_start = 5;
> +	fl.l_len = 1;
> +	rc = lock_get(fd2, &fl);
> +	if (rc != 0)
> +		return -1;
> +	if (fl.l_type != F_UNLCK) {
> +		ksft_print_msg("[FAIL] read locks conflicted\n");
> +		return -1;
> +	}
> +	/* Make sure read/write locks do conflict on different fds. */
> +	fl.l_type = F_WRLCK;
> +	fl.l_start = 5;
> +	fl.l_len = 1;
> +	rc = lock_get(fd2, &fl);
> +	if (rc != 0)
> +		return -1;
> +	if (fl.l_type != F_UNLCK) {
> +		ksft_print_msg
> +		    ("[SUCCESS] read and write locks conflicted\n");
> +	} else {
> +		ksft_print_msg
> +		    ("[SUCCESS] read and write locks not conflicted\n");
> +		return -1;
> +	}
> +	/* Get info about the lock on first fd. */
> +	fl.l_type = F_UNLCK;
> +	fl.l_start = 5;
> +	fl.l_len = 1;
> +	rc = lock_get(fd, &fl);
> +	if (rc != 0) {
> +		ksft_print_msg
> +		    ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
> +		return -1;
> +	}
> +	if (fl.l_type != F_UNLCK) {
> +		if (fl.l_pid != getpid()) {
> +			ksft_print_msg
> +			    ("[FAIL] F_OFD_GETLK does not return pid, %i\n",
> +			    fl.l_pid);
> +			return -1;
> +		}

A selftest seems like a reasonable thing to add. The above check will
need to be fixed to not expect a real pid on a OFD lock, of course.

> +		ksft_print_msg
> +		    ("[SUCCESS] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
> +		     fl.l_type, fl.l_pid, fl.l_len);
> +	} else {
> +		ksft_print_msg
> +		    ("[FAIL] F_OFD_GETLK with F_UNLCK did not return lock info\n");
> +		return -1;
> +	}
> +	/* Try the same but by locking everything by len==0. */
> +	fl2.l_type = F_UNLCK;
> +	fl2.l_start = 0;
> +	fl2.l_len = 0;
> +	rc = lock_get(fd, &fl2);
> +	if (rc != 0) {
> +		ksft_print_msg
> +		    ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
> +		return -1;
> +	}
> +	if (memcmp(&fl, &fl2, sizeof(fl))) {
> +		ksft_print_msg
> +		    ("[FAIL] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
> +		     fl.l_type, fl.l_pid, fl.l_len);
> +		return -1;
> +	}
> +	ksft_print_msg("[SUCCESS] F_UNLCK with len==0 returned the same\n");
> +	/* Get info about the lock on second fd - no locks on it. */
> +	fl.l_type = F_UNLCK;
> +	fl.l_start = 0;
> +	fl.l_len = 0;
> +	lock_get(fd2, &fl);
> +	if (fl.l_type != F_UNLCK) {
> +		ksft_print_msg
> +		    ("[FAIL] F_OFD_GETLK with F_UNLCK return lock info from another fd\n");
> +		return -1;
> +	}
> +	return 0;
> +}

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 15:57 +0500, stsp wrote:
> Hello,
> 
> 20.06.2023 15:51, Jeff Layton пишет:
> > On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> > > Currently F_OFD_GETLK sets the pid of the lock owner to -1.
> > > Remove such behavior to allow getting the proper owner's pid.
> > > This may be helpful when you want to send some message (like SIGKILL)
> > > to the offending locker.
> > > 
> > > Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> > > 
> > > CC: Jeff Layton <jlayton@kernel.org>
> > > CC: Chuck Lever <chuck.lever@oracle.com>
> > > CC: Alexander Viro <viro@zeniv.linux.org.uk>
> > > CC: Christian Brauner <brauner@kernel.org>
> > > CC: linux-fsdevel@vger.kernel.org
> > > CC: linux-kernel@vger.kernel.org
> > > 
> > > ---
> > >   fs/locks.c | 2 --
> > >   1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 210766007e63..ee265e166542 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
> > >   	pid_t vnr;
> > >   	struct pid *pid;
> > >   
> > > -	if (IS_OFDLCK(fl))
> > > -		return -1;
> > >   	if (IS_REMOTELCK(fl))
> > >   		return fl->fl_pid;
> > >   	/*
> > NACK on this one.
> > 
> > OFD locks are not owned by processes. They are owned by the file
> > description (hence the name). Because of this, returning a pid here is
> > wrong.
> 
> But fd is owned by a process.

No, it isn't.

fd's can be handed off between processes via unix descriptors.

Multithreaded processes are also a bit of a gray area here: Suppose I
open a file and set an OFD lock on it in one task, and then let that
task exit while the file is still open. What should l_pid say in that
case?

> PID has a meaning, you can send SIGKILL
> to the returned PID, and the lock is clear.
> Was there any reason to hide the PID at
> a first place?
> 

Yes, because OFD locks are not tied to a pid in the same way that
traditional POSIX locks are.

> 
> > This precedent comes from BSD, where flock() and POSIX locks can
> > conflict. BSD returns -1 for the pid if you call F_GETLK on a file
> > locked with flock(). Since OFD locks have similar ownership semantics to
> > flock() locks, we use the same convention here.
> OK if you insist I can drop this one and
> search the PID by some other means.
> Just a bit unsure what makes it so important
> to overwrite the potentially useful info
> with -1.
> 
> So in case you insist on that, then should
> I send a v2 or can you just drop the patch
> yourself?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 16:00 +0500, stsp wrote:
> Hello,
> 
> 20.06.2023 15:46, Jeff Layton пишет:
> > On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> > > Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
> > > The proposed extension allows to use it for getting the lock
> > > information from the particular fd.
> > > 
> > > Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> > > 
> > > CC: Jeff Layton <jlayton@kernel.org>
> > > CC: Chuck Lever <chuck.lever@oracle.com>
> > > CC: Alexander Viro <viro@zeniv.linux.org.uk>
> > > CC: Christian Brauner <brauner@kernel.org>
> > > CC: linux-fsdevel@vger.kernel.org
> > > CC: linux-kernel@vger.kernel.org
> > > 
> > > ---
> > >   fs/locks.c | 23 ++++++++++++++++++++---
> > >   1 file changed, 20 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index df8b26a42524..210766007e63 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
> > >   	return locks_conflict(caller_fl, sys_fl);
> > >   }
> > >   
> > > +/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
> > > + * path so checks for additional GETLK-specific things like F_UNLCK.
> > > + */
> > > +static bool posix_test_locks_conflict(struct file_lock *caller_fl,
> > > +				      struct file_lock *sys_fl)
> > > +{
> > > +	/* F_UNLCK checks any locks on the same fd. */
> > > +	if (caller_fl->fl_type == F_UNLCK) {
> > > +		if (!posix_same_owner(caller_fl, sys_fl))
> > > +			return false;
> > > +		return locks_overlap(caller_fl, sys_fl);
> > > +	}
> > > +	return posix_locks_conflict(caller_fl, sys_fl);
> > > +}
> > > +
> > >   /* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
> > >    * checking before calling the locks_conflict().
> > >    */
> > > @@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> > >   retry:
> > >   	spin_lock(&ctx->flc_lock);
> > >   	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
> > > -		if (!posix_locks_conflict(fl, cfl))
> > > +		if (!posix_test_locks_conflict(fl, cfl))
> > >   			continue;
> > >   		if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
> > >   			&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
> > > @@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
> > >   	if (fl == NULL)
> > >   		return -ENOMEM;
> > >   	error = -EINVAL;
> > > -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> > > +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> > > +			&& flock->l_type != F_WRLCK)
> > >   		goto out;
> > >   
> > >   	error = flock_to_posix_lock(filp, fl, flock);
> > > @@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
> > >   		return -ENOMEM;
> > >   
> > >   	error = -EINVAL;
> > > -	if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
> > > +	if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
> > > +			&& flock->l_type != F_WRLCK)
> > >   		goto out;
> > >   
> > >   	error = flock64_to_posix_lock(filp, fl, flock);
> > This seems like a reasonable sort of interface to add, particularly for
> > the CRIU case.
> 
> Just for the record: my own cases are
> the remaining 2. CRIU case is not mine
> and I haven't talked to CRIU people
> about that.
> 
> 
> >   Using F_UNLCK for this is a bit kludgey, but adding a new
> > constant is probably worse.
> > 
> > I'm willing to take this in with an eye toward v6.6. Are you also
> > willing to draft up some manpage patches that detail this new interface?
> Sure thing.
> As soon as its applied, I'll prepare a man
> patch, or should it be done before that point?

These days, it's a good idea to go ahead and draft that up early. You'll
be surprised what sort of details you notice once you have to start
writing documentation. ;)

You can post it as part of this set on the next posting and just mention
that it's a draft manpage patch. You should also include the linux-api
mailing list on the next posting so we get some feedback on the
interface itself.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 16:12, Jeff Layton пишет:
> Multithreaded processes are also a bit of a gray area here: Suppose I
> open a file and set an OFD lock on it in one task, and then let that
> task exit while the file is still open. What should l_pid say in that
> case?

If by the "task" you mean a process, then
the result should be no lock at all.
If you mean the thread exit, then I would
expect l_pid to contain tgid, in which case
it will still point to the valid pid.
Or do you mean l_pid contains tid?
Checking... no, l_pid contains tgid.
So I don't really see the problem you are
pointing with the above example, could
you please clarify?

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 16:45 +0500, stsp wrote:
> 20.06.2023 16:12, Jeff Layton пишет:
> > Multithreaded processes are also a bit of a gray area here: Suppose I
> > open a file and set an OFD lock on it in one task, and then let that
> > task exit while the file is still open. What should l_pid say in that
> > case?
> 
> If by the "task" you mean a process, then
> the result should be no lock at all.
> If you mean the thread exit, then I would
> expect l_pid to contain tgid, in which case
> it will still point to the valid pid.
> Or do you mean l_pid contains tid?
> Checking... no, l_pid contains tgid.
> So I don't really see the problem you are
> pointing with the above example, could
> you please clarify?
> 

Suppose I start a process (call it pid 100), and then spawn a thread
(101). I then have 101 open a file and set an OFD lock on it (such that
the resulting fl_pid field in the file_lock is set to 101). Now, join
the thread so that task 101 exits.

Wait a while, and eventually pid 101 will be recycled, so that now there
is a new task 101 that is unrelated to the process above. Another
(unrelated) task then calls F_GETLK on the file and sees that lock. Its
pid is now set to 101 and sends SIGKILL to it, killing an unrelated
process.

That's just one example, of course. The underlying problem is that OFD
locks are not owned by processes in the same way that traditional POSIX
locks are, so reporting a pid there is unreliable, at best.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 17:02, Jeff Layton пишет:
> Suppose I start a process (call it pid 100), and then spawn a thread
> (101). I then have 101 open a file and set an OFD lock on it (such that
> the resulting fl_pid field in the file_lock is set to 101).

How come?
There are multiple places in locks.c
with this line:
fl->fl_pid = current->tgid;

And I've yet to see the line like:
fl->fl_pid = current->pid;
Its simply not there.

No, we put tgid into l_pid!
tgid will still be 100, no matter how
many threads you spawn or destroy.
Or what am I misseng?


> That's just one example, of course. The underlying problem is that OFD
> locks are not owned by processes in the same way that traditional POSIX
> locks are, so reporting a pid there is unreliable, at best.
But we report tgid.
It doesn't depend on threads.
I don't understand. :)

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 17:34 +0500, stsp wrote:
> 20.06.2023 17:02, Jeff Layton пишет:
> > Suppose I start a process (call it pid 100), and then spawn a thread
> > (101). I then have 101 open a file and set an OFD lock on it (such that
> > the resulting fl_pid field in the file_lock is set to 101).
> 
> How come?
> There are multiple places in locks.c
> with this line:
> fl->fl_pid = current->tgid;
> 
> And I've yet to see the line like:
> fl->fl_pid = current->pid;
> Its simply not there.
> 
> No, we put tgid into l_pid!
> tgid will still be 100, no matter how
> many threads you spawn or destroy.
> Or what am I misseng?
> 
>
> > That's just one example, of course. The underlying problem is that OFD
> > locks are not owned by processes in the same way that traditional POSIX
> > locks are, so reporting a pid there is unreliable, at best.
> But we report tgid.
> It doesn't depend on threads.
> I don't understand. :)

Good point. I had forgotten that we stuffed the l_pid in there. So in
principle, that example would be OK. But...there is still the problem of
passing file descriptors via unix sockets.

The bottom line is that these locks are specifically not owned by a
process, so returning the l_pid field is unreliable (at best). There is
no guarantee that the pid returned will still represent the task that
set the lock.

You may want to review this article. They're called "File-private" locks
here, but the name was later changed to "Open file description" (OFD)
locks:

    https://lwn.net/Articles/586904/

The rationale for why -1 is reported is noted there.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 18:19, Jeff Layton пишет:
> The bottom line is that these locks are specifically not owned by a
> process, so returning the l_pid field is unreliable (at best). There is
> no guarantee that the pid returned will still represent the task that
> set the lock.

Though it will, for sure, represent the
task that _owns_ the lock.

> You may want to review this article. They're called "File-private" locks
> here, but the name was later changed to "Open file description" (OFD)
> locks:
>
>      https://lwn.net/Articles/586904/
>
> The rationale for why -1 is reported is noted there.
Well, they point to fork() and SCM_RIGHTS.
Yes, these 2 beasts can make the same lock
owned by more than one process.
Yet l_pid returned, is going to be always valid:
it will still represent one of the valid owners.
So my call is to be brave and just re-consider
the conclusion of that article, made 10 years
ago! :)

Of course if returning just 1 of possibly multiple
owners is a problem, then oh well, I'll drop
this patch.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Matthew Wilcox]

On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> Though it will, for sure, represent the
> task that _owns_ the lock.

No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
and then exit.  Now the only owner of that lock is the recipient ...
who may not even have received the fd yet.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 18:46, Matthew Wilcox пишет:
> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
>> Though it will, for sure, represent the
>> task that _owns_ the lock.
> No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
> and then exit.  Now the only owner of that lock is the recipient ...
Won't I get the recipient's pid in an
l_pid then?

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Tue, 2023-06-20 at 18:39 +0500, stsp wrote:
> 20.06.2023 18:19, Jeff Layton пишет:
> > The bottom line is that these locks are specifically not owned by a
> > process, so returning the l_pid field is unreliable (at best). There is
> > no guarantee that the pid returned will still represent the task that
> > set the lock.
> 
> Though it will, for sure, represent the
> task that _owns_ the lock.
> 
> > You may want to review this article. They're called "File-private" locks
> > here, but the name was later changed to "Open file description" (OFD)
> > locks:
> > 
> >      https://lwn.net/Articles/586904/
> > 
> > The rationale for why -1 is reported is noted there.
> Well, they point to fork() and SCM_RIGHTS.
> Yes, these 2 beasts can make the same lock
> owned by more than one process.
> Yet l_pid returned, is going to be always valid:
> it will still represent one of the valid owners.

No, it won't. The l_pid field is populated from the file_lock->fl_pid.
That field is set when the lock is set, and never updated. So it's quite
possible for F_GETLK to return the pid of a process that no longer
exists.

In principle, we could try to address that by changing how we track lock
ownership, but that's a fairly major overhaul, and I'm not clear on any
use-cases where that matters.

> So my call is to be brave and just re-consider
> the conclusion of that article, made 10 years
> ago! :)
> 

I think my foot has too many bullet wounds for that sort of bravery.

> Of course if returning just 1 of possibly multiple
> owners is a problem, then oh well, I'll drop
> this patch.


-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Matthew Wilcox]

On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
> 
> 20.06.2023 18:46, Matthew Wilcox пишет:
> > On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > > Though it will, for sure, represent the
> > > task that _owns_ the lock.
> > No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
> > and then exit.  Now the only owner of that lock is the recipient ...
> Won't I get the recipient's pid in an
> l_pid then?

You snipped the part where I pointed out that at times there can be
_no_ task that owns it.  open a fd, set the lock, pass the fd to another
task, exit.  until that task calls recvmsg(), no task owns it.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 19:36, Matthew Wilcox пишет:
> On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
>> 20.06.2023 18:46, Matthew Wilcox пишет:
>>> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
>>>> Though it will, for sure, represent the
>>>> task that _owns_ the lock.
>>> No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
>>> and then exit.  Now the only owner of that lock is the recipient ...
>> Won't I get the recipient's pid in an
>> l_pid then?
> You snipped the part where I pointed out that at times there can be
> _no_ task that owns it.  open a fd, set the lock, pass the fd to another
> task, exit.  until that task calls recvmsg(), no task owns it.
Hmm, interesting case.
So at least it seems if recipient also exits,
then the untransferred fd gets closed.
Does this mean, by any chance, that the
recipient actually owns an fd before
recvmsg() is done?

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Matthew Wilcox]

On Tue, Jun 20, 2023 at 08:45:21PM +0500, stsp wrote:
> 
> 20.06.2023 19:36, Matthew Wilcox пишет:
> > On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
> > > 20.06.2023 18:46, Matthew Wilcox пишет:
> > > > On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > > > > Though it will, for sure, represent the
> > > > > task that _owns_ the lock.
> > > > No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
> > > > and then exit.  Now the only owner of that lock is the recipient ...
> > > Won't I get the recipient's pid in an
> > > l_pid then?
> > You snipped the part where I pointed out that at times there can be
> > _no_ task that owns it.  open a fd, set the lock, pass the fd to another
> > task, exit.  until that task calls recvmsg(), no task owns it.
> Hmm, interesting case.
> So at least it seems if recipient also exits,
> then the untransferred fd gets closed.

yes, pretty sure this is done by garbage collection in the unix socket
handling code, though i've never looked at it.  it's done that way
because annoying people can do things like open two sockets and send the
fd of each to the other, then exit.

> Does this mean, by any chance, that the
> recipient actually owns an fd before
> recvmsg() is done?

no, it's not in their fd table.  they don't own it.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 22:05, Matthew Wilcox пишет:
>> Does this mean, by any chance, that the
>> recipient actually owns an fd before
>> recvmsg() is done?
> no, it's not in their fd table.  they don't own it.
OK, thanks for showing this pathological
case. Let me just note that this changes
nothing at all. :)

The important thing to note here is that
any lock query is race-prone: locks can
come and go at any time. So if you need
some sequence of operations, you need
to employ some global locking for that.
I use flock(LOCK_EX) on the same fd, before
doing F_OFD_GETLK, and I do flock(LOCK_UN)
only when the entire sequence of operations
is completed. And I do the same on an
F_OFD_SETLK's side to guarantee the
atomicity. You can't do it otherwise,
it would be race-prone.

So given the above, the only thing we
need for l_pid consistency is for the
"donor" process to put LOCK_EX on an
fd before doing SCM_RIGHTS, and the
recipient should do LOCK_UN. Then
the other side, which also uses LOCK_EX,
will never see the owner-less state.
And as for the kernel's POV, l_pid should
be set to -1 only when there is no owner,
like in an example you mentioned.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

20.06.2023 18:58, Jeff Layton пишет:
> No, it won't. The l_pid field is populated from the file_lock->fl_pid.
> That field is set when the lock is set, and never updated. So it's quite
> possible for F_GETLK to return the pid of a process that no longer
> exists.
>
> In principle, we could try to address that by changing how we track lock
> ownership, but that's a fairly major overhaul, and I'm not clear on any
> use-cases where that matters.

OK, in this case I'll just put a comments
into the code, summarizing the info I got
from you and Matthew.
Thanks guys for all the info, its very helpful.

Now I only need to convert the current
"fundamental problem" attitude into a "not
implemented yet" via the code comment.


>> So my call is to be brave and just re-consider
>> the conclusion of that article, made 10 years
>> ago! :)
>>
> I think my foot has too many bullet wounds for that sort of bravery.
I am perfectly fine with leaving this thing
unimplemented. But what really bothers
me is the posix proposal, which I think was
done. Please tell me it allows fixing fl_pid
in the future (rather than to mandate -1),
and I am calm.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Wed, 2023-06-21 at 11:57 +0500, stsp wrote:
> 20.06.2023 18:58, Jeff Layton пишет:
> > No, it won't. The l_pid field is populated from the file_lock->fl_pid.
> > That field is set when the lock is set, and never updated. So it's quite
> > possible for F_GETLK to return the pid of a process that no longer
> > exists.
> > 
> > In principle, we could try to address that by changing how we track lock
> > ownership, but that's a fairly major overhaul, and I'm not clear on any
> > use-cases where that matters.
> 
> OK, in this case I'll just put a comments
> into the code, summarizing the info I got
> from you and Matthew.
> Thanks guys for all the info, its very helpful.
> 
> Now I only need to convert the current
> "fundamental problem" attitude into a "not
> implemented yet" via the code comment.
> 
> 
> > > So my call is to be brave and just re-consider
> > > the conclusion of that article, made 10 years
> > > ago! :)
> > > 
> > I think my foot has too many bullet wounds for that sort of bravery.
> I am perfectly fine with leaving this thing
> unimplemented. But what really bothers
> me is the posix proposal, which I think was
> done. Please tell me it allows fixing fl_pid
> in the future (rather than to mandate -1),
> and I am calm.

I don't think we can change this at this point.

The bottom line (again) is that OFD locks are owned by the file
descriptor (much like with flock()), and since file descriptors can be
shared across multiple process it's impossible to say that some single
process owns it.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

21.06.2023 15:35, Jeff Layton пишет:
> I don't think we can change this at this point.
>
> The bottom line (again) is that OFD locks are owned by the file
> descriptor (much like with flock()), and since file descriptors can be
> shared across multiple process it's impossible to say that some single
> process owns it.
What's the problem with 2 owners?
Can't you get one of them, rather than
meaningless -1?
Compare this situation with read locks.
They can overlap, so when you get an
info about a read lock (except for the
new F_UNLCK case), you get the info
about *some* of the locks in that range.
In the case of multiple owners, you
likewise get the info about about some
owner. If you iteratively send them a
"please release this lock" message
(eg in a form of SIGKILL), then you
traverse all, and end up with the
lock-free area.
Is there really any problem here?

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> 21.06.2023 15:35, Jeff Layton пишет:
> > I don't think we can change this at this point.
> > 
> > The bottom line (again) is that OFD locks are owned by the file
> > descriptor (much like with flock()), and since file descriptors can be
> > shared across multiple process it's impossible to say that some single
> > process owns it.
> What's the problem with 2 owners?
> Can't you get one of them, rather than
> meaningless -1?
> Compare this situation with read locks.
> They can overlap, so when you get an
> info about a read lock (except for the
> new F_UNLCK case), you get the info
> about *some* of the locks in that range.
> In the case of multiple owners, you
> likewise get the info about about some
> owner. If you iteratively send them a
> "please release this lock" message
> (eg in a form of SIGKILL), then you
> traverse all, and end up with the
> lock-free area.
> Is there really any problem here?

Yes. Ambiguous answers are worse than none at all.

What problem are you trying to solve by having F_OFD_GETLK report a pid?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

21.06.2023 16:05, Jeff Layton пишет:
> Yes. Ambiguous answers are worse than none at all.

But same for read locks, when you
query them with F_OFD_GETLK.
It doesn't sound ambiguous to me,
you get the valid owner, and you can
iterate them if you kill them in a
process (same as for read locks).


> What problem are you trying to solve by having F_OFD_GETLK report a pid?
Just a way to abruptly kill offending lockers,
as that most likely means the process hanged
(I have a 3rd party code that drives the locking,
so it can't be trusted not to hang).
Its not essential though, for sure.
Curiosity also plays the role here. :)
Though if you don't want, I can as well
not add a TODO comment to the code.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

21.06.2023 16:22, stsp пишет:
>
> 21.06.2023 16:05, Jeff Layton пишет:
>> Yes. Ambiguous answers are worse than none at all.
>
> But same for read locks, when you
> query them with F_OFD_GETLK.
> It doesn't sound ambiguous to me,
> you get the valid owner, and you can
> iterate them if you kill them in a
> process (same as for read locks).
And in fact, no, you can't iterate the
read locks *unless* you use pid to
kill its owners! :) So this way you can
actually iterate the read locks, but
not in a most convenient way. :)

Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK

[stsp]

20.06.2023 16:15, Jeff Layton пишет:
> These days, it's a good idea to go ahead and draft that up early. You'll
> be surprised what sort of details you notice once you have to start
> writing documentation. ;)
>
> You can post it as part of this set on the next posting and just mention
> that it's a draft manpage patch. You should also include the linux-api
> mailing list on the next posting so we get some feedback on the
> interface itself.
v2 is sent with the proposed man page
update and a drop of an l_pid.
Thanks.

RE: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[David Laight]

From: Matthew Wilcox
> Sent: 20 June 2023 14:46
> 
> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > Though it will, for sure, represent the
> > task that _owns_ the lock.
> 
> No, it *DOESN'T*.  I can open a file, SCM_RIGHTS pass it to another task
> and then exit.  Now the only owner of that lock is the recipient ...
> who may not even have received the fd yet.

Do these locks persist across fork+exec?

What happens is a completely unrelated process opens /proc/<pid>/fd
while a lock is held and then the (nominally) lock-holding
process closes the fd (or exits).

While it might be a useful diagnostic to know the pid of the
process that acquired the lock it clearly has no relationship
with any process that currently has an fd[] table entry that
references the file.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Christian Brauner]

On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
> On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> > 21.06.2023 15:35, Jeff Layton пишет:
> > > I don't think we can change this at this point.
> > > 
> > > The bottom line (again) is that OFD locks are owned by the file
> > > descriptor (much like with flock()), and since file descriptors can be
> > > shared across multiple process it's impossible to say that some single
> > > process owns it.
> > What's the problem with 2 owners?
> > Can't you get one of them, rather than
> > meaningless -1?
> > Compare this situation with read locks.
> > They can overlap, so when you get an
> > info about a read lock (except for the
> > new F_UNLCK case), you get the info
> > about *some* of the locks in that range.
> > In the case of multiple owners, you
> > likewise get the info about about some
> > owner. If you iteratively send them a
> > "please release this lock" message
> > (eg in a form of SIGKILL), then you
> > traverse all, and end up with the
> > lock-free area.
> > Is there really any problem here?
> 
> Yes. Ambiguous answers are worse than none at all.

I agree.

A few minor observations:

SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
it's been mentioned explicitly but that trivially means it's possible to
send an fd to a completely separate thread-group, then kill off the
sending thread-group by killing their thread-group leader. Bad enough as
the identifier is now useless. But it also means that at some later
point that pid can be recycled. So using that pid for killing sprees is
usually a bad idea and might cause killing a completely separate
thread-group.

pidfd_getfd() can be used to retrieve a file descriptor from another
process; like an inverse dup. Opens up similar problems as SCM_RIGHTS.

Expressing ownership doesn't make sense as this is an inherently shared
concept as Jeff mentioned. So best case is to try and track the creator
similar to SO_PEERCRED for unix sockets.

In any case, this is all made worse by the fact that struct file_lock
stashes raw pid numbers. That means that the kernel itself can't even
detect whether the creator of that lock has died and the pid possibly
been recycled. In essence, it risk reporting as owner a random process
that just happens to have the same pid number as the creator that
might've died.

Even for unix sockets SO_PEERCRED and SCM_CREDS are inherently racy even
though they stash a struct pid instead of a raw pid number and that
makes them problematic in a lot of scenario which is why they will
support pidfds with v6.5.

The situation could probably be improved by stashing a struct pid in
struct file_lock then you could at least somewhat reasonably track the
creator as the kernel would be able to detect whether the creating
process has already been reaped (not exited). For example, I did this
for pidfds where it reports -1 if the process the pidfd refers to has
been reaped.

Btw, is there a way to limit the number of file locks a user might
create? Is that implicitly bound to the maximum number of fds and files
a caller may create because I saw RLIMIT_LOCKS but I didn't see it
checked anywhere.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

23.06.2023 20:25, Christian Brauner пишет:
> On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
>> On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
>>> 21.06.2023 15:35, Jeff Layton пишет:
>>>> I don't think we can change this at this point.
>>>>
>>>> The bottom line (again) is that OFD locks are owned by the file
>>>> descriptor (much like with flock()), and since file descriptors can be
>>>> shared across multiple process it's impossible to say that some single
>>>> process owns it.
>>> What's the problem with 2 owners?
>>> Can't you get one of them, rather than
>>> meaningless -1?
>>> Compare this situation with read locks.
>>> They can overlap, so when you get an
>>> info about a read lock (except for the
>>> new F_UNLCK case), you get the info
>>> about *some* of the locks in that range.
>>> In the case of multiple owners, you
>>> likewise get the info about about some
>>> owner. If you iteratively send them a
>>> "please release this lock" message
>>> (eg in a form of SIGKILL), then you
>>> traverse all, and end up with the
>>> lock-free area.
>>> Is there really any problem here?
>> Yes. Ambiguous answers are worse than none at all.
> I agree.
>
> A few minor observations:
>
> SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
> it's been mentioned explicitly but that trivially means it's possible to
> send an fd to a completely separate thread-group, then kill off the
> sending thread-group by killing their thread-group leader. Bad enough as
> the identifier is now useless. But it also means that at some later
> point that pid can be recycled.
Come on.
I never proposed anything like this.
Of course the returned pid should be
the pid of the current, actual owner,
or one of current owners.
If someone else proposed to return
stalled pids, then it wasn't me.

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[Jeff Layton]

On Fri, 2023-06-23 at 22:18 +0500, stsp wrote:
> 23.06.2023 20:25, Christian Brauner пишет:
> > On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
> > > On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> > > > 21.06.2023 15:35, Jeff Layton пишет:
> > > > > I don't think we can change this at this point.
> > > > > 
> > > > > The bottom line (again) is that OFD locks are owned by the file
> > > > > descriptor (much like with flock()), and since file descriptors can be
> > > > > shared across multiple process it's impossible to say that some single
> > > > > process owns it.
> > > > What's the problem with 2 owners?
> > > > Can't you get one of them, rather than
> > > > meaningless -1?
> > > > Compare this situation with read locks.
> > > > They can overlap, so when you get an
> > > > info about a read lock (except for the
> > > > new F_UNLCK case), you get the info
> > > > about *some* of the locks in that range.
> > > > In the case of multiple owners, you
> > > > likewise get the info about about some
> > > > owner. If you iteratively send them a
> > > > "please release this lock" message
> > > > (eg in a form of SIGKILL), then you
> > > > traverse all, and end up with the
> > > > lock-free area.
> > > > Is there really any problem here?
> > > Yes. Ambiguous answers are worse than none at all.
> > I agree.
> > 
> > A few minor observations:
> > 
> > SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
> > it's been mentioned explicitly but that trivially means it's possible to
> > send an fd to a completely separate thread-group, then kill off the
> > sending thread-group by killing their thread-group leader. Bad enough as
> > the identifier is now useless. But it also means that at some later
> > point that pid can be recycled.
> Come on.
> I never proposed anything like this.
> Of course the returned pid should be
> the pid of the current, actual owner,
> or one of current owners.
> If someone else proposed to return
> stalled pids, then it wasn't me.


Beyond all of this, there is a long history of problems with the l_pid
field as well with network filesystems, even with traditional POSIX
locks. What should go into the l_pid when a traditional POSIX lock is
held by a process on a separate host?

While POSIX mandates it, the l_pid is really sort of a "legacy" field
that is really just for informational purposes only nowadays. It might
have been a reliable bit of information back in the 1980's, but even
since the 90's it was suspect as a source of information.

Even if you _know_ you hold a traditional POSIX lock, be careful
trusting the information in that field.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK

[stsp]

27.06.2023 21:00, Jeff Layton пишет:
> Beyond all of this, there is a long history of problems with the l_pid
> field as well with network filesystems, even with traditional POSIX
> locks. What should go into the l_pid when a traditional POSIX lock is
> held by a process on a separate host?
>
> While POSIX mandates it, the l_pid is really sort of a "legacy" field
> that is really just for informational purposes only nowadays. It might
> have been a reliable bit of information back in the 1980's, but even
> since the 90's it was suspect as a source of information.
>
> Even if you _know_ you hold a traditional POSIX lock, be careful
> trusting the information in that field.
Thanks for info.
Additional problem with multiple owners
that I can think of, is that you don't know
if more owners are present. And even if
you use SIGKILL to "iterate", you still don't
know if you got another owner of the prev
lock, or maybe you got entirely different
read lock with the same range from another
owner.

Still if you do "man fcntl" you'll see this:

                pid_t l_pid;     /* PID of process blocking our lock

                                    (set by F_GETLK and F_OFD_GETLK) */

And no, its not my patch that did this. :)
So unless properly documented, this would
be treated as a bug. And it should _not_ be
documented as "OFD locks has no owner by
definition" or alike - no one buys that.