| Message ID | 9559FAE9-4E4A-4161-995F-32D800EC0D5B@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs_test_lock - should it WARN if F_UNLCK and modified file_lock? | expand |
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.
On 8 Jun 2022, at 9:36, J. Bruce Fields wrote: > 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. Oh yeah, good point. fl_owner is just internal. That's enough of a reason for me to drop this idea. Ben
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); + return ret; + } + posix_test_lock(filp, fl); return 0;