locks: move checks from locks_free_lock() to locks_release_private()
diff mbox series

Message ID 87r29s5fiv.fsf@notabene.neil.brown.name
State New
Headers show
Series
  • locks: move checks from locks_free_lock() to locks_release_private()
Related show

Commit Message

NeilBrown April 24, 2019, 2 a.m. UTC
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(-)

Comments

Jeff Layton April 24, 2019, 1:47 p.m. UTC | #1
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>
J. Bruce Fields April 24, 2019, 1:55 p.m. UTC | #2
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.

Patch
diff mbox series

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);
 }