Skip to contentSkip to conversation

+ fs-epoll-add-procfs-fdinfo-helper.patch added to -mm tree

DISCUSSION Oleg Nesterov 2 participants 9 messages Nov 13, 2012

Subsystems (2)
VFS Proc filesystem
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

Review SYSCALL_DEFINE4(signalfd4, int, ufd, sig · line 308

Quoted 13 lines

struct signalfd_ctx {

(quoted diff) +3 -0
+	seqcount_t cnt;
 	sigset_t sigmask;
 };
...

@@ -278,7 +302,9 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sig
 			return -EINVAL;
 		}
 		spin_lock_irq(&current->sighand->siglock);
+		write_seqcount_begin(&ctx->cnt);
 		ctx->sigmask = sigmask;
+		write_seqcount_end(&ctx->cnt);
 		spin_unlock_irq(&current->sighand->siglock);

This doesn't look right.

The problem is, the current locking is broken, ->siglock can not serialize ->sigmask changes. Just suppose the the child inherits sigfd from parent and they both do sys_signalfd4() at the same time.

Nothing really bad can happen, that is why nobody bothers to fix this. But this patch makes the thing worse, write_seqcount_begin() must be serialized correctly.

Oleg.

#1 lore raw
Quoted 9 lines · Oleg Nesterov

This doesn't look right.

The problem is, the current locking is broken, ->siglock can not serialize ->sigmask changes. Just suppose the the child inherits sigfd from parent and they both do sys_signalfd4() at the same time.

Nothing really bad can happen, that is why nobody bothers to fix this. But this patch makes the thing worse, write_seqcount_begin() must be serialized correctly.

Thanks a lot, Oleg! I'll update.

#2 lore raw
Quoted 7 lines · Cyrill Gorcunov
Quoted 6 lines · Oleg Nesterov

This doesn't look right.

The problem is, the current locking is broken, ->siglock can not serialize ->sigmask changes. Just suppose the the child inherits sigfd from parent and they both do sys_signalfd4() at the same time.

Quoted 5 lines
Quoted 3 lines

Nothing really bad can happen, that is why nobody bothers to fix this. But this patch makes the thing worse, write_seqcount_begin() must be serialized correctly.

Thanks a lot, Oleg! I'll update.

Something like below?
---
From: Cyrill Gorcunov <[email protected]>
Subject: fdinfo: Show sigmask for signalfd fd v3

Signed-off-by Cyrill Gorcunov <[email protected]>
CC Pavel Emelyanov <[email protected]>
CC Al Viro <[email protected]>
CC Alexey Dobriyan <[email protected]>
CC Andrew Morton <[email protected]>
CC James Bottomley <[email protected]>
CC "Aneesh Kumar K.V" <[email protected]>
CC Alexey Dobriyan <[email protected]>
CC Matthew Helsley <[email protected]>
CC "J. Bruce Fields" <[email protected]>
CC "Aneesh Kumar K.V" <[email protected]>
---
 fs/proc/array.c         |    2 +-
 fs/signalfd.c           |   24 ++++++++++++++++++++++++
 include/linux/proc_fs.h |    3 +++
 3 files changed, 28 insertions(+), 1 deletion(-)
fs/proc/array.c +1 -1
Index: linux-2.6.git/fs/proc/array.c
===================================================================
@@ -220,7 +220,7 @@ static inline void task_state(struct seq
 	seq_putc(m, '\n');
 }
 
-static void render_sigset_t(struct seq_file *m, const char *header,
+void render_sigset_t(struct seq_file *m, const char *header,
 				sigset_t *set)
 {
 	int i;
fs/signalfd.c +24 -0
Index: linux-2.6.git/fs/signalfd.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/anon_inodes.h>
 #include <linux/signalfd.h>
 #include <linux/syscalls.h>
+#include <linux/proc_fs.h>
 
 void signalfd_cleanup(struct sighand_struct *sighand)
 {

@@ -46,6 +47,7 @@ void signalfd_cleanup(struct sighand_str
 }
 
 struct signalfd_ctx {
+	rwlock_t lock;
 	sigset_t sigmask;
 };
 

@@ -227,7 +229,26 @@ static ssize_t signalfd_read(struct file
 	return total ? total: ret;
 }
 
+#ifdef CONFIG_PROC_FS
+static int signalfd_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct signalfd_ctx *ctx = f->private_data;
+	sigset_t sigmask;
+
+	read_lock(&ctx->lock);
+	sigmask = ctx->sigmask;
+	read_unlock(&ctx->lock);
+
+	signotset(&sigmask);
+	render_sigset_t(m, "sigmask:\t", &sigmask);
+	return 0;
+}
+#endif
+
 static const struct file_operations signalfd_fops = {
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= signalfd_show_fdinfo,
+#endif
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
 	.read		= signalfd_read,

@@ -259,6 +280,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sig
 			return -ENOMEM;
 
 		ctx->sigmask = sigmask;
+		rwlock_init(&ctx->lock);
 
 		/*
 		 * When we call this, the initialization must be complete, since

@@ -278,7 +300,9 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sig
 			return -EINVAL;
 		}
 		spin_lock_irq(&current->sighand->siglock);
+		write_lock(&ctx->lock);
 		ctx->sigmask = sigmask;
+		write_unlock(&ctx->lock);
 		spin_unlock_irq(&current->sighand->siglock);
 
 		wake_up(&current->sighand->signalfd_wqh);
include/linux/proc_fs.h +3 -0
Index: linux-2.6.git/include/linux/proc_fs.h
===================================================================
@@ -290,4 +290,7 @@ static inline struct net *PDE_NET(struct
 	return pde->parent->data;
 }
 
+#include <linux/signal.h>
+
+void render_sigset_t(struct seq_file *m, const char *header, sigset_t *set);
 #endif /* _LINUX_PROC_FS_H */
author
#3 lore raw

fs/signalfd.c void signalfd_cleanup(struct sighand_str · line 51

Quoted 4 lines · Cyrill Gorcunov

struct signalfd_ctx {

fs/signalfd.c +1 -0
+	rwlock_t lock;
 	sigset_t sigmask;

Oh, I don't think.

rwlock_t is horrible in general, and what it can buy for signalfd? A plain spinlock would be better. Or seqlock_t.

Whatever you do, you are trying to introduce the lock which should serialize the access to ->sigmask correctly. In this case I think you should split this change into 2 patches. The first one should fix the locking, imo. sys_signalfd4() should not use ->siglock at all, and the users which take ->siglock to read ->sigmask should be updated.

Or,

fs/signalfd.c static ssize_t signalfd_read(struct file · line 240

Quoted 9 lines
fs/signalfd.c +9 -0
+#ifdef CONFIG_PROC_FS
+static int signalfd_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct signalfd_ctx *ctx = f->private_data;
+	sigset_t sigmask;
+
+	read_lock(&ctx->lock);
+	sigmask = ctx->sigmask;
+	read_unlock(&ctx->lock);

Just read ctx->sigmask lockless. Do we really care if show_fdinfo() reads the value "in between" ?

Oleg.

#4 lore raw
Quoted 11 lines · Oleg Nesterov

fs/signalfd.c void signalfd_cleanup(struct sighand_str · line 51

Quoted 4 lines · Cyrill Gorcunov

struct signalfd_ctx {

fs/signalfd.c +1 -0
+	rwlock_t lock;
 	sigset_t sigmask;

Oh, I don't think.

rwlock_t is horrible in general, and what it can buy for signalfd? A plain spinlock would be better. Or seqlock_t.

Quoted 6 lines

Whatever you do, you are trying to introduce the lock which should serialize the access to ->sigmask correctly. In this case I think you should split this change into 2 patches. The first one should fix the locking, imo. sys_signalfd4() should not use ->siglock at all, and the users which take ->siglock to read ->sigmask should be updated.

I see

Quoted 15 lines

Or,

fs/signalfd.c static ssize_t signalfd_read(struct file · line 240

Quoted 9 lines
fs/signalfd.c +9 -0
+#ifdef CONFIG_PROC_FS
+static int signalfd_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct signalfd_ctx *ctx = f->private_data;
+	sigset_t sigmask;
+
+	read_lock(&ctx->lock);
+	sigmask = ctx->sigmask;
+	read_unlock(&ctx->lock);

Just read ctx->sigmask lockless. Do we really care if show_fdinfo() reads the value "in between" ?

As from c/r patch I think we can read it lockless (since we do stop tasks anyway before doing checkpoint). So I would prefer to provide it without locks at all.

#5 lore raw
Quoted 4 lines · Cyrill Gorcunov

As from c/r patch I think we can read it lockless (since we do stop tasks anyway before doing checkpoint). So I would prefer to provide it without locks at all.

Something like this. (Also I wonder where the documentation about fdinfo should go?)
---
From: Cyrill Gorcunov <[email protected]>
Subject: fdinfo: Show sigmask for signalfd fd v3

The sigmask is read in lockless manner for a sake of code simplicity, thus if precise data needed here the tasks which refer to the signalfd should be stopped before read.

Signed-off-by Cyrill Gorcunov <[email protected]>
CC Pavel Emelyanov <[email protected]>
CC Al Viro <[email protected]>
CC Alexey Dobriyan <[email protected]>
CC Andrew Morton <[email protected]>
CC James Bottomley <[email protected]>
CC "Aneesh Kumar K.V" <[email protected]>
CC Alexey Dobriyan <[email protected]>
CC Matthew Helsley <[email protected]>
CC "J. Bruce Fields" <[email protected]>
CC "Aneesh Kumar K.V" <[email protected]>
---
 fs/proc/array.c         |    2 +-
 fs/signalfd.c           |   18 ++++++++++++++++++
 include/linux/proc_fs.h |    3 +++
 3 files changed, 22 insertions(+), 1 deletion(-)
fs/proc/array.c +1 -1
Index: linux-2.6.git/fs/proc/array.c
===================================================================
@@ -220,7 +220,7 @@ static inline void task_state(struct seq
 	seq_putc(m, '\n');
 }
 
-static void render_sigset_t(struct seq_file *m, const char *header,
+void render_sigset_t(struct seq_file *m, const char *header,
 				sigset_t *set)
 {
 	int i;
fs/signalfd.c +18 -0
Index: linux-2.6.git/fs/signalfd.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/anon_inodes.h>
 #include <linux/signalfd.h>
 #include <linux/syscalls.h>
+#include <linux/proc_fs.h>
 
 void signalfd_cleanup(struct sighand_struct *sighand)
 {

@@ -227,7 +228,24 @@ static ssize_t signalfd_read(struct file
 	return total ? total: ret;
 }
 
+#ifdef CONFIG_PROC_FS
+static int signalfd_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct signalfd_ctx *ctx = f->private_data;
+	sigset_t sigmask;
+
+	sigmask = ctx->sigmask;
+	signotset(&sigmask);
+	render_sigset_t(m, "sigmask:\t", &sigmask);
+
+	return 0;
+}
+#endif
+
 static const struct file_operations signalfd_fops = {
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= signalfd_show_fdinfo,
+#endif
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
 	.read		= signalfd_read,
include/linux/proc_fs.h +3 -0
Index: linux-2.6.git/include/linux/proc_fs.h
===================================================================
@@ -290,4 +290,7 @@ static inline struct net *PDE_NET(struct
 	return pde->parent->data;
 }
 
+#include <linux/signal.h>
+
+void render_sigset_t(struct seq_file *m, const char *header, sigset_t *set);
 #endif /* _LINUX_PROC_FS_H */
author
#6 lore raw
Quoted 7 lines · Cyrill Gorcunov
Quoted 1 line · Cyrill Gorcunov

The sigmask is read in lockless manner for a sake of code simplicity, thus if precise data needed here the tasks which refer to the signalfd should be stopped before read.

Yes, I think this is fine, and this patch should replace fs-epoll-add-procfs-fdinfo-helper.patch in -mm.

fs/signalfd.c static ssize_t signalfd_read(struct file · line 248

Quoted 4 lines

static const struct file_operations signalfd_fops = {

fs/signalfd.c +3 -0
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= signalfd_show_fdinfo,
+#endif

I am just curious and I can't find the patch which adds .show_fdinfo. Could you please send me link/patch offlist?

Oleg.

#7 lore raw
Quoted 11 lines · Oleg Nesterov
Quoted 7 lines · Cyrill Gorcunov
Quoted 1 line · Cyrill Gorcunov

The sigmask is read in lockless manner for a sake of code simplicity, thus if precise data needed here the tasks which refer to the signalfd should be stopped before read.

Yes, I think this is fine, and this patch should replace fs-epoll-add-procfs-fdinfo-helper.patch in -mm.

No, epoll is different one, this patch is for fdinfo-show-sigmask-for-signalfd-fd.patch, I'll send the updates as new series.

Quoted 7 lines

fs/signalfd.c static ssize_t signalfd_read(struct file · line 248

Quoted 4 lines

static const struct file_operations signalfd_fops = {

fs/signalfd.c +3 -0
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= signalfd_show_fdinfo,
+#endif

I am just curious and I can't find the patch which adds .show_fdinfo. Could you please send me link/patch offlist?

It is titled as procfs-add-ability-to-plug-in-auxiliary-fdinfo-providers.patch Sure I'll resend you it offlist in a minute.

Cyrill

author
#8 lore raw
In reply to Oleg Nesterov

Damn, sorry for noise...

Quoted 5 lines · Oleg Nesterov
Quoted 3 lines · Cyrill Gorcunov
Quoted 1 line · Cyrill Gorcunov
Quoted 7 lines
Quoted 4 lines

The sigmask is read in lockless manner for a sake of code simplicity, thus if precise data needed here the tasks which refer to the signalfd should be stopped before read.

Yes, I think this is fine, and this patch should replace fs-epoll-add-procfs-fdinfo-helper.patch in -mm.

I meant fdinfo-show-sigmask-for-signalfd-fd.patch

And the subject was wrong from the very beginning, sorry for confusion.

Oleg.