| From 40962df8441fdfbebf196795772e78e6467dec7f Mon Sep 17 00:00:00 2001 |
| From: Linus Torvalds <torvalds@linux-foundation.org> |
| Date: Wed, 18 Mar 2020 07:52:21 -0400 |
| Subject: [PATCH] locks: reinstate locks_delete_block optimization |
| |
| commit dcf23ac3e846ca0cf626c155a0e3fcbbcf4fae8a upstream. |
| |
| There is measurable performance impact in some synthetic tests due to |
| commit 6d390e4b5d48 (locks: fix a potential use-after-free problem when |
| wakeup a waiter). Fix the race condition instead by clearing the |
| fl_blocker pointer after the wake_up, using explicit acquire/release |
| semantics. |
| |
| This does mean that we can no longer use the clearing of fl_blocker as |
| the wait condition, so switch the waiters over to checking whether the |
| fl_blocked_member list_head is empty. |
| |
| Reviewed-by: yangerkun <yangerkun@huawei.com> |
| Reviewed-by: NeilBrown <neilb@suse.de> |
| Fixes: 6d390e4b5d48 (locks: fix a potential use-after-free problem when wakeup a waiter) |
| Signed-off-by: Jeff Layton <jlayton@kernel.org> |
| Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/fs/cifs/file.c b/fs/cifs/file.c |
| index b971f7a79481..5910494c3f65 100644 |
| --- a/fs/cifs/file.c |
| +++ b/fs/cifs/file.c |
| @@ -1149,7 +1149,8 @@ cifs_posix_lock_set(struct file *file, struct file_lock *flock) |
| rc = posix_lock_file(file, flock, NULL); |
| up_write(&cinode->lock_sem); |
| if (rc == FILE_LOCK_DEFERRED) { |
| - rc = wait_event_interruptible(flock->fl_wait, !flock->fl_blocker); |
| + rc = wait_event_interruptible(flock->fl_wait, |
| + list_empty(&flock->fl_blocked_member)); |
| if (!rc) |
| goto try_again; |
| locks_delete_block(flock); |
| diff --git a/fs/locks.c b/fs/locks.c |
| index e0c920ff599e..ceb202de1307 100644 |
| --- a/fs/locks.c |
| +++ b/fs/locks.c |
| @@ -729,7 +729,6 @@ static void __locks_delete_block(struct file_lock *waiter) |
| { |
| locks_delete_global_blocked(waiter); |
| list_del_init(&waiter->fl_blocked_member); |
| - waiter->fl_blocker = NULL; |
| } |
| |
| static void __locks_wake_up_blocks(struct file_lock *blocker) |
| @@ -744,6 +743,13 @@ static void __locks_wake_up_blocks(struct file_lock *blocker) |
| waiter->fl_lmops->lm_notify(waiter); |
| else |
| wake_up(&waiter->fl_wait); |
| + |
| + /* |
| + * The setting of fl_blocker to NULL marks the "done" |
| + * point in deleting a block. Paired with acquire at the top |
| + * of locks_delete_block(). |
| + */ |
| + smp_store_release(&waiter->fl_blocker, NULL); |
| } |
| } |
| |
| @@ -757,11 +763,42 @@ int locks_delete_block(struct file_lock *waiter) |
| { |
| int status = -ENOENT; |
| |
| + /* |
| + * If fl_blocker is NULL, it won't be set again as this thread "owns" |
| + * the lock and is the only one that might try to claim the lock. |
| + * |
| + * We use acquire/release to manage fl_blocker so that we can |
| + * optimize away taking the blocked_lock_lock in many cases. |
| + * |
| + * The smp_load_acquire guarantees two things: |
| + * |
| + * 1/ that fl_blocked_requests can be tested locklessly. If something |
| + * was recently added to that list it must have been in a locked region |
| + * *before* the locked region when fl_blocker was set to NULL. |
| + * |
| + * 2/ that no other thread is accessing 'waiter', so it is safe to free |
| + * it. __locks_wake_up_blocks is careful not to touch waiter after |
| + * fl_blocker is released. |
| + * |
| + * If a lockless check of fl_blocker shows it to be NULL, we know that |
| + * no new locks can be inserted into its fl_blocked_requests list, and |
| + * can avoid doing anything further if the list is empty. |
| + */ |
| + if (!smp_load_acquire(&waiter->fl_blocker) && |
| + list_empty(&waiter->fl_blocked_requests)) |
| + return status; |
| + |
| spin_lock(&blocked_lock_lock); |
| if (waiter->fl_blocker) |
| status = 0; |
| __locks_wake_up_blocks(waiter); |
| __locks_delete_block(waiter); |
| + |
| + /* |
| + * The setting of fl_blocker to NULL marks the "done" point in deleting |
| + * a block. Paired with acquire at the top of this function. |
| + */ |
| + smp_store_release(&waiter->fl_blocker, NULL); |
| spin_unlock(&blocked_lock_lock); |
| return status; |
| } |
| @@ -1354,7 +1391,8 @@ static int posix_lock_inode_wait(struct inode *inode, struct file_lock *fl) |
| error = posix_lock_inode(inode, fl, NULL); |
| if (error != FILE_LOCK_DEFERRED) |
| break; |
| - error = wait_event_interruptible(fl->fl_wait, !fl->fl_blocker); |
| + error = wait_event_interruptible(fl->fl_wait, |
| + list_empty(&fl->fl_blocked_member)); |
| if (error) |
| break; |
| } |
| @@ -1439,7 +1477,8 @@ int locks_mandatory_area(struct inode *inode, struct file *filp, loff_t start, |
| error = posix_lock_inode(inode, &fl, NULL); |
| if (error != FILE_LOCK_DEFERRED) |
| break; |
| - error = wait_event_interruptible(fl.fl_wait, !fl.fl_blocker); |
| + error = wait_event_interruptible(fl.fl_wait, |
| + list_empty(&fl.fl_blocked_member)); |
| if (!error) { |
| /* |
| * If we've been sleeping someone might have |
| @@ -1632,7 +1671,8 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) |
| |
| locks_dispose_list(&dispose); |
| error = wait_event_interruptible_timeout(new_fl->fl_wait, |
| - !new_fl->fl_blocker, break_time); |
| + list_empty(&new_fl->fl_blocked_member), |
| + break_time); |
| |
| percpu_down_read(&file_rwsem); |
| spin_lock(&ctx->flc_lock); |
| @@ -2043,7 +2083,8 @@ static int flock_lock_inode_wait(struct inode *inode, struct file_lock *fl) |
| error = flock_lock_inode(inode, fl); |
| if (error != FILE_LOCK_DEFERRED) |
| break; |
| - error = wait_event_interruptible(fl->fl_wait, !fl->fl_blocker); |
| + error = wait_event_interruptible(fl->fl_wait, |
| + list_empty(&fl->fl_blocked_member)); |
| if (error) |
| break; |
| } |
| @@ -2320,7 +2361,8 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd, |
| error = vfs_lock_file(filp, cmd, fl, NULL); |
| if (error != FILE_LOCK_DEFERRED) |
| break; |
| - error = wait_event_interruptible(fl->fl_wait, !fl->fl_blocker); |
| + error = wait_event_interruptible(fl->fl_wait, |
| + list_empty(&fl->fl_blocked_member)); |
| if (error) |
| break; |
| } |
| -- |
| 2.7.4 |
| |