fs/lock: Cleanup around flock syscall.

PATCH Superseded Kuniyuki Iwashima 1 participant 4 messages Jul 16, 2022 – Jul 17, 2022

Subsystems (2)

VFS File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Kuniyuki Iwashima

author

2022-07-16 23:34 UTC #0 lore raw

v2 cover fs/lock: Cleanup around flock syscall.

The first patch removes allocate-and-free for struct file_lock in flock syscall and the second patch rearrange some operations.

Changes
v2:
* Use F_UNLCK in locks_remove_flock() (Chuck Lever)
* Fix uninitialised error in flock syscall (kernel test robot)
* Fix error when setting LOCK_NB
* Split patches not to mix different kinds of optimisations and not to miss errors reported by kernel test robot

v1: https://lore.kernel.org/linux-fsdevel/[email protected]/

Kuniyuki Iwashima (2):
fs/lock: Don't allocate file_lock in flock_make_lock(). fs/lock: Rearrange ops in flock syscall.

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

Signature · 1 line

2.30.2

Kuniyuki Iwashima

author

2022-07-16 23:34 UTC #1 lore raw

PATCH v2 1/2 fs/lock: Don't allocate file_lock in flock_make_lock().

Two functions, flock syscall and locks_remove_flock(), call flock_make_lock(). It allocates struct file_lock from slab cache if its argument fl is NULL.

When we call flock syscall, we pass NULL to allocate memory for struct file_lock. However, we always free it at the end by locks_free_lock(). We need not allocate it and instead should use a local variable as locks_remove_flock() does.

Also, the validation for flock_translate_cmd() is not necessary for locks_remove_flock(). So we move the part to flock syscall and make flock_make_lock() return nothing.

Signed-off-by Kuniyuki Iwashima <[email protected]>

Reviewed-by Chuck Lever <[email protected]>

---
 fs/locks.c | 46 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)

fs/locks.c +15 ~~-31~~

index ca28e0e50e56..b134eaefd7d6 100644
@@ -425,21 +425,9 @@ static inline int flock_translate_cmd(int cmd) {
 }
 
 /* Fill in a file_lock structure with an appropriate FLOCK lock. */
-static struct file_lock *
-flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
+static void flock_make_lock(struct file *filp, struct file_lock *fl, int type)
 {
-	int type = flock_translate_cmd(cmd);
-
-	if (type < 0)
-		return ERR_PTR(type);
-
-	if (fl == NULL) {
-		fl = locks_alloc_lock();
-		if (fl == NULL)
-			return ERR_PTR(-ENOMEM);
-	} else {
-		locks_init_lock(fl);
-	}
+	locks_init_lock(fl);
 
 	fl->fl_file = filp;
 	fl->fl_owner = filp;

@@ -447,8 +435,6 @@ flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
 	fl->fl_flags = FL_FLOCK;
 	fl->fl_type = type;
 	fl->fl_end = OFFSET_MAX;
-
-	return fl;
 }
 
 static int assign_type(struct file_lock *fl, long type)

@@ -2097,10 +2083,9 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
  */
 SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 {
+	int can_sleep, error, unlock, type;
 	struct fd f = fdget(fd);
-	struct file_lock *lock;
-	int can_sleep, unlock;
-	int error;
+	struct file_lock fl;
 
 	error = -EBADF;
 	if (!f.file)

@@ -2127,28 +2112,27 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 		goto out_putf;
 	}
 
-	lock = flock_make_lock(f.file, cmd, NULL);
-	if (IS_ERR(lock)) {
-		error = PTR_ERR(lock);
+	type = flock_translate_cmd(cmd);
+	if (type < 0) {
+		error = type;
 		goto out_putf;
 	}
 
+	flock_make_lock(f.file, &fl, type);
+
 	if (can_sleep)
-		lock->fl_flags |= FL_SLEEP;
+		fl.fl_flags |= FL_SLEEP;
 
-	error = security_file_lock(f.file, lock->fl_type);
+	error = security_file_lock(f.file, fl.fl_type);
 	if (error)
-		goto out_free;
+		goto out_putf;
 
 	if (f.file->f_op->flock)
 		error = f.file->f_op->flock(f.file,
 					  (can_sleep) ? F_SETLKW : F_SETLK,
-					  lock);
+					    &fl);
 	else
-		error = locks_lock_file_wait(f.file, lock);
-
- out_free:
-	locks_free_lock(lock);
+		error = locks_lock_file_wait(f.file, &fl);
 
  out_putf:
 	fdput(f);

@@ -2614,7 +2598,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 	if (list_empty(&flctx->flc_flock))
 		return;
 
-	flock_make_lock(filp, LOCK_UN, &fl);
+	flock_make_lock(filp, &fl, F_UNLCK);
 	fl.fl_flags |= FL_CLOSE;
 
 	if (filp->f_op->flock)

Signature · 1 line

2.30.2

Kuniyuki Iwashima

author

2022-07-16 23:34 UTC #2 lore raw

PATCH v2 2/2 fs/lock: Rearrange ops in flock syscall.

In reply to Kuniyuki Iwashima

The previous patch added flock_translate_cmd() in flock syscall. The test does not depend on struct fd and the cheapest, so we can put it at the top and defer fdget() after that.

Also, we can remove the unlock variable and use type instead. As a bonus, we fix this checkpatch error.

CHECK: spaces preferred around that '|' (ctx:VxV) #45: FILE: fs/locks.c:2099:
+ if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^

Finally, we can move the can_sleep part just before we use it.

Signed-off-by Kuniyuki Iwashima <[email protected]>

---
 fs/locks.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

fs/locks.c +15 ~~-19~~

index b134eaefd7d6..97581678c4d4 100644
@@ -2083,19 +2083,20 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
  */
 SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 {
-	int can_sleep, error, unlock, type;
-	struct fd f = fdget(fd);
+	int can_sleep, error, type;
 	struct file_lock fl;
+	struct fd f;
+
+	type = flock_translate_cmd(cmd & ~LOCK_NB);
+	if (type < 0)
+		return type;
 
 	error = -EBADF;
+	f = fdget(fd);
 	if (!f.file)
-		goto out;
-
-	can_sleep = !(cmd & LOCK_NB);
-	cmd &= ~LOCK_NB;
-	unlock = (cmd == LOCK_UN);
+		return error;
 
-	if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
+	if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ | FMODE_WRITE)))
 		goto out_putf;
 
 	/*

@@ -2112,31 +2113,26 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 		goto out_putf;
 	}
 
-	type = flock_translate_cmd(cmd);
-	if (type < 0) {
-		error = type;
-		goto out_putf;
-	}
-
 	flock_make_lock(f.file, &fl, type);
 
-	if (can_sleep)
-		fl.fl_flags |= FL_SLEEP;
-
 	error = security_file_lock(f.file, fl.fl_type);
 	if (error)
 		goto out_putf;
 
+	can_sleep = !(cmd & LOCK_NB);
+	if (can_sleep)
+		fl.fl_flags |= FL_SLEEP;
+
 	if (f.file->f_op->flock)
 		error = f.file->f_op->flock(f.file,
-					  (can_sleep) ? F_SETLKW : F_SETLK,
+					    (can_sleep) ? F_SETLKW : F_SETLK,
 					    &fl);
 	else
 		error = locks_lock_file_wait(f.file, &fl);
 
  out_putf:
 	fdput(f);
- out:
+
 	return error;
 }

Signature · 1 line

2.30.2

Kuniyuki Iwashima

author

2022-07-17 04:12 UTC #3 lore raw

Re: [PATCH v2 2/2] fs/lock: Rearrange ops in flock syscall.

From:   Kuniyuki Iwashima <[email protected]>
Date:   Sat, 16 Jul 2022 16:33:43 -0700

Quoted 26 lines

The previous patch added flock_translate_cmd() in flock syscall. The test does not depend on struct fd and the cheapest, so we can put it at the top and defer fdget() after that.
Also, we can remove the unlock variable and use type instead. As a bonus, we fix this checkpatch error.
CHECK: spaces preferred around that '|' (ctx:VxV) #45: FILE: fs/locks.c:2099:
+ if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^
Finally, we can move the can_sleep part just before we use it.
Signed-off-by Kuniyuki Iwashima <[email protected]>
---
 fs/locks.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)
fs/locks.c +0 -0
index b134eaefd7d6..97581678c4d4 100644
@@ -2083,19 +2083,20 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
  */
 SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)

fs/locks.c EXPORT_SYMBOL(locks_lock_inode_wait); · line 2092

Quoted 10 lines

fs/locks.c +6 -2

 {
-	int can_sleep, error, unlock, type;
-	struct fd f = fdget(fd);
+	int can_sleep, error, type;
 	struct file_lock fl;
+	struct fd f;
+
+	type = flock_translate_cmd(cmd & ~LOCK_NB);
+	if (type < 0)
+		return type;

Sorry, we have to check (cmd & LOCK_MAND) first.
Will fix in v3.

Quoted 56 lines

fs/locks.c +9 ~~-17~~

 	error = -EBADF;
+	f = fdget(fd);
 	if (!f.file)
-		goto out;
-
-	can_sleep = !(cmd & LOCK_NB);
-	cmd &= ~LOCK_NB;
-	unlock = (cmd == LOCK_UN);
+		return error;
 
-	if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
+	if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ | FMODE_WRITE)))
 		goto out_putf;
 
 	/*

@@ -2112,31 +2113,26 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
 		goto out_putf;
 	}
 
-	type = flock_translate_cmd(cmd);
-	if (type < 0) {
-		error = type;
-		goto out_putf;
-	}
-
 	flock_make_lock(f.file, &fl, type);
 
-	if (can_sleep)
-		fl.fl_flags |= FL_SLEEP;
-
 	error = security_file_lock(f.file, fl.fl_type);
 	if (error)
 		goto out_putf;
 
+	can_sleep = !(cmd & LOCK_NB);
+	if (can_sleep)
+		fl.fl_flags |= FL_SLEEP;
+
 	if (f.file->f_op->flock)
 		error = f.file->f_op->flock(f.file,
-					  (can_sleep) ? F_SETLKW : F_SETLK,
+					    (can_sleep) ? F_SETLKW : F_SETLK,
 					    &fl);
 	else
 		error = locks_lock_file_wait(f.file, &fl);
 
  out_putf:
 	fdput(f);
- out:
+
 	return error;
 }

Signature · 1 line

2.30.2