On Wed, Jun 08, 2022 at 09:19:25AM -0400, Benjamin Coddington wrote:
> NLM sometimes gets burnt by implementations of f_op->lock for F_GETLK
> modifying the lock structure (swapping out fl_owner) when the return is
> F_UNLCK.
>
> Yes, NLM should be more defensive, but perhaps we should be checking for
> everyone, as per POSIX "If no lock is found that would prevent this lock
> from being created, then the structure shall be left unchanged
> except for
> the lock type which shall be set to F_UNLCK."
Doesn't seem like changing fl_owner affects fcntl_getlk results in this
case, so I don't think posix applies? Though, OK, maybe it violates the
principle of least surprise for vfs_test_lock to behave differently.
> That would save others from the pain, as the offenders would
> hopefully take
> notice.
>
> Something like:
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 32c948fe2944..4cc425008036 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2274,8 +2274,16 @@ SYSCALL_DEFINE2(flock, unsigned int, fd,
> unsigned int, cmd)
> */
> int vfs_test_lock(struct file *filp, struct file_lock *fl)
> {
> - if (filp->f_op->lock)
> - return filp->f_op->lock(filp, F_GETLK, fl);
> + int ret;
> + fl_owner_t test_owner = fl->fl_owner;
> +
> + if (filp->f_op->lock) {
> + ret = filp->f_op->lock(filp, F_GETLK, fl);
> + if (fl->fl_type == F_UNLCK)
> + WARN_ON(fl->fl_owner != test_owner);
WARN_ON_ONCE?
> + return ret;
> + }
> +
> posix_test_lock(filp, fl);
> return 0;
> }
>
> .. I'm worried that might be too big of a hammer though. Any thoughts?
No strong opinions here.
--b.
