* [PATCH 0/3] RFC: F_OFD_GETLK should provide more info @ 2023-06-20 9:55 Stas Sergeev 2023-06-20 9:55 ` [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev ` (2 more replies) 0 siblings, 3 replies; 35+ messages in thread From: Stas Sergeev @ 2023-06-20 9:55 UTC (permalink / raw) To: linux-kernel Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Shuah Khan, linux-kselftest 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK 2023-06-20 9:55 [PATCH 0/3] RFC: F_OFD_GETLK should provide more info Stas Sergeev @ 2023-06-20 9:55 ` Stas Sergeev 2023-06-20 10:46 ` Jeff Layton 2023-06-20 9:55 ` [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK Stas Sergeev 2023-06-20 9:55 ` [PATCH 3/3] selftests: add OFD lock tests Stas Sergeev 2 siblings, 1 reply; 35+ messages in thread From: Stas Sergeev @ 2023-06-20 9:55 UTC (permalink / raw) To: linux-kernel Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK 2023-06-20 9:55 ` [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev @ 2023-06-20 10:46 ` Jeff Layton 2023-06-20 11:00 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 10:46 UTC (permalink / raw) To: Stas Sergeev, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK 2023-06-20 10:46 ` Jeff Layton @ 2023-06-20 11:00 ` stsp 2023-06-20 11:15 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 11:00 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK 2023-06-20 11:00 ` stsp @ 2023-06-20 11:15 ` Jeff Layton 2023-06-21 15:24 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 11:15 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK 2023-06-20 11:15 ` Jeff Layton @ 2023-06-21 15:24 ` stsp 0 siblings, 0 replies; 35+ messages in thread From: stsp @ 2023-06-21 15:24 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 9:55 [PATCH 0/3] RFC: F_OFD_GETLK should provide more info Stas Sergeev 2023-06-20 9:55 ` [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev @ 2023-06-20 9:55 ` Stas Sergeev 2023-06-20 10:51 ` Jeff Layton 2023-06-20 9:55 ` [PATCH 3/3] selftests: add OFD lock tests Stas Sergeev 2 siblings, 1 reply; 35+ messages in thread From: Stas Sergeev @ 2023-06-20 9:55 UTC (permalink / raw) To: linux-kernel Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 9:55 ` [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK Stas Sergeev @ 2023-06-20 10:51 ` Jeff Layton 2023-06-20 10:57 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 10:51 UTC (permalink / raw) To: Stas Sergeev, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 10:51 ` Jeff Layton @ 2023-06-20 10:57 ` stsp 2023-06-20 11:12 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 10:57 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 10:57 ` stsp @ 2023-06-20 11:12 ` Jeff Layton 2023-06-20 11:45 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 11:12 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 11:12 ` Jeff Layton @ 2023-06-20 11:45 ` stsp 2023-06-20 12:02 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 11:45 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 11:45 ` stsp @ 2023-06-20 12:02 ` Jeff Layton 2023-06-20 12:34 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 12:02 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 12:02 ` Jeff Layton @ 2023-06-20 12:34 ` stsp 2023-06-20 13:19 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 12:34 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. :) ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 12:34 ` stsp @ 2023-06-20 13:19 ` Jeff Layton 2023-06-20 13:39 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 13:19 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:19 ` Jeff Layton @ 2023-06-20 13:39 ` stsp 2023-06-20 13:46 ` Matthew Wilcox 2023-06-20 13:58 ` Jeff Layton 0 siblings, 2 replies; 35+ messages in thread From: stsp @ 2023-06-20 13:39 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:39 ` stsp @ 2023-06-20 13:46 ` Matthew Wilcox 2023-06-20 13:47 ` stsp 2023-06-23 13:10 ` David Laight 2023-06-20 13:58 ` Jeff Layton 1 sibling, 2 replies; 35+ messages in thread From: Matthew Wilcox @ 2023-06-20 13:46 UTC (permalink / raw) To: stsp Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:46 ` Matthew Wilcox @ 2023-06-20 13:47 ` stsp 2023-06-20 14:36 ` Matthew Wilcox 2023-06-23 13:10 ` David Laight 1 sibling, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 13:47 UTC (permalink / raw) To: Matthew Wilcox Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:47 ` stsp @ 2023-06-20 14:36 ` Matthew Wilcox 2023-06-20 15:45 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Matthew Wilcox @ 2023-06-20 14:36 UTC (permalink / raw) To: stsp Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 14:36 ` Matthew Wilcox @ 2023-06-20 15:45 ` stsp 2023-06-20 17:05 ` Matthew Wilcox 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-20 15:45 UTC (permalink / raw) To: Matthew Wilcox Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 15:45 ` stsp @ 2023-06-20 17:05 ` Matthew Wilcox 2023-06-21 2:54 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Matthew Wilcox @ 2023-06-20 17:05 UTC (permalink / raw) To: stsp Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 17:05 ` Matthew Wilcox @ 2023-06-21 2:54 ` stsp 0 siblings, 0 replies; 35+ messages in thread From: stsp @ 2023-06-21 2:54 UTC (permalink / raw) To: Matthew Wilcox Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:46 ` Matthew Wilcox 2023-06-20 13:47 ` stsp @ 2023-06-23 13:10 ` David Laight 1 sibling, 0 replies; 35+ messages in thread From: David Laight @ 2023-06-23 13:10 UTC (permalink / raw) To: 'Matthew Wilcox', stsp Cc: Jeff Layton, linux-kernel, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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) ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:39 ` stsp 2023-06-20 13:46 ` Matthew Wilcox @ 2023-06-20 13:58 ` Jeff Layton 2023-06-21 6:57 ` stsp 1 sibling, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-20 13:58 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-20 13:58 ` Jeff Layton @ 2023-06-21 6:57 ` stsp 2023-06-21 10:35 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-21 6:57 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 6:57 ` stsp @ 2023-06-21 10:35 ` Jeff Layton 2023-06-21 10:42 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-21 10:35 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 10:35 ` Jeff Layton @ 2023-06-21 10:42 ` stsp 2023-06-21 11:05 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-21 10:42 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 10:42 ` stsp @ 2023-06-21 11:05 ` Jeff Layton 2023-06-21 11:22 ` stsp 2023-06-23 15:25 ` Christian Brauner 0 siblings, 2 replies; 35+ messages in thread From: Jeff Layton @ 2023-06-21 11:05 UTC (permalink / raw) To: stsp, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 11:05 ` Jeff Layton @ 2023-06-21 11:22 ` stsp 2023-06-21 11:26 ` stsp 2023-06-23 15:25 ` Christian Brauner 1 sibling, 1 reply; 35+ messages in thread From: stsp @ 2023-06-21 11:22 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 11:22 ` stsp @ 2023-06-21 11:26 ` stsp 0 siblings, 0 replies; 35+ messages in thread From: stsp @ 2023-06-21 11:26 UTC (permalink / raw) To: Jeff Layton, linux-kernel Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel, Matthew Wilcox 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. :) ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-21 11:05 ` Jeff Layton 2023-06-21 11:22 ` stsp @ 2023-06-23 15:25 ` Christian Brauner 2023-06-23 17:18 ` stsp 1 sibling, 1 reply; 35+ messages in thread From: Christian Brauner @ 2023-06-23 15:25 UTC (permalink / raw) To: Jeff Layton Cc: stsp, linux-kernel, Chuck Lever, Alexander Viro, linux-fsdevel, Matthew Wilcox 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-23 15:25 ` Christian Brauner @ 2023-06-23 17:18 ` stsp 2023-06-27 16:00 ` Jeff Layton 0 siblings, 1 reply; 35+ messages in thread From: stsp @ 2023-06-23 17:18 UTC (permalink / raw) To: Christian Brauner, Jeff Layton Cc: linux-kernel, Chuck Lever, Alexander Viro, linux-fsdevel, Matthew Wilcox 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-23 17:18 ` stsp @ 2023-06-27 16:00 ` Jeff Layton 2023-06-27 16:20 ` stsp 0 siblings, 1 reply; 35+ messages in thread From: Jeff Layton @ 2023-06-27 16:00 UTC (permalink / raw) To: stsp, Christian Brauner Cc: linux-kernel, Chuck Lever, Alexander Viro, linux-fsdevel, Matthew Wilcox 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK 2023-06-27 16:00 ` Jeff Layton @ 2023-06-27 16:20 ` stsp 0 siblings, 0 replies; 35+ messages in thread From: stsp @ 2023-06-27 16:20 UTC (permalink / raw) To: Jeff Layton, Christian Brauner Cc: linux-kernel, Chuck Lever, Alexander Viro, linux-fsdevel, Matthew Wilcox 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. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/3] selftests: add OFD lock tests 2023-06-20 9:55 [PATCH 0/3] RFC: F_OFD_GETLK should provide more info Stas Sergeev 2023-06-20 9:55 ` [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev 2023-06-20 9:55 ` [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK Stas Sergeev @ 2023-06-20 9:55 ` Stas Sergeev 2023-06-20 11:06 ` Jeff Layton 2 siblings, 1 reply; 35+ messages in thread From: Stas Sergeev @ 2023-06-20 9:55 UTC (permalink / raw) To: linux-kernel Cc: Stas Sergeev, Shuah Khan, linux-kselftest, Jeff Layton, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/3] selftests: add OFD lock tests 2023-06-20 9:55 ` [PATCH 3/3] selftests: add OFD lock tests Stas Sergeev @ 2023-06-20 11:06 ` Jeff Layton 0 siblings, 0 replies; 35+ messages in thread From: Jeff Layton @ 2023-06-20 11:06 UTC (permalink / raw) To: Stas Sergeev, linux-kernel Cc: Shuah Khan, linux-kselftest, Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2023-06-27 16:21 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-20 9:55 [PATCH 0/3] RFC: F_OFD_GETLK should provide more info Stas Sergeev 2023-06-20 9:55 ` [PATCH 1/3] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev 2023-06-20 10:46 ` Jeff Layton 2023-06-20 11:00 ` stsp 2023-06-20 11:15 ` Jeff Layton 2023-06-21 15:24 ` stsp 2023-06-20 9:55 ` [PATCH 2/3] fd/locks: allow get the lock owner by F_OFD_GETLK Stas Sergeev 2023-06-20 10:51 ` Jeff Layton 2023-06-20 10:57 ` stsp 2023-06-20 11:12 ` Jeff Layton 2023-06-20 11:45 ` stsp 2023-06-20 12:02 ` Jeff Layton 2023-06-20 12:34 ` stsp 2023-06-20 13:19 ` Jeff Layton 2023-06-20 13:39 ` stsp 2023-06-20 13:46 ` Matthew Wilcox 2023-06-20 13:47 ` stsp 2023-06-20 14:36 ` Matthew Wilcox 2023-06-20 15:45 ` stsp 2023-06-20 17:05 ` Matthew Wilcox 2023-06-21 2:54 ` stsp 2023-06-23 13:10 ` David Laight 2023-06-20 13:58 ` Jeff Layton 2023-06-21 6:57 ` stsp 2023-06-21 10:35 ` Jeff Layton 2023-06-21 10:42 ` stsp 2023-06-21 11:05 ` Jeff Layton 2023-06-21 11:22 ` stsp 2023-06-21 11:26 ` stsp 2023-06-23 15:25 ` Christian Brauner 2023-06-23 17:18 ` stsp 2023-06-27 16:00 ` Jeff Layton 2023-06-27 16:20 ` stsp 2023-06-20 9:55 ` [PATCH 3/3] selftests: add OFD lock tests Stas Sergeev 2023-06-20 11:06 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).