[PATCH] ksmbd: use F_SETLK when unlocking a file

[PATCH] ksmbd: use F_SETLK when unlocking a file

[Jeff Layton]

ksmbd seems to be trying to use a cmd value of 0 when unlocking a file.
That activity requires a type of F_UNLCK with a cmd of F_SETLK. For
local POSIX locking, it doesn't matter much since vfs_lock_file ignores
@cmd, but filesystems that define their own ->lock operation expect to
see it set sanely.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index b2fc85d440d0..f2bcd2a5fb7f 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -6751,7 +6751,7 @@ static int smb2_set_flock_flags(struct file_lock *flock, int flags)
 	case SMB2_LOCKFLAG_UNLOCK:
 		ksmbd_debug(SMB, "received unlock request\n");
 		flock->fl_type = F_UNLCK;
-		cmd = 0;
+		cmd = F_SETLK;
 		break;
 	}
 
@@ -7129,7 +7129,7 @@ int smb2_lock(struct ksmbd_work *work)
 		rlock->fl_start = smb_lock->start;
 		rlock->fl_end = smb_lock->end;
 
-		rc = vfs_lock_file(filp, 0, rlock, NULL);
+		rc = vfs_lock_file(filp, F_SETLK, rlock, NULL);
 		if (rc)
 			pr_err("rollback unlock fail : %d\n", rc);
 
-- 
2.38.1

Re: [PATCH] ksmbd: use F_SETLK when unlocking a file

[Namjae Jeon]

2022-11-11 22:11 GMT+09:00, Jeff Layton <jlayton@kernel.org>:
> ksmbd seems to be trying to use a cmd value of 0 when unlocking a file.
> That activity requires a type of F_UNLCK with a cmd of F_SETLK. For
> local POSIX locking, it doesn't matter much since vfs_lock_file ignores
> @cmd, but filesystems that define their own ->lock operation expect to
> see it set sanely.
>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks for your patch.

Re: [PATCH] ksmbd: use F_SETLK when unlocking a file

[David Howells]

Jeff Layton <jlayton@kernel.org> wrote:

> ksmbd seems to be trying to use a cmd value of 0 when unlocking a file.
> That activity requires a type of F_UNLCK with a cmd of F_SETLK. For
> local POSIX locking, it doesn't matter much since vfs_lock_file ignores
> @cmd, but filesystems that define their own ->lock operation expect to
> see it set sanely.
> 
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Reviewed-by: David Howells <dhowells@redhat.com>

Re: [PATCH] ksmbd: use F_SETLK when unlocking a file

[Christoph Hellwig]

On Fri, Nov 11, 2022 at 08:11:53AM -0500, Jeff Layton wrote:
> ksmbd seems to be trying to use a cmd value of 0 when unlocking a file.
> That activity requires a type of F_UNLCK with a cmd of F_SETLK. For
> local POSIX locking, it doesn't matter much since vfs_lock_file ignores
> @cmd, but filesystems that define their own ->lock operation expect to
> see it set sanely.

Btw, I really wonder if we should split vfs_lock_file into separate
calls for locking vs unlocking.  The current interface seems very
confusing.

Re: [PATCH] ksmbd: use F_SETLK when unlocking a file

[Jeff Layton]

On Tue, 2022-11-15 at 01:01 -0800, Christoph Hellwig wrote:
> On Fri, Nov 11, 2022 at 08:11:53AM -0500, Jeff Layton wrote:
> > ksmbd seems to be trying to use a cmd value of 0 when unlocking a file.
> > That activity requires a type of F_UNLCK with a cmd of F_SETLK. For
> > local POSIX locking, it doesn't matter much since vfs_lock_file ignores
> > @cmd, but filesystems that define their own ->lock operation expect to
> > see it set sanely.
> 
> Btw, I really wonder if we should split vfs_lock_file into separate
> calls for locking vs unlocking.  The current interface seems very
> confusing.

Maybe, though the current scheme basically of mirrors the userland API,
as do the ->lock and ->flock file_operations.

FWIW, the filelocking API is pretty rife with warts. Several other
things that I wouldn't mind doing, just off the top of my head:

- move the file locking API into a separate header. No need for it to be
in fs.h, which is already too bloated.

- define a new struct for leases, and drop lease-specific fields from
file_lock

- remove more separate filp and inode arguments

- maybe rename locks.c to filelock.c? "locks.c" is too ambiguous

Any others?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ksmbd: use F_SETLK when unlocking a file

[Christoph Hellwig]

On Tue, Nov 15, 2022 at 10:22:42AM -0500, Jeff Layton wrote:
> Maybe, though the current scheme basically of mirrors the userland API,
> as do the ->lock and ->flock file_operations.

Yes.  But the userland API is pretty horrible and the file_operations
should go along with any locks API change.

> FWIW, the filelocking API is pretty rife with warts. Several other
> things that I wouldn't mind doing, just off the top of my head:
> 
> - move the file locking API into a separate header. No need for it to be
> in fs.h, which is already too bloated.
> 
> - define a new struct for leases, and drop lease-specific fields from
> file_lock
> 
> - remove more separate filp and inode arguments
> 
> - maybe rename locks.c to filelock.c? "locks.c" is too ambiguous

These all sounds pretty reasonable to me.