Message-ID: <20200121185250.fk6nkcw5nqbkab5o@linux-p48b>
Date:   Tue, 21 Jan 2020 10:52:50 -0800
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Alexey Mateosyan <alexey.mateosyan@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Markus Elfring <elfring@...rs.sourceforge.net>,
        Li Rongqing <lirongqing@...du.com>,
        Arnd Bergmann <arnd@...db.de>,
        Kees Cook <keescook@...omium.org>,
        David Howells <dhowells@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: mq_notify loose notification request on fork()

On Tue, 14 Jan 2020, Alexey Mateosyan wrote:

>Reported bug on bugzilla  https://bugzilla.kernel.org/show_bug.cgi?id=206207
>Duplicating here:
>
>Bug is related to implementation of POSIX mqueues, specifically
>mq_notify syscall.\n
>
>The problem is: if we do subscription on notifications with help of
>mq_notify() syscall, and then do fork() and this fork() fail for some
>reason (for example there is pending signals for the parent process),
>then we loose the subscription on the notification.
>
>The root cause is: during copy_process() we duplicate opened file
>descriptors, and then if something goes wrong inside copy_process()
>(for example there is pending signals for the parent process) we do
>cleanup for the duplicated file descriptors via exit_files() which is
>finally calling filp_close() and flush(). From the other side, current
>implementation of ipc/mqueue.c implements file_operations.flush so
>that it removes notification for the current process, which is
>certainly not designed(desired) behavior.
>
>There is another case how to reproduce this behavior. Do fd =
>mq_open("my_queue"); mq_notify(fd, ...); and never do close(fd) to
>preserve notifications. Then if we open() and close() the queue file
>from the same process - we loose the notification request
>(subscription) for the still opened fd. I.e. current behavior is the
>first close() removes notification.

Is this a real issue? Do you have an actual reproducer? See below.

>
>If we do remove_notification() in file_ops.release instead of
>file_ops.flush in mqueue.c then this change fixes the described above
>fork() related problem. But this will change the behavior so that the
>last close() will remove the notifications.

Right, we don't want to change any user-visible behavior; and only
address the failed fork case.

I guess mqueue_flush_file() really wants to be using the result of
copy_process() task_struct pointer for its cleanup, instead of the
parent 'current'. The following does this but adds a way of getting
the pointer back from files_struct, perhaps you could test this?

Alternatively we could 'hijack' the fl_owner_t id before performing
f_op->flush() callback in filp_close() -- afaict no users actually
use the fl_owner_t field during flush callback, then we could just
restore it back to 'files' before dnotify_flush() etc paths, but I'm
not certain and is rather hacky to begin with.

Thanks,
Davidlohr

--------8<--------------------------------------------------------
diff --git a/fs/file.c b/fs/file.c
index 2f4fcf985079..70e5fe1c70d5 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -440,6 +440,7 @@ void exit_files(struct task_struct *tsk)
 
 	if (files) {
 		task_lock(tsk);
+		files->tsk = NULL;
 		tsk->files = NULL;
 		task_unlock(tsk);
 		put_files_struct(files);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index f07c55ea0c22..29f906157b73 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -55,6 +55,7 @@ struct files_struct {
 
 	struct fdtable __rcu *fdt;
 	struct fdtable fdtab;
+	struct task_struct *tsk;
   /*
    * written part on a separate cache line in SMP
    */
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 3d920ff15c80..22300f0fb35a 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -17,6 +17,7 @@
 #include <linux/init.h>
 #include <linux/pagemap.h>
 #include <linux/file.h>
+#include <linux/fdtable.h>
 #include <linux/mount.h>
 #include <linux/fs_context.h>
 #include <linux/namei.h>
@@ -589,10 +590,12 @@ static ssize_t mqueue_read_file(struct file *filp, char __user *u_data,
 static int mqueue_flush_file(struct file *filp, fl_owner_t id)
 {
 	struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
+	struct files_struct *files = id;
 
 	spin_lock(&info->lock);
-	if (task_tgid(current) == info->notify_owner)
+	if (task_tgid(files->tsk) == info->notify_owner) {
 		remove_notification(info);
+	}
 
 	spin_unlock(&info->lock);
 	return 0;
diff --git a/kernel/fork.c b/kernel/fork.c
index 2508a4f238a3..8a85858db9f5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1467,6 +1467,7 @@ static int copy_files(unsigned long clone_flags, struct task_struct *tsk)
 	if (!newf)
 		goto out;
 
+	newf->tsk = tsk;
 	tsk->files = newf;
 	error = 0;
 out:

Powered by blists - more mailing lists