[PATCH RFC] kernel/locking, fs/direct-io: Introduce and use down_write_nolockdep()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The rwsem locking functions contain lockdep annotations (down_write(),
up_write(), down_read(), up_read(), ...). Unfortunately lockdep and
semaphores are not a good match because lockdep assumes that releasing
a synchronization object will happen from the same kernel thread that
acquired the synchronization object. This is the case for most but not
all semaphore locking calls. When a semaphore is released from another
context than the context from which it has been acquired lockdep may
report a false positive circular locking report. This patch avoids
that lockdep reports the false positive report shown below for the
direct I/O code. Please note that the lockdep complaint shown below is
not the same as a closely related lockdep complaint that has been
reported recently by syzbot. This patch has been tested on top of a
patch that was suggested by Ted and Tejun, namely to change a
destroy_workqueue() call in the direct-io code into a call to
destroy_workqueue_no_drain(). For the syzbot report and the discussion
of that report, see also https://lkml.org/lkml/2018/10/23/511.

======================================================
WARNING: possible circular locking dependency detected
4.19.0-dbg+ #14 Not tainted
------------------------------------------------------
kworker/3:1/56 is trying to acquire lock:
0000000076123785 (&sb->s_type->i_mutex_key#14){+.+.}, at:
__generic_file_fsync+0x77/0xf0

but task is already holding lock:
000000006a866e67 ((work_completion)(&dio->complete_work)){.+.+}, at:
process_one_work+0x3c9/0x9f0

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #2 ((work_completion)(&dio->complete_work)){.+.+}:
       process_one_work+0x44d/0x9f0
       worker_thread+0x63/0x5a0
       kthread+0x1cf/0x1f0
       ret_from_fork+0x24/0x30

-> #1 ((wq_completion)"dio/%s"sb->s_id){++++}:
       flush_workqueue+0xf3/0x970
       drain_workqueue+0xec/0x220
       destroy_workqueue+0x23/0x350
       sb_init_dio_done_wq+0x6a/0x80
       do_blockdev_direct_IO+0x1f33/0x4be0
       __blockdev_direct_IO+0x79/0x86
       ext4_direct_IO+0x5df/0xbb0
       generic_file_direct_write+0x119/0x220
       __generic_file_write_iter+0x131/0x2d0
       ext4_file_write_iter+0x3fa/0x710
       aio_write+0x235/0x330
       io_submit_one+0x510/0xeb0
       __x64_sys_io_submit+0x122/0x340
       do_syscall_64+0x71/0x220
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

-> #0 (&sb->s_type->i_mutex_key#14){+.+.}:
       lock_acquire+0xc5/0x200
       down_write+0x3d/0x80
       __generic_file_fsync+0x77/0xf0
       ext4_sync_file+0x3c9/0x780
       vfs_fsync_range+0x66/0x100
       dio_complete+0x2f5/0x360
       dio_aio_complete_work+0x1c/0x20
       process_one_work+0x487/0x9f0
       worker_thread+0x63/0x5a0
       kthread+0x1cf/0x1f0
       ret_from_fork+0x24/0x30

other info that might help us debug this:

Chain exists of:
  &sb->s_type->i_mutex_key#14 --> (wq_completion)"dio/%s"sb->s_id -->
(work_completion)(&dio->complete_work)

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock((work_completion)(&dio->complete_work));
                               lock((wq_completion)"dio/%s"sb->s_id);
                               lock((work_completion)(&dio->complete_work));
  lock(&sb->s_type->i_mutex_key#14);

 *** DEADLOCK ***

2 locks held by kworker/3:1/56:
 #0: 000000005cbfa331 ((wq_completion)"dio/%s"sb->s_id){++++}, at:
process_one_work+0x3c9/0x9f0
 #1: 000000006a866e67 ((work_completion)(&dio->complete_work)){.+.+}, at:
process_one_work+0x3c9/0x9f0

stack backtrace:
CPU: 3 PID: 56 Comm: kworker/3:1 Not tainted 4.19.0-dbg+ #14
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1
04/01/2014
Workqueue: dio/dm-0 dio_aio_complete_work
Call Trace:
 dump_stack+0x86/0xc5
 print_circular_bug.isra.32+0x20a/0x218
 __lock_acquire+0x1c68/0x1cf0
 lock_acquire+0xc5/0x200
 down_write+0x3d/0x80
 __generic_file_fsync+0x77/0xf0
 ext4_sync_file+0x3c9/0x780
 vfs_fsync_range+0x66/0x100
 dio_complete+0x2f5/0x360
 dio_aio_complete_work+0x1c/0x20
 process_one_work+0x487/0x9f0
 worker_thread+0x63/0x5a0
 kthread+0x1cf/0x1f0
 ret_from_fork+0x24/0x30

Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Theodore Ts'o <tytso@xxxxxxx>
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
 fs/direct-io.c                  |  2 +-
 include/linux/fs.h              |  5 ++++
 include/linux/rwsem.h           |  5 ++++
 kernel/locking/rwsem-spinlock.c |  1 +
 kernel/locking/rwsem-xadd.c     |  1 +
 kernel/locking/rwsem.c          | 51 +++++++++++++++++++++++++++++----
 mm/rmap.c                       |  1 +
 7 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index b49f40594d3b..426ed5b4fe66 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -1221,7 +1221,7 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 					iocb->ki_filp->f_mapping;
 
 			/* will be released by direct_io_worker */
-			inode_lock(inode);
+			inode_lock_nolockdep(inode);
 
 			retval = filemap_write_and_wait_range(mapping, offset,
 							      end - 1);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 897eae8faee1..0bbfde0af5b6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -733,6 +733,11 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT2,
 };
 
+static inline void inode_lock_nolockdep(struct inode *inode)
+{
+	down_write_nolockdep(&inode->i_rwsem);
+}
+
 static inline void inode_lock(struct inode *inode)
 {
 	down_write(&inode->i_rwsem);
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 67dbb57508b1..4354de397f1b 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -41,6 +41,10 @@ struct rw_semaphore {
 #endif
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map	dep_map;
+	/*
+	 * Number of up_write() calls that must skip rwsem_release().
+	 */
+	unsigned		nolockdep;
 #endif
 };
 
@@ -128,6 +132,7 @@ extern int down_read_trylock(struct rw_semaphore *sem);
 /*
  * lock for writing
  */
+extern void down_write_nolockdep(struct rw_semaphore *sem);
 extern void down_write(struct rw_semaphore *sem);
 extern int __must_check down_write_killable(struct rw_semaphore *sem);
 
diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
index a7ffb2a96ede..d6ca3c41681c 100644
--- a/kernel/locking/rwsem-spinlock.c
+++ b/kernel/locking/rwsem-spinlock.c
@@ -47,6 +47,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	 */
 	debug_check_no_locks_freed((void *)sem, sizeof(*sem));
 	lockdep_init_map(&sem->dep_map, name, key, 0);
+	sem->nolockdep = 0;
 #endif
 	sem->count = 0;
 	raw_spin_lock_init(&sem->wait_lock);
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 09b180063ee1..b7bab2ddf6c8 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -82,6 +82,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	 */
 	debug_check_no_locks_freed((void *)sem, sizeof(*sem));
 	lockdep_init_map(&sem->dep_map, name, key, 0);
+	sem->nolockdep = 0;
 #endif
 	atomic_long_set(&sem->count, RWSEM_UNLOCKED_VALUE);
 	raw_spin_lock_init(&sem->wait_lock);
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index e586f0d03ad3..aed55ce92ec3 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -61,18 +61,52 @@ int down_read_trylock(struct rw_semaphore *sem)
 
 EXPORT_SYMBOL(down_read_trylock);
 
-/*
- * lock for writing
- */
-void __sched down_write(struct rw_semaphore *sem)
+void down_write_impl(struct rw_semaphore *sem)
 {
 	might_sleep();
-	rwsem_acquire(&sem->dep_map, 0, 0, _RET_IP_);
 
 	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);
 	rwsem_set_owner(sem);
 }
 
+/*
+ * down_write - lock for writing without informing lockdep
+ * @sem: Semaphore that serializes writers against other readers and writers.
+ *
+ * Lockdep assumes that up_write() will be called from the same thread that
+ * calls down_write(). That can result in false positive lockdep complaints
+ * if another thread will call up_write().
+ */
+void __sched down_write_nolockdep(struct rw_semaphore *sem)
+{
+	down_write_impl(sem);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	/*
+	 * It is assumed that up_write() will not be called (from another
+	 * thread) before down_write() returns.
+	 */
+	sem->nolockdep++;
+#endif
+}
+EXPORT_SYMBOL(down_write_nolockdep);
+
+/**
+ * down_write - lock for writing
+ * @sem: Semaphore that serializes writers against other readers and writers.
+ */
+void __sched down_write(struct rw_semaphore *sem)
+{
+	rwsem_acquire(&sem->dep_map, 0, 0, _RET_IP_);
+	down_write_impl(sem);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	/*
+	 * Complain if down_write() is called without having called
+	 * init_rwsem() first.
+	 */
+	WARN_ON_ONCE(sem->nolockdep);
+#endif
+}
+
 EXPORT_SYMBOL(down_write);
 
 /*
@@ -130,7 +164,12 @@ EXPORT_SYMBOL(up_read);
  */
 void up_write(struct rw_semaphore *sem)
 {
-	rwsem_release(&sem->dep_map, 1, _RET_IP_);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	if (sem->nolockdep == 0)
+		rwsem_release(&sem->dep_map, 1, _RET_IP_);
+	else
+		sem->nolockdep--;
+#endif
 	DEBUG_RWSEMS_WARN_ON(sem->owner != current);
 
 	rwsem_clear_owner(sem);
diff --git a/mm/rmap.c b/mm/rmap.c
index 1e79fac3186b..2a953d3b7431 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -81,6 +81,7 @@ static inline struct anon_vma *anon_vma_alloc(void)
 
 	anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
 	if (anon_vma) {
+		init_rwsem(&anon_vma->rwsem);
 		atomic_set(&anon_vma->refcount, 1);
 		anon_vma->degree = 1;	/* Reference for first vma */
 		anon_vma->parent = anon_vma;
-- 
2.19.1.568.g152ad8e336-goog




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux