[Devel] [PATCH RHEL7 COMMIT] fasync: Fix deadlock between task-context and interrupt-context kill_fasync()
Konstantin Khorenko
khorenko at virtuozzo.com
Fri Apr 6 18:39:57 MSK 2018
The commit is pushed to "branch-rh7-3.10.0-693.21.1.vz7.46.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-693.21.1.vz7.46.4
------>
commit c0dbd2f569be404689b1059ce434a8f4052a9d51
Author: Kirill Tkhai <ktkhai at virtuozzo.com>
Date: Fri Apr 6 18:39:57 2018 +0300
fasync: Fix deadlock between task-context and interrupt-context kill_fasync()
[This also sent to ms]
I've observed the following deadlock:
[task 1] [task 2] [task 3]
kill_fasync() mm_update_next_owner() copy_process()
spin_lock_irqsave(&fa->fa_lock) read_lock(&tasklist_lock) write_lock_irq(&tasklist_lock)
send_sigio() <IRQ> ...
read_lock(&fown->lock) kill_fasync() ...
read_lock(&tasklist_lock) spin_lock_irqsave(&fa->fa_lock) ...
Task 1 can't acquire read locked tasklist_lock, since there is
already task 3 expressed its wish to take the lock exclusive.
Task 2 holds the read locked lock, but it can't take the spin lock.
Also, there is possible another deadlock (which I haven't observed):
[task 1] [task 2]
f_getown() kill_fasync()
read_lock(&f_own->lock) spin_lock_irqsave(&fa->fa_lock,)
<IRQ> send_sigio() write_lock_irq(&f_own->lock)
kill_fasync() read_lock(&fown->lock)
spin_lock_irqsave(&fa->fa_lock,)
Actually, we do not need exclusive fa->fa_lock in kill_fasync_rcu(),
as it guarantees fa->fa_file->f_owner integrity only. It may seem,
that it used to give a task a small possibility to receive two sequential
signals, if there are two parallel kill_fasync() callers, and task
handles the first signal fastly, but the behaviour won't become
different, since there is exclusive sighand lock in do_send_sig_info().
The patch converts fa_lock into rwlock_t, and this fixes two above
deadlocks, as rwlock is allowed to be taken from interrupt handler
by qrwlock design.
https://jira.sw.ru/browse/PSBM-83102
Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
Reviewed-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
I used the following program for testing:
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>
#include <fcntl.h>
#include <errno.h>
#include <stdio.h>
#ifndef F_SETSIG
#define F_SETSIG 10
#endif
void handler(int sig)
{
}
main()
{
unsigned int flags;
int fd;
system("echo 8 > /proc/sys/kernel/random/read_wakeup_threshold");
system("while :; do ls -R / > /dev/random 2>&1 ; echo 3 > /proc/sys/vm/drop_caches; done &");
if (signal(SIGINT, handler) < 0) {
perror("Signal");
exit(1);
}
fd = open("/dev/random", O_RDWR);
if (fd < 0) {
perror("Can't open");
exit(1);
}
flags = FASYNC | fcntl(fd, F_GETFL);
if (fcntl(fd, F_SETFL, flags) < 0) {
perror("Setfl");
exit(1);
}
if (fcntl(fd, F_SETOWN, getpid())) {
perror("Setown");
exit(1);
}
if (fcntl(fd, F_SETSIG, SIGINT)) {
perror("Setsig");
exit(1);
}
while (1)
sleep(100);
}
---
fs/fcntl.c | 15 +++++++--------
include/linux/fs.h | 2 +-
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index e611047fb904..a25130bb3cd0 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -615,9 +615,9 @@ int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
if (fa->fa_file != filp)
continue;
- spin_lock_irq(&fa->fa_lock);
+ write_lock_irq(&fa->fa_lock);
fa->fa_file = NULL;
- spin_unlock_irq(&fa->fa_lock);
+ write_unlock_irq(&fa->fa_lock);
*fp = fa->fa_next;
call_rcu(&fa->fa_rcu, fasync_free_rcu);
@@ -662,13 +662,13 @@ struct fasync_struct *fasync_insert_entry(int fd, struct file *filp, struct fasy
if (fa->fa_file != filp)
continue;
- spin_lock_irq(&fa->fa_lock);
+ write_lock_irq(&fa->fa_lock);
fa->fa_fd = fd;
- spin_unlock_irq(&fa->fa_lock);
+ write_unlock_irq(&fa->fa_lock);
goto out;
}
- spin_lock_init(&new->fa_lock);
+ rwlock_init(&new->fa_lock);
new->magic = FASYNC_MAGIC;
new->fa_file = filp;
new->fa_fd = fd;
@@ -731,14 +731,13 @@ static void kill_fasync_rcu(struct fasync_struct *fa, int sig, int band)
{
while (fa) {
struct fown_struct *fown;
- unsigned long flags;
if (fa->magic != FASYNC_MAGIC) {
printk(KERN_ERR "kill_fasync: bad magic number in "
"fasync_struct!\n");
return;
}
- spin_lock_irqsave(&fa->fa_lock, flags);
+ read_lock(&fa->fa_lock);
if (fa->fa_file) {
fown = &fa->fa_file->f_owner;
/* Don't send SIGURG to processes which have not set a
@@ -747,7 +746,7 @@ static void kill_fasync_rcu(struct fasync_struct *fa, int sig, int band)
if (!(sig == SIGURG && fown->signum == 0))
send_sigio(fown, fa->fa_fd, band);
}
- spin_unlock_irqrestore(&fa->fa_lock, flags);
+ read_unlock(&fa->fa_lock);
fa = rcu_dereference(fa->fa_next);
}
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a1dc3521f979..1c5144180012 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1541,7 +1541,7 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
}
struct fasync_struct {
- spinlock_t fa_lock;
+ rwlock_t fa_lock;
int magic;
int fa_fd;
struct fasync_struct *fa_next; /* singly linked list */
More information about the Devel
mailing list