| Message ID | 87r29s5fiv.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locks: move checks from locks_free_lock() to locks_release_private() | expand |
On Tue, Apr 23, 2019 at 10:00 PM NeilBrown <neilb@suse.com> wrote: > > > Code that allocates locks using locks_alloc_lock() will free it > using locks_free_lock(), and will benefit from the BUG_ON() > consistency checks therein. > > However some code (nfsd and lockd) allocate a lock embedded in > some other data structure, and so free the lock themselves after > calling locks_release_private(). This path does not benefit from > the consistency checks. > > To help catch future errors, move the BUG_ON() checks to > locks_release_private() - which locks_free_lock() already calls. > This ensures that all users for locks will find out if the lock > isn't detached properly before being free. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > fs/locks.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 71d0c6c2aac5..456a3782c6ca 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -352,6 +352,12 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock); > > void locks_release_private(struct file_lock *fl) > { > + BUG_ON(waitqueue_active(&fl->fl_wait)); > + BUG_ON(!list_empty(&fl->fl_list)); > + BUG_ON(!list_empty(&fl->fl_blocked_requests)); > + BUG_ON(!list_empty(&fl->fl_blocked_member)); > + BUG_ON(!hlist_unhashed(&fl->fl_link)); > + > if (fl->fl_ops) { > if (fl->fl_ops->fl_release_private) > fl->fl_ops->fl_release_private(fl); > @@ -371,12 +377,6 @@ EXPORT_SYMBOL_GPL(locks_release_private); > /* Free a lock which is not in use. */ > void locks_free_lock(struct file_lock *fl) > { > - BUG_ON(waitqueue_active(&fl->fl_wait)); > - BUG_ON(!list_empty(&fl->fl_list)); > - BUG_ON(!list_empty(&fl->fl_blocked_requests)); > - BUG_ON(!list_empty(&fl->fl_blocked_member)); > - BUG_ON(!hlist_unhashed(&fl->fl_link)); > - > locks_release_private(fl); > kmem_cache_free(filelock_cache, fl); > } > -- > 2.14.0.rc0.dirty > Looks good to me. Bruce, would you mind picking this up for the next merge window? I don't have any other file locking patches queued up for v5.2, and I hate to send Linus a PR for just this. Either way: Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Wed, Apr 24, 2019 at 09:47:59AM -0400, Jeff Layton wrote: > Looks good to me. Bruce, would you mind picking this up for the next > merge window? I don't have any other file locking patches queued up > for v5.2, and I hate to send Linus a PR for just this. > > Either way: > > Reviewed-by: Jeff Layton <jlayton@kernel.org> Got it, thanks.--b.
diff --git a/fs/locks.c b/fs/locks.c index 71d0c6c2aac5..456a3782c6ca 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -352,6 +352,12 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock); void locks_release_private(struct file_lock *fl) { + BUG_ON(waitqueue_active(&fl->fl_wait)); + BUG_ON(!list_empty(&fl->fl_list)); + BUG_ON(!list_empty(&fl->fl_blocked_requests)); + BUG_ON(!list_empty(&fl->fl_blocked_member)); + BUG_ON(!hlist_unhashed(&fl->fl_link)); + if (fl->fl_ops) { if (fl->fl_ops->fl_release_private) fl->fl_ops->fl_release_private(fl); @@ -371,12 +377,6 @@ EXPORT_SYMBOL_GPL(locks_release_private); /* Free a lock which is not in use. */ void locks_free_lock(struct file_lock *fl) { - BUG_ON(waitqueue_active(&fl->fl_wait)); - BUG_ON(!list_empty(&fl->fl_list)); - BUG_ON(!list_empty(&fl->fl_blocked_requests)); - BUG_ON(!list_empty(&fl->fl_blocked_member)); - BUG_ON(!hlist_unhashed(&fl->fl_link)); - locks_release_private(fl); kmem_cache_free(filelock_cache, fl); }
Code that allocates locks using locks_alloc_lock() will free it using locks_free_lock(), and will benefit from the BUG_ON() consistency checks therein. However some code (nfsd and lockd) allocate a lock embedded in some other data structure, and so free the lock themselves after calling locks_release_private(). This path does not benefit from the consistency checks. To help catch future errors, move the BUG_ON() checks to locks_release_private() - which locks_free_lock() already calls. This ensures that all users for locks will find out if the lock isn't detached properly before being free. Signed-off-by: NeilBrown <neilb@suse.com> --- fs/locks.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)