> struct signalfd_ctx {
> + seqcount_t cnt;
> sigset_t sigmask;
> };
> ...
> @@ -278,7 +302,9 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sig
> return -EINVAL;
> }
> spin_lock_irq(¤t->sighand->siglock);
> + write_seqcount_begin(&ctx->cnt);
> ctx->sigmask = sigmask;
> + write_seqcount_end(&ctx->cnt);
> spin_unlock_irq(¤t->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.
On Tue, Nov 13, 2012 at 03:50:50PM +0100, Oleg Nesterov wrote:
> 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.
On Tue, Nov 13, 2012 at 07:53:13PM +0400, Cyrill Gorcunov wrote:
> On Tue, Nov 13, 2012 at 03:50:50PM +0100, Oleg Nesterov wrote:
> > 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.
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(-)
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ 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;
Index: linux-2.6.git/fs/signalfd.c
===================================================================
--- linux-2.6.git.orig/fs/signalfd.c
+++ 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(¤t->sighand->siglock);
+ write_lock(&ctx->lock);
ctx->sigmask = sigmask;
+ write_unlock(&ctx->lock);
spin_unlock_irq(¤t->sighand->siglock);
wake_up(¤t->sighand->signalfd_wqh);
Index: linux-2.6.git/include/linux/proc_fs.h
===================================================================
--- linux-2.6.git.orig/include/linux/proc_fs.h
+++ 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 */
On 11/13, Cyrill Gorcunov wrote:
>
> struct signalfd_ctx {
> + 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,
> +#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.
On Tue, Nov 13, 2012 at 05:49:51PM +0100, Oleg Nesterov wrote:
> On 11/13, Cyrill Gorcunov wrote:
> >
> > struct signalfd_ctx {
> > + 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.
I see
>
> Or,
>
> > +#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.
On Tue, Nov 13, 2012 at 09:01:59PM +0400, Cyrill Gorcunov wrote:
>
> 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(-)
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ 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;
Index: linux-2.6.git/fs/signalfd.c
===================================================================
--- linux-2.6.git.orig/fs/signalfd.c
+++ 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,
Index: linux-2.6.git/include/linux/proc_fs.h
===================================================================
--- linux-2.6.git.orig/include/linux/proc_fs.h
+++ 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 */
On 11/13, Cyrill Gorcunov wrote:
>
> On Tue, Nov 13, 2012 at 09:01:59PM +0400, Cyrill Gorcunov wrote:
> >
> 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.
> static const struct file_operations signalfd_fops = {
> +#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.
On Tue, Nov 13, 2012 at 06:47:06PM +0100, Oleg Nesterov wrote:
> On 11/13, Cyrill Gorcunov wrote:
> >
> > On Tue, Nov 13, 2012 at 09:01:59PM +0400, Cyrill Gorcunov wrote:
> > >
> > 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.
> > static const struct file_operations signalfd_fops = {
> > +#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
Damn, sorry for noise...
On 11/13, Oleg Nesterov wrote:
>
> On 11/13, Cyrill Gorcunov wrote:
> >
> > On Tue, Nov 13, 2012 at 09:01:59PM +0400, Cyrill Gorcunov wrote:
> > >
> > 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.