Some of the exported functions in fs/locks.c take both a struct file
argument and a struct file_lock. struct file_lock has a dedicated field
to record which file it was set on (fl_file). This is redundant, and
there have been some cases where the two didn't match [1], leading to
bugs.
This patchset is intended to remove this ambiguity by eliminating the
separate struct file argument from vfs_lock_file, vfs_test_lock and
vfs_cancel_lock.
Most callers are easy to vet to ensure that they set this correctly, but
lockd had a few places where it wasn't doing the right thing. This
series depends on the lockd patches I sent late last week [2].
I'm targeting this series for v6.3. I'll plan to get it into linux-next
soon unless there are objections.
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=216582
[2]: https://lore.kernel.org/linux-nfs/[email protected]/T/#t
Jeff Layton (3):
filelock: remove redundant filp argument from vfs_lock_file
filelock: remove redundant filp argument from vfs_test_lock
filelock: remove redundant filp arg from vfs_cancel_lock
fs/ksmbd/smb2pdu.c | 4 ++--
fs/lockd/svclock.c | 21 +++++++--------------
fs/lockd/svcsubs.c | 4 ++--
fs/locks.c | 29 ++++++++++++++---------------
fs/nfsd/nfs4state.c | 6 +++---
include/linux/fs.h | 14 +++++++-------
6 files changed, 35 insertions(+), 43 deletions(-)
--
2.38.1
The existing API requires that the fl_file field be filled out when
calling it, as some underlying filesystems require that information
deep down in their call stacks.
Simplify vfs_lock_file by removing the redundant @filp argument and
using fl_file in its place.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/ksmbd/smb2pdu.c | 4 ++--
fs/lockd/svclock.c | 13 +++++--------
fs/lockd/svcsubs.c | 4 ++--
fs/locks.c | 13 ++++++-------
fs/nfsd/nfs4state.c | 4 ++--
include/linux/fs.h | 6 +++---
6 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index f2bcd2a5fb7f..4668553be5e3 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7007,7 +7007,7 @@ int smb2_lock(struct ksmbd_work *work)
flock = smb_lock->fl;
list_del(&smb_lock->llist);
retry:
- rc = vfs_lock_file(filp, smb_lock->cmd, flock, NULL);
+ rc = vfs_lock_file(smb_lock->cmd, flock, NULL);
skip:
if (flags & SMB2_LOCKFLAG_UNLOCK) {
if (!rc) {
@@ -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, F_SETLK, rlock, NULL);
+ rc = vfs_lock_file(F_SETLK, rlock, NULL);
if (rc)
pr_err("rollback unlock fail : %d\n", rc);
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 4e30f3c50970..c965783b71a6 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -475,7 +475,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
#endif
struct nlm_block *block = NULL;
int error;
- int mode;
int async_block = 0;
__be32 ret;
@@ -534,8 +533,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
if (!wait)
lock->fl.fl_flags &= ~FL_SLEEP;
- mode = lock_to_openmode(&lock->fl);
- error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
+ error = vfs_lock_file(F_SETLK, &lock->fl, NULL);
lock->fl.fl_flags &= ~FL_SLEEP;
dprintk("lockd: vfs_lock_file returned %d\n", error);
@@ -661,12 +659,10 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
lock->fl.fl_type = F_UNLCK;
lock->fl.fl_file = file->f_file[O_RDONLY];
if (lock->fl.fl_file)
- error = vfs_lock_file(lock->fl.fl_file, F_SETLK,
- &lock->fl, NULL);
+ error = vfs_lock_file(F_SETLK, &lock->fl, NULL);
lock->fl.fl_file = file->f_file[O_WRONLY];
if (lock->fl.fl_file)
- error |= vfs_lock_file(lock->fl.fl_file, F_SETLK,
- &lock->fl, NULL);
+ error |= vfs_lock_file(F_SETLK, &lock->fl, NULL);
return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
}
@@ -845,7 +841,8 @@ nlmsvc_grant_blocked(struct nlm_block *block)
fl_start = lock->fl.fl_start;
fl_end = lock->fl.fl_end;
mode = lock_to_openmode(&lock->fl);
- error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
+ WARN_ON_ONCE(lock->fl.fl_file != file->f_file[mode]);
+ error = vfs_lock_file(F_SETLK, &lock->fl, NULL);
lock->fl.fl_flags &= ~FL_SLEEP;
lock->fl.fl_start = fl_start;
lock->fl.fl_end = fl_end;
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 3515f17eaf3f..33842d67daa7 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -189,10 +189,10 @@ static int nlm_unlock_files(struct nlm_file *file, const struct file_lock *fl)
lock.fl_flags = FL_POSIX;
lock.fl_file = file->f_file[O_RDONLY];
- if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
+ if (lock.fl_file && vfs_lock_file(F_SETLK, &lock, NULL))
goto out_err;
lock.fl_file = file->f_file[O_WRONLY];
- if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
+ if (lock.fl_file && vfs_lock_file(F_SETLK, &lock, NULL))
goto out_err;
return 0;
out_err:
diff --git a/fs/locks.c b/fs/locks.c
index b429d614316b..a38845633f73 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2263,7 +2263,6 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
/**
* vfs_lock_file - file byte range lock
- * @filp: The file to apply the lock to
* @cmd: type of locking operation (F_SETLK, F_GETLK, etc.)
* @fl: The lock to be applied
* @conf: Place to return a copy of the conflicting lock, if found.
@@ -2294,13 +2293,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
* ->lm_grant() before returning to the caller with a FILE_LOCK_DEFERRED
* return code.
*/
-int vfs_lock_file(struct file *filp, unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
+int vfs_lock_file(unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
{
- WARN_ON_ONCE(filp != fl->fl_file);
+ struct file *filp = fl->fl_file;
+
if (filp->f_op->lock)
return filp->f_op->lock(filp, cmd, fl);
- else
- return posix_lock_file(filp, fl, conf);
+ return posix_lock_file(filp, fl, conf);
}
EXPORT_SYMBOL_GPL(vfs_lock_file);
@@ -2314,7 +2313,7 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd,
return error;
for (;;) {
- error = vfs_lock_file(filp, cmd, fl, NULL);
+ error = vfs_lock_file(cmd, fl, NULL);
if (error != FILE_LOCK_DEFERRED)
break;
error = wait_event_interruptible(fl->fl_wait,
@@ -2578,7 +2577,7 @@ void locks_remove_posix(struct file *filp, fl_owner_t owner)
lock.fl_ops = NULL;
lock.fl_lmops = NULL;
- error = vfs_lock_file(filp, F_SETLK, &lock, NULL);
+ error = vfs_lock_file(F_SETLK, &lock, NULL);
if (lock.fl_ops && lock.fl_ops->fl_release_private)
lock.fl_ops->fl_release_private(&lock);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 836bd825ca4a..eab75ba9f44d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7448,7 +7448,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
spin_unlock(&nn->blocked_locks_lock);
}
- err = vfs_lock_file(nf->nf_file, F_SETLK, file_lock, conflock);
+ err = vfs_lock_file(F_SETLK, file_lock, conflock);
switch (err) {
case 0: /* success! */
nfs4_inc_and_copy_stateid(&lock->lk_resp_stateid, &lock_stp->st_stid);
@@ -7670,7 +7670,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
locku->lu_length);
nfs4_transform_lock_offset(file_lock);
- err = vfs_lock_file(nf->nf_file, F_SETLK, file_lock, NULL);
+ err = vfs_lock_file(F_SETLK, file_lock, NULL);
if (err) {
dprintk("NFSD: nfs4_locku: vfs_lock_file failed!\n");
goto out_nfserr;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e4d0f1fa7f9f..57b6eed9a44d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1168,7 +1168,7 @@ extern void posix_test_lock(struct file *, struct file_lock *);
extern int posix_lock_file(struct file *, struct file_lock *, struct file_lock *);
extern int locks_delete_block(struct file_lock *);
extern int vfs_test_lock(struct file *, struct file_lock *);
-extern int vfs_lock_file(struct file *, unsigned int, struct file_lock *, struct file_lock *);
+extern int vfs_lock_file(unsigned int, struct file_lock *, struct file_lock *);
extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
bool vfs_file_has_locks(struct file *file);
extern int locks_lock_inode_wait(struct inode *inode, struct file_lock *fl);
@@ -1274,8 +1274,8 @@ static inline int vfs_test_lock(struct file *filp, struct file_lock *fl)
return 0;
}
-static inline int vfs_lock_file(struct file *filp, unsigned int cmd,
- struct file_lock *fl, struct file_lock *conf)
+static inline int vfs_lock_file(unsigned int cmd, struct file_lock *fl,
+ struct file_lock *conf)
{
return -ENOLCK;
}
--
2.38.1
struct file_lock already has a fl_file field that must be populated, so
the @filp argument to this function is redundant. Remove it and use
fl_file instead.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/lockd/svclock.c | 4 +---
fs/locks.c | 10 +++++-----
fs/nfsd/nfs4state.c | 2 +-
include/linux/fs.h | 4 ++--
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index c965783b71a6..21ee6b1c4d9e 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -586,7 +586,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
struct nlm_lock *conflock, struct nlm_cookie *cookie)
{
int error;
- int mode;
__be32 ret;
dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
@@ -601,8 +600,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
goto out;
}
- mode = lock_to_openmode(&lock->fl);
- error = vfs_test_lock(file->f_file[mode], &lock->fl);
+ error = vfs_test_lock(&lock->fl);
if (error) {
/* We can't currently deal with deferred test requests */
if (error == FILE_LOCK_DEFERRED)
diff --git a/fs/locks.c b/fs/locks.c
index a38845633f73..0bc1808f7d98 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2138,15 +2138,15 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
/**
* vfs_test_lock - test file byte range lock
- * @filp: The file to test lock for
* @fl: The lock to test; also used to hold result
*
* Returns -ERRNO on failure. Indicates presence of conflicting lock by
* setting conf->fl_type to something other than F_UNLCK.
*/
-int vfs_test_lock(struct file *filp, struct file_lock *fl)
+int vfs_test_lock(struct file_lock *fl)
{
- WARN_ON_ONCE(filp != fl->fl_file);
+ struct file *filp = fl->fl_file;
+
if (filp->f_op->lock)
return filp->f_op->lock(filp, F_GETLK, fl);
posix_test_lock(filp, fl);
@@ -2246,7 +2246,7 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
fl->fl_owner = filp;
}
- error = vfs_test_lock(filp, fl);
+ error = vfs_test_lock(fl);
if (error)
goto out;
@@ -2453,7 +2453,7 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
fl->fl_owner = filp;
}
- error = vfs_test_lock(filp, fl);
+ error = vfs_test_lock(fl);
if (error)
goto out;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index eab75ba9f44d..beec9da50016 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7537,7 +7537,7 @@ static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct
if (err)
goto out;
lock->fl_file = nf->nf_file;
- err = nfserrno(vfs_test_lock(nf->nf_file, lock));
+ err = nfserrno(vfs_test_lock(lock));
lock->fl_file = NULL;
out:
inode_unlock(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 57b6eed9a44d..507fa1a61bb5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1167,7 +1167,7 @@ extern void locks_release_private(struct file_lock *);
extern void posix_test_lock(struct file *, struct file_lock *);
extern int posix_lock_file(struct file *, struct file_lock *, struct file_lock *);
extern int locks_delete_block(struct file_lock *);
-extern int vfs_test_lock(struct file *, struct file_lock *);
+extern int vfs_test_lock(struct file_lock *);
extern int vfs_lock_file(unsigned int, struct file_lock *, struct file_lock *);
extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
bool vfs_file_has_locks(struct file *file);
@@ -1269,7 +1269,7 @@ static inline int locks_delete_block(struct file_lock *waiter)
return -ENOENT;
}
-static inline int vfs_test_lock(struct file *filp, struct file_lock *fl)
+static inline int vfs_test_lock(struct file_lock *fl)
{
return 0;
}
--
2.38.1
struct file_lock already has a fl_file field that must be populated, so
the @filp argument to this function is redundant. Remove it and use
fl_file instead.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/lockd/svclock.c | 4 +---
fs/locks.c | 6 +++---
include/linux/fs.h | 4 ++--
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 21ee6b1c4d9e..2bced428b078 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -677,7 +677,6 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
{
struct nlm_block *block;
int status = 0;
- int mode;
dprintk("lockd: nlmsvc_cancel(%s/%ld, pi=%d, %Ld-%Ld)\n",
nlmsvc_file_inode(file)->i_sb->s_id,
@@ -695,8 +694,7 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
if (block != NULL) {
struct file_lock *fl = &block->b_call->a_args.lock.fl;
- mode = lock_to_openmode(fl);
- vfs_cancel_lock(block->b_file->f_file[mode], fl);
+ vfs_cancel_lock(fl);
status = nlmsvc_unlink_block(block);
nlmsvc_release_block(block);
}
diff --git a/fs/locks.c b/fs/locks.c
index 0bc1808f7d98..64eeb4002bbb 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2657,14 +2657,14 @@ void locks_remove_file(struct file *filp)
/**
* vfs_cancel_lock - file byte range unblock lock
- * @filp: The file to apply the unblock to
* @fl: The lock to be unblocked
*
* Used by lock managers to cancel blocked requests
*/
-int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
+int vfs_cancel_lock(struct file_lock *fl)
{
- WARN_ON_ONCE(filp != fl->fl_file);
+ struct file *filp = fl->fl_file;
+
if (filp->f_op->lock)
return filp->f_op->lock(filp, F_CANCELLK, fl);
return 0;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 507fa1a61bb5..d5da4c448cd8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1169,7 +1169,7 @@ extern int posix_lock_file(struct file *, struct file_lock *, struct file_lock *
extern int locks_delete_block(struct file_lock *);
extern int vfs_test_lock(struct file_lock *);
extern int vfs_lock_file(unsigned int, struct file_lock *, struct file_lock *);
-extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
+extern int vfs_cancel_lock(struct file_lock *fl);
bool vfs_file_has_locks(struct file *file);
extern int locks_lock_inode_wait(struct inode *inode, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
@@ -1280,7 +1280,7 @@ static inline int vfs_lock_file(unsigned int cmd, struct file_lock *fl,
return -ENOLCK;
}
-static inline int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
+static inline int vfs_cancel_lock(struct file_lock *fl)
{
return 0;
}
--
2.38.1
> On Nov 14, 2022, at 10:02 AM, Jeff Layton <[email protected]> wrote:
>
> Some of the exported functions in fs/locks.c take both a struct file
> argument and a struct file_lock. struct file_lock has a dedicated field
> to record which file it was set on (fl_file). This is redundant, and
> there have been some cases where the two didn't match [1], leading to
> bugs.
Hi Jeff, doesn't the same argument apply to f_ops->lock ? Do you
have a plan for updating that API as well?
> This patchset is intended to remove this ambiguity by eliminating the
> separate struct file argument from vfs_lock_file, vfs_test_lock and
> vfs_cancel_lock.
>
> Most callers are easy to vet to ensure that they set this correctly, but
> lockd had a few places where it wasn't doing the right thing. This
> series depends on the lockd patches I sent late last week [2].
>
> I'm targeting this series for v6.3. I'll plan to get it into linux-next
> soon unless there are objections.
>
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=216582
> [2]: https://lore.kernel.org/linux-nfs/[email protected]/T/#t
>
> Jeff Layton (3):
> filelock: remove redundant filp argument from vfs_lock_file
> filelock: remove redundant filp argument from vfs_test_lock
> filelock: remove redundant filp arg from vfs_cancel_lock
>
> fs/ksmbd/smb2pdu.c | 4 ++--
> fs/lockd/svclock.c | 21 +++++++--------------
> fs/lockd/svcsubs.c | 4 ++--
> fs/locks.c | 29 ++++++++++++++---------------
> fs/nfsd/nfs4state.c | 6 +++---
> include/linux/fs.h | 14 +++++++-------
> 6 files changed, 35 insertions(+), 43 deletions(-)
>
> --
> 2.38.1
>
--
Chuck Lever
On Mon, 2022-11-14 at 15:08 +0000, Chuck Lever III wrote:
>
> > On Nov 14, 2022, at 10:02 AM, Jeff Layton <[email protected]> wrote:
> >
> > Some of the exported functions in fs/locks.c take both a struct file
> > argument and a struct file_lock. struct file_lock has a dedicated field
> > to record which file it was set on (fl_file). This is redundant, and
> > there have been some cases where the two didn't match [1], leading to
> > bugs.
>
> Hi Jeff, doesn't the same argument apply to f_ops->lock ? Do you
> have a plan for updating that API as well?
>
>
It does apply to fops->lock. I don't have a real plan as of yet. I
figure we'll get this set in first and then we can look at changing that
API as well.
> > This patchset is intended to remove this ambiguity by eliminating the
> > separate struct file argument from vfs_lock_file, vfs_test_lock and
> > vfs_cancel_lock.
> >
> > Most callers are easy to vet to ensure that they set this correctly, but
> > lockd had a few places where it wasn't doing the right thing. This
> > series depends on the lockd patches I sent late last week [2].
> >
> > I'm targeting this series for v6.3. I'll plan to get it into linux-next
> > soon unless there are objections.
> >
> > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=216582
> > [2]: https://lore.kernel.org/linux-nfs/[email protected]/T/#t
> >
> > Jeff Layton (3):
> > filelock: remove redundant filp argument from vfs_lock_file
> > filelock: remove redundant filp argument from vfs_test_lock
> > filelock: remove redundant filp arg from vfs_cancel_lock
> >
> > fs/ksmbd/smb2pdu.c | 4 ++--
> > fs/lockd/svclock.c | 21 +++++++--------------
> > fs/lockd/svcsubs.c | 4 ++--
> > fs/locks.c | 29 ++++++++++++++---------------
> > fs/nfsd/nfs4state.c | 6 +++---
> > include/linux/fs.h | 14 +++++++-------
> > 6 files changed, 35 insertions(+), 43 deletions(-)
> >
> > --
> > 2.38.1
> >
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
On Mon, Nov 14, 2022 at 10:02:38AM -0500, Jeff Layton wrote:
> -int vfs_lock_file(struct file *filp, unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
> +int vfs_lock_file(unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
I'd pass fl as the first argument for a saner argument order here.
Also can you please break the line at 80 characters? The previous
version is insanely unreadable, and the new one just slightly less
so.
> +extern int vfs_lock_file(unsigned int, struct file_lock *, struct file_lock *);
And please drop the pointless extern here.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Same nitpick once more :)
Otherwise:
Reviewed-by: Christoph Hellwig <[email protected]>
Same nitpck about the extern as for the last one, otherwise:
Reviewed-by: Christoph Hellwig <[email protected]>