lkml.org 
[lkml]   [2020]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 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:
\
 
 \ /
  Last update: 2020-01-21 20:03    [W:0.128 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site