On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote:
> This patch is changing the fl_owner value in case of an nfs lock request
> to not be the pid of lockd. Instead this patch changes it to be the
> owner value that nfs is giving us.
>
> Currently there exists proved problems with this behaviour. One nfsd
> server was created to export a gfs2 filesystem mount. Two nfs clients
> doing a nfs mount of this export. Those two clients should conflict each
> other operating on the same nfs file.
>
> A small test program was written:
>
> int main(int argc, const char *argv[])
> {
> struct flock fl = {
> .l_type = F_WRLCK,
> .l_whence = SEEK_SET,
> .l_start = 1L,
> .l_len = 1L,
> };
> int fd;
>
> fd = open("filename", O_RDWR | O_CREAT, 0700);
> printf("try to lock...\n");
> fcntl(fd, F_SETLKW, &fl);
> printf("locked!\n");
> getc(stdin);
>
> return 0;
> }
>
> Running on both clients at the same time and don't interrupting by
> pressing any key. It will show that both clients are able to acquire the
> lock which shouldn't be the case. The issue is here that the fl_owner
> value is the same and the lock context of both clients should be
> separated.
>
> This patch lets lockd define how to deal with lock contexts and chose
> hopefully the right fl_owner value. A test after this patch was made and
> the locks conflicts each other which should be the case.
>
> Signed-off-by: Alexander Aring <aahri...@redhat.com>
> ---
> fs/dlm/plock.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index 00e1d802a81c..0094fa4004cc 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -145,6 +145,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> op->info.number = number;
> op->info.start = fl->fl_start;
> op->info.end = fl->fl_end;
> + op->info.owner = (__u64)(long)fl->fl_owner;
> /* async handling */
> if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
> op_data = kzalloc(sizeof(*op_data), GFP_NOFS);
> @@ -154,9 +155,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> goto out;
> }
>
> - /* fl_owner is lockd which doesn't distinguish
> - processes on the nfs client */
> - op->info.owner = (__u64) fl->fl_pid;
> op_data->callback = fl->fl_lmops->lm_grant;
> locks_init_lock(&op_data->flc);
> locks_copy_lock(&op_data->flc, fl);
> @@ -168,8 +166,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> send_op(op);
> rv = FILE_LOCK_DEFERRED;
> goto out;
> - } else {
> - op->info.owner = (__u64)(long) fl->fl_owner;
> }
>
> send_op(op);
> @@ -326,10 +322,7 @@ int dlm_posix_unlock(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> op->info.number = number;
> op->info.start = fl->fl_start;
> op->info.end = fl->fl_end;
> - if (fl->fl_lmops && fl->fl_lmops->lm_grant)
> - op->info.owner = (__u64) fl->fl_pid;
> - else
> - op->info.owner = (__u64)(long) fl->fl_owner;
> + op->info.owner = (__u64)(long)fl->fl_owner;
>
> if (fl->fl_flags & FL_CLOSE) {
> op->info.flags |= DLM_PLOCK_FL_CLOSE;
> @@ -389,7 +382,7 @@ int dlm_posix_cancel(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> info.number = number;
> info.start = fl->fl_start;
> info.end = fl->fl_end;
> - info.owner = (__u64)fl->fl_pid;
> + info.owner = (__u64)(long)fl->fl_owner;
>
> rv = do_lock_cancel(&info);
> switch (rv) {
> @@ -450,10 +443,7 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> op->info.number = number;
> op->info.start = fl->fl_start;
> op->info.end = fl->fl_end;
> - if (fl->fl_lmops && fl->fl_lmops->lm_grant)
> - op->info.owner = (__u64) fl->fl_pid;
> - else
> - op->info.owner = (__u64)(long) fl->fl_owner;
> + op->info.owner = (__u64)(long)fl->fl_owner;
>
> send_op(op);
> wait_event(recv_wq, (op->done != 0));
This is the way.
Acked-by: Jeff Layton <jlay...@kernel.org>
