| Message ID | 20220716013140.61445-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/lock: Don't allocate file_lock in flock_make_lock(). | expand |
> On Jul 15, 2022, at 9:31 PM, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > 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 <kuniyu@amazon.com> It looks like a reasonable clean-up. Handful of comments below. Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/locks.c | 57 +++++++++++++++++++----------------------------------- > 1 file changed, 20 insertions(+), 37 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index ca28e0e50e56..db75f4537abc 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -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,14 +2083,18 @@ EXPORT_SYMBOL(locks_lock_inode_wait); > */ > SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > { > - struct fd f = fdget(fd); > - struct file_lock *lock; > - int can_sleep, unlock; > + int can_sleep, unlock, type; > + struct file_lock fl; > + struct fd f; > int error; "struct file_lock" on my system is 216 bytes. That's a lot to allocate on the stack, but there isn't much else there in addition to "struct file_lock", so OK. > - error = -EBADF; > + type = flock_translate_cmd(cmd); > + if (type < 0) > + return type; > + > + f = fdget(fd); > if (!f.file) > - goto out; > + return -EBADF; > > can_sleep = !(cmd & LOCK_NB); > cmd &= ~LOCK_NB; > @@ -2127,32 +2117,25 @@ 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); > - 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); > + can_sleep ? F_SETLKW : F_SETLK, > + &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); > - out: > + > return error; > } > > @@ -2614,7 +2597,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, flock_translate_cmd(LOCK_UN)); We hope the compiler recognizes that passing a constant value through a switch statement means the flock_translate_cmd() call here is reduced to a constant F_UNLCK. It might be slightly easier to read if you explicitly pass F_UNLCK here? Dunno. > fl.fl_flags |= FL_CLOSE; > > if (filp->f_op->flock) > -- > 2.30.2 > -- Chuck Lever
Hi Kuniyuki,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.19-rc6 next-20220715]
[cannot apply to jlayton/linux-next cel-2.6/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/fs-lock-Don-t-allocate-file_lock-in-flock_make_lock/20220716-225519
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9b59ec8d50a1f28747ceff9a4f39af5deba9540e
config: mips-randconfig-r003-20220715 (https://download.01.org/0day-ci/archive/20220717/202207170400.ddCuQ7sy-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/7f68b5b24c3d8d371fb96ebe278dabb8c08bbf51
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kuniyuki-Iwashima/fs-lock-Don-t-allocate-file_lock-in-flock_make_lock/20220716-225519
git checkout 7f68b5b24c3d8d371fb96ebe278dabb8c08bbf51
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/locks.c:2103:6: warning: variable 'error' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/locks.c:2139:9: note: uninitialized use occurs here
return error;
^~~~~
fs/locks.c:2103:2: note: remove the 'if' if its condition is always false
if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/locks.c:2089:11: note: initialize the variable 'error' to silence this warning
int error;
^
= 0
1 warning generated.
vim +2103 fs/locks.c
e55c34a66f87e7 Benjamin Coddington 2015-10-22 2068
^1da177e4c3f41 Linus Torvalds 2005-04-16 2069 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 2070 * sys_flock: - flock() system call.
^1da177e4c3f41 Linus Torvalds 2005-04-16 2071 * @fd: the file descriptor to lock.
^1da177e4c3f41 Linus Torvalds 2005-04-16 2072 * @cmd: the type of lock to apply.
^1da177e4c3f41 Linus Torvalds 2005-04-16 2073 *
^1da177e4c3f41 Linus Torvalds 2005-04-16 2074 * Apply a %FL_FLOCK style lock to an open file descriptor.
80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2075 * The @cmd can be one of:
^1da177e4c3f41 Linus Torvalds 2005-04-16 2076 *
80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2077 * - %LOCK_SH -- a shared lock.
80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2078 * - %LOCK_EX -- an exclusive lock.
80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2079 * - %LOCK_UN -- remove an existing lock.
90f7d7a0d0d686 Jeff Layton 2021-09-10 2080 * - %LOCK_MAND -- a 'mandatory' flock. (DEPRECATED)
^1da177e4c3f41 Linus Torvalds 2005-04-16 2081 *
90f7d7a0d0d686 Jeff Layton 2021-09-10 2082 * %LOCK_MAND support has been removed from the kernel.
^1da177e4c3f41 Linus Torvalds 2005-04-16 2083 */
002c8976ee5377 Heiko Carstens 2009-01-14 2084 SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
^1da177e4c3f41 Linus Torvalds 2005-04-16 2085 {
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2086 int can_sleep, unlock, type;
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2087 struct file_lock fl;
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2088 struct fd f;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2089 int error;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2090
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2091 type = flock_translate_cmd(cmd);
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2092 if (type < 0)
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2093 return type;
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2094
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2095 f = fdget(fd);
2903ff019b346a Al Viro 2012-08-28 2096 if (!f.file)
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2097 return -EBADF;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2098
^1da177e4c3f41 Linus Torvalds 2005-04-16 2099 can_sleep = !(cmd & LOCK_NB);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2100 cmd &= ~LOCK_NB;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2101 unlock = (cmd == LOCK_UN);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2102
90f7d7a0d0d686 Jeff Layton 2021-09-10 @2103 if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^1da177e4c3f41 Linus Torvalds 2005-04-16 2104 goto out_putf;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2105
90f7d7a0d0d686 Jeff Layton 2021-09-10 2106 /*
90f7d7a0d0d686 Jeff Layton 2021-09-10 2107 * LOCK_MAND locks were broken for a long time in that they never
90f7d7a0d0d686 Jeff Layton 2021-09-10 2108 * conflicted with one another and didn't prevent any sort of open,
90f7d7a0d0d686 Jeff Layton 2021-09-10 2109 * read or write activity.
90f7d7a0d0d686 Jeff Layton 2021-09-10 2110 *
90f7d7a0d0d686 Jeff Layton 2021-09-10 2111 * Just ignore these requests now, to preserve legacy behavior, but
90f7d7a0d0d686 Jeff Layton 2021-09-10 2112 * throw a warning to let people know that they don't actually work.
90f7d7a0d0d686 Jeff Layton 2021-09-10 2113 */
90f7d7a0d0d686 Jeff Layton 2021-09-10 2114 if (cmd & LOCK_MAND) {
90f7d7a0d0d686 Jeff Layton 2021-09-10 2115 pr_warn_once("Attempt to set a LOCK_MAND lock via flock(2). This support has been removed and the request ignored.\n");
90f7d7a0d0d686 Jeff Layton 2021-09-10 2116 error = 0;
90f7d7a0d0d686 Jeff Layton 2021-09-10 2117 goto out_putf;
90f7d7a0d0d686 Jeff Layton 2021-09-10 2118 }
90f7d7a0d0d686 Jeff Layton 2021-09-10 2119
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2120 flock_make_lock(f.file, &fl, type);
6e129d00689c4d Jeff Layton 2014-09-04 2121
^1da177e4c3f41 Linus Torvalds 2005-04-16 2122 if (can_sleep)
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2123 fl.fl_flags |= FL_SLEEP;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2124
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2125 error = security_file_lock(f.file, fl.fl_type);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2126 if (error)
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2127 goto out_putf;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2128
de2a4a501e716b Miklos Szeredi 2018-07-18 2129 if (f.file->f_op->flock)
2903ff019b346a Al Viro 2012-08-28 2130 error = f.file->f_op->flock(f.file,
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2131 can_sleep ? F_SETLKW : F_SETLK,
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2132 &fl);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2133 else
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2134 error = locks_lock_file_wait(f.file, &fl);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2135
^1da177e4c3f41 Linus Torvalds 2005-04-16 2136 out_putf:
2903ff019b346a Al Viro 2012-08-28 2137 fdput(f);
7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2138
^1da177e4c3f41 Linus Torvalds 2005-04-16 2139 return error;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2140 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 2141
From: Chuck Lever III <chuck.lever@oracle.com> Date: Sat, 16 Jul 2022 16:18:41 +0000 > > On Jul 15, 2022, at 9:31 PM, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > 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 <kuniyu@amazon.com> > > It looks like a reasonable clean-up. Handful of comments below. > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com> Thank you for reviewing! > > --- > > fs/locks.c | 57 +++++++++++++++++++----------------------------------- > > 1 file changed, 20 insertions(+), 37 deletions(-) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index ca28e0e50e56..db75f4537abc 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -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,14 +2083,18 @@ EXPORT_SYMBOL(locks_lock_inode_wait); > > */ > > SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > > { > > - struct fd f = fdget(fd); > > - struct file_lock *lock; > > - int can_sleep, unlock; > > + int can_sleep, unlock, type; > > + struct file_lock fl; > > + struct fd f; > > int error; > > "struct file_lock" on my system is 216 bytes. That's a lot to > allocate on the stack, but there isn't much else there in > addition to "struct file_lock", so OK. > > > > - error = -EBADF; > > + type = flock_translate_cmd(cmd); > > + if (type < 0) > > + return type; > > + > > + f = fdget(fd); > > if (!f.file) > > - goto out; > > + return -EBADF; > > > > can_sleep = !(cmd & LOCK_NB); > > cmd &= ~LOCK_NB; > > @@ -2127,32 +2117,25 @@ 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); > > - 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); > > + can_sleep ? F_SETLKW : F_SETLK, > > + &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); > > - out: > > + > > return error; > > } > > > > @@ -2614,7 +2597,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, flock_translate_cmd(LOCK_UN)); > > We hope the compiler recognizes that passing a constant value through > a switch statement means the flock_translate_cmd() call here is > reduced to a constant F_UNLCK. It might be slightly easier to read > if you explicitly pass F_UNLCK here? Dunno. My thoughts exactly. I wrote this way because flock_translate_cmd() was called in flock_make_lock(), so I guessed there might be coding conventions like we should try to use uAPI value. I have no strong preference though, if there is no such convention, I like using F_UNLCK directly instead of trusting the compiler. > > fl.fl_flags |= FL_CLOSE; > > > > if (filp->f_op->flock) > > -- > > 2.30.2 > > > > -- > Chuck Lever
From: kernel test robot <lkp@intel.com> Date: Sun, 17 Jul 2022 04:46:27 +0800 > Hi Kuniyuki, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on linus/master] > [also build test WARNING on v5.19-rc6 next-20220715] > [cannot apply to jlayton/linux-next cel-2.6/for-next] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/fs-lock-Don-t-allocate-file_lock-in-flock_make_lock/20220716-225519 > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9b59ec8d50a1f28747ceff9a4f39af5deba9540e > config: mips-randconfig-r003-20220715 (https://download.01.org/0day-ci/archive/20220717/202207170400.ddCuQ7sy-lkp@intel.com/config) > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install mips cross compiling tool for clang build > # apt-get install binutils-mipsel-linux-gnu > # https://github.com/intel-lab-lkp/linux/commit/7f68b5b24c3d8d371fb96ebe278dabb8c08bbf51 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Kuniyuki-Iwashima/fs-lock-Don-t-allocate-file_lock-in-flock_make_lock/20220716-225519 > git checkout 7f68b5b24c3d8d371fb96ebe278dabb8c08bbf51 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > >> fs/locks.c:2103:6: warning: variable 'error' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE))) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Oops, I'll fix this in v2. Thanks! > fs/locks.c:2139:9: note: uninitialized use occurs here > return error; > ^~~~~ > fs/locks.c:2103:2: note: remove the 'if' if its condition is always false > if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE))) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > fs/locks.c:2089:11: note: initialize the variable 'error' to silence this warning > int error; > ^ > = 0 > 1 warning generated. > > > vim +2103 fs/locks.c > > e55c34a66f87e7 Benjamin Coddington 2015-10-22 2068 > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2069 /** > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2070 * sys_flock: - flock() system call. > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2071 * @fd: the file descriptor to lock. > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2072 * @cmd: the type of lock to apply. > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2073 * > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2074 * Apply a %FL_FLOCK style lock to an open file descriptor. > 80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2075 * The @cmd can be one of: > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2076 * > 80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2077 * - %LOCK_SH -- a shared lock. > 80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2078 * - %LOCK_EX -- an exclusive lock. > 80b79dd0e2f29f Mauro Carvalho Chehab 2017-05-27 2079 * - %LOCK_UN -- remove an existing lock. > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2080 * - %LOCK_MAND -- a 'mandatory' flock. (DEPRECATED) > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2081 * > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2082 * %LOCK_MAND support has been removed from the kernel. > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2083 */ > 002c8976ee5377 Heiko Carstens 2009-01-14 2084 SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2085 { > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2086 int can_sleep, unlock, type; > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2087 struct file_lock fl; > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2088 struct fd f; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2089 int error; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2090 > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2091 type = flock_translate_cmd(cmd); > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2092 if (type < 0) > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2093 return type; > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2094 > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2095 f = fdget(fd); > 2903ff019b346a Al Viro 2012-08-28 2096 if (!f.file) > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2097 return -EBADF; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2098 > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2099 can_sleep = !(cmd & LOCK_NB); > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2100 cmd &= ~LOCK_NB; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2101 unlock = (cmd == LOCK_UN); > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2102 > 90f7d7a0d0d686 Jeff Layton 2021-09-10 @2103 if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE))) > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2104 goto out_putf; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2105 > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2106 /* > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2107 * LOCK_MAND locks were broken for a long time in that they never > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2108 * conflicted with one another and didn't prevent any sort of open, > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2109 * read or write activity. > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2110 * > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2111 * Just ignore these requests now, to preserve legacy behavior, but > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2112 * throw a warning to let people know that they don't actually work. > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2113 */ > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2114 if (cmd & LOCK_MAND) { > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2115 pr_warn_once("Attempt to set a LOCK_MAND lock via flock(2). This support has been removed and the request ignored.\n"); > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2116 error = 0; > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2117 goto out_putf; > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2118 } > 90f7d7a0d0d686 Jeff Layton 2021-09-10 2119 > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2120 flock_make_lock(f.file, &fl, type); > 6e129d00689c4d Jeff Layton 2014-09-04 2121 > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2122 if (can_sleep) > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2123 fl.fl_flags |= FL_SLEEP; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2124 > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2125 error = security_file_lock(f.file, fl.fl_type); > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2126 if (error) > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2127 goto out_putf; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2128 > de2a4a501e716b Miklos Szeredi 2018-07-18 2129 if (f.file->f_op->flock) > 2903ff019b346a Al Viro 2012-08-28 2130 error = f.file->f_op->flock(f.file, > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2131 can_sleep ? F_SETLKW : F_SETLK, > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2132 &fl); > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2133 else > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2134 error = locks_lock_file_wait(f.file, &fl); > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2135 > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2136 out_putf: > 2903ff019b346a Al Viro 2012-08-28 2137 fdput(f); > 7f68b5b24c3d8d Kuniyuki Iwashima 2022-07-15 2138 > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2139 return error; > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2140 } > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2141 > > -- > 0-DAY CI Kernel Test Service > https://01.org/lkp
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: 7f68b5b24c3d8d371fb96ebe278dabb8c08bbf51 ("[PATCH] fs/lock: Don't allocate file_lock in flock_make_lock().")
url: https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/fs-lock-Don-t-allocate-file_lock-in-flock_make_lock/20220716-225519
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 9b59ec8d50a1f28747ceff9a4f39af5deba9540e
patch link: https://lore.kernel.org/linux-fsdevel/20220716013140.61445-1-kuniyu@amazon.com
in testcase: nvml
version: nvml-x86_64-3de7d358f-1_20211217
with following parameters:
test: pmem
group: ex
nr_pmem: 1
fs: ext4
mount_option: dax
bp_memmap: 32G!4G
ucode: 0x700001c
on test machine: 16 threads 1 sockets Intel(R) Xeon(R) CPU D-1541 @ 2.10GHz with 48G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <oliver.sang@intel.com>
also found below fails which could pass on parent.
9b59ec8d50a1f287 7f68b5b24c3d8d371fb96ebe278
---------------- ---------------------------
fail:runs %reproduction fail:runs
| | |
:6 100% 6:6 nvml.ex_libpmemblk_TEST0_check_pmem_debug.fail
:6 100% 6:6 nvml.ex_libpmemlog_TEST0_check_pmem_debug.fail
:6 100% 6:6 nvml.ex_libpmemobj_TEST0_check_pmem_debug.fail
:6 100% 6:6 nvml.ex_linkedlist_TEST0_check_pmem_debug.fail
2022-07-17 16:07:44 ./RUNTESTS -f pmem ex_libpmemblk
ex_libpmemblk/TEST0: SETUP (check/pmem/debug)
RUNTESTS: stopping: ex_libpmemblk/TEST0 failed, TEST=check FS=pmem BUILD=debug
2022-07-17 16:07:44 ./RUNTESTS -f pmem ex_libpmemlog
ex_libpmemlog/TEST0: SETUP (check/pmem/debug)
RUNTESTS: stopping: ex_libpmemlog/TEST0 failed, TEST=check FS=pmem BUILD=debug
2022-07-17 16:07:44 ./RUNTESTS -f pmem ex_libpmemobj
ex_libpmemobj/TEST0: SETUP (check/pmem/debug)
RUNTESTS: stopping: ex_libpmemobj/TEST0 failed, TEST=check FS=pmem BUILD=debug
...
2022-07-17 16:07:45 ./RUNTESTS -f pmem ex_linkedlist
ex_linkedlist/TEST0: SETUP (check/pmem/debug)
ex_linkedlist/TEST0 crashed (signal 6). err0.log below.
{ex_linkedlist.c:248 main} ex_linkedlist/TEST0: Error: pmemobj_create: /fs/pmem0//test_ex_linkedlist0
diff --git a/fs/locks.c b/fs/locks.c index ca28e0e50e56..db75f4537abc 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -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,14 +2083,18 @@ EXPORT_SYMBOL(locks_lock_inode_wait); */ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) { - struct fd f = fdget(fd); - struct file_lock *lock; - int can_sleep, unlock; + int can_sleep, unlock, type; + struct file_lock fl; + struct fd f; int error; - error = -EBADF; + type = flock_translate_cmd(cmd); + if (type < 0) + return type; + + f = fdget(fd); if (!f.file) - goto out; + return -EBADF; can_sleep = !(cmd & LOCK_NB); cmd &= ~LOCK_NB; @@ -2127,32 +2117,25 @@ 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); - 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); + can_sleep ? F_SETLKW : F_SETLK, + &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); - out: + return error; } @@ -2614,7 +2597,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, flock_translate_cmd(LOCK_UN)); fl.fl_flags |= FL_CLOSE; if (filp->f_op->flock)
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 <kuniyu@amazon.com> --- fs/locks.c | 57 +++++++++++++++++++----------------------------------- 1 file changed, 20 insertions(+), 37 deletions(-)