Re: 2.3.31++: Badness at kernel/softirq.c:143 due to new session leader connector

Re: 2.3.31++: Badness at kernel/softirq.c:143 due to new session leader connector

[Oleg Nesterov]

On 09/29, Evgeny Polyakov wrote:
>
> I'm yet do download the latest git to check this particular patch, but
> I suppose it is possible to copy data under the lock into temporary
> buffer and then send it using existing infrastructure instead of
> calling it under the tasklist lock.

Afaics, we can just shift proc_sid_connector() from __set_special_pids()
to sys_setsid().

Oleg.

[PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Christian Borntraeger]

Am Dienstag 29 September 2009 15:24:15 schrieb Oleg Nesterov:
> On 09/29, Evgeny Polyakov wrote:
> > I'm yet do download the latest git to check this particular patch, but
> > I suppose it is possible to copy data under the lock into temporary
> > buffer and then send it using existing infrastructure instead of
> > calling it under the tasklist lock.
> 
> Afaics, we can just shift proc_sid_connector() from __set_special_pids()
> to sys_setsid().

Ok,  can confirm that this patch fixes my problem, but I am not sure if the
intended behaviour is still working as expected.

[PATCH] connector: Fix sid connector

The sid connector gives the following warning:
Badness at kernel/softirq.c:143
[...]
Call Trace:
([<000000013fe04100>] 0x13fe04100)
 [<000000000048a946>] sk_filter+0x9a/0xd0
 [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
 [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
 [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
 [<0000000000142604>] __set_special_pids+0x58/0x90
 [<0000000000159938>] sys_setsid+0xb4/0xd8
 [<00000000001187fe>] sysc_noemu+0x10/0x16
 [<00000041616cb266>] 0x41616cb266

The warning is
--->    WARN_ON_ONCE(in_irq() || irqs_disabled());

The network code must not be called with disabled interrupts but
sys_setsid holds the tasklist_lock with spinlock_irq while calling
the connector. We can safely move proc_sid_connector from
__set_special_pids to sys_setsid.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
 kernel/exit.c |    4 +---
 kernel/sys.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/kernel/exit.c
===================================================================
--- linux-2.6.orig/kernel/exit.c
+++ linux-2.6/kernel/exit.c
@@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
 {
 	struct task_struct *curr = current->group_leader;
 
-	if (task_session(curr) != pid) {
+	if (task_session(curr) != pid)
 		change_pid(curr, PIDTYPE_SID, pid);
-		proc_sid_connector(curr);
-	}
 
 	if (task_pgrp(curr) != pid)
 		change_pid(curr, PIDTYPE_PGID, pid);
Index: linux-2.6/kernel/sys.c
===================================================================
--- linux-2.6.orig/kernel/sys.c
+++ linux-2.6/kernel/sys.c
@@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
 	err = session;
 out:
 	write_unlock_irq(&tasklist_lock);
+	if (!err)
+		proc_sid_connector(sid);
 	return err;
 }
 

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Oleg Nesterov]

On 09/29, Christian Borntraeger wrote:
>
> --- linux-2.6.orig/kernel/sys.c
> +++ linux-2.6/kernel/sys.c
> @@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
>  	err = session;
>  out:
>  	write_unlock_irq(&tasklist_lock);
> +	if (!err)
> +		proc_sid_connector(sid);

sys_setsid() returns the session nr on success, not zero.

	if (err > 0)
		proc_sid_connector(sid);

Otherwize I think the patch is fine. Not only it should fix the problem,
imho it makes the code cleaner.

If Scott still thinks daemonize() should report too, we can change it.

(I'd suggest you to CC Andrew if you are going to re-send)

Oleg.

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Evgeniy Polyakov]

On Tue, Sep 29, 2009 at 03:47:21PM +0200, Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> Ok,  can confirm that this patch fixes my problem, but I am not sure if the
> intended behaviour is still working as expected.

Your patch breaks assumption that task_session(current->group_leader) is
not equal to new session id, but to check task_session() we need either
rcu or task lock. Also setsid() return value is not zero or negative
error, but new session ID or negative error, so I believe attached patch
is a proper fix, although it looks rather ugly.

Also proc_sid_connector() uses GFP_KERNEL allocation which is way too
wrong to use under any locks.

Something like this (not tested :)

diff --git a/kernel/exit.c b/kernel/exit.c
index 5859f59..1565baf 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
 {
 	struct task_struct *curr = current->group_leader;
 
-	if (task_session(curr) != pid) {
+	if (task_session(curr) != pid)
 		change_pid(curr, PIDTYPE_SID, pid);
-		proc_sid_connector(curr);
-	}
 
 	if (task_pgrp(curr) != pid)
 		change_pid(curr, PIDTYPE_PGID, pid);
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..b852a8b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1090,6 +1090,7 @@ SYSCALL_DEFINE0(setsid)
 	struct pid *sid = task_pid(group_leader);
 	pid_t session = pid_vnr(sid);
 	int err = -EPERM;
+	int send_cn = 0;
 
 	write_lock_irq(&tasklist_lock);
 	/* Fail if I am already a session leader */
@@ -1104,12 +1105,18 @@ SYSCALL_DEFINE0(setsid)
 
 	group_leader->signal->leader = 1;
 	__set_special_pids(sid);
+	if (task_session(group_leader) != sid)
+		send_cn = 1;
 
 	proc_clear_tty(group_leader);
 
 	err = session;
 out:
 	write_unlock_irq(&tasklist_lock);
+
+	if (send_cn)
+		proc_sid_connector(group_leader);
+
 	return err;
 }
 

-- 
	Evgeniy Polyakov

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Christian Borntraeger]

Am Dienstag 29 September 2009 16:07:18 schrieb Evgeniy Polyakov:
> Your patch breaks assumption that task_session(current->group_leader) is
> not equal to new session id, but to check task_session() we need either
> rcu or task lock. Also setsid() return value is not zero or negative
> error, but new session ID or negative error,

Right.

> so I believe attached patch is a proper fix, although it looks rather ugly.
> 
> Also proc_sid_connector() uses GFP_KERNEL allocation which is way too
> wrong to use under any locks.
> 
> Something like this (not tested :)

Patch compiles and seems to work.

Christian

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Oleg Nesterov]

On 09/29, Evgeniy Polyakov wrote:
>
> On Tue, Sep 29, 2009 at 03:47:21PM +0200, Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> > Ok,  can confirm that this patch fixes my problem, but I am not sure if the
> > intended behaviour is still working as expected.
>
> Your patch breaks assumption that task_session(current->group_leader) is
> not equal to new session id,

Afaics, no.

> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1090,6 +1090,7 @@ SYSCALL_DEFINE0(setsid)
>  	struct pid *sid = task_pid(group_leader);
>  	pid_t session = pid_vnr(sid);
>  	int err = -EPERM;
> +	int send_cn = 0;
>
>  	write_lock_irq(&tasklist_lock);
>  	/* Fail if I am already a session leader */
> @@ -1104,12 +1105,18 @@ SYSCALL_DEFINE0(setsid)
>
>  	group_leader->signal->leader = 1;
>  	__set_special_pids(sid);
> +	if (task_session(group_leader) != sid)
> +		send_cn = 1;

This is not right, task_session(group_leader) must be == sid after
__set_special_pids().

And I don't think "int send_cn" is needed. sys_setsid() must not
succeed if the caller lived in session == task_pid(group_leader).

Or I missed your point?

Oleg.

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Oleg Nesterov]

On 09/29, Oleg Nesterov wrote:
>
> On 09/29, Evgeniy Polyakov wrote:
> >
> > On Tue, Sep 29, 2009 at 03:47:21PM +0200, Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> > > Ok,  can confirm that this patch fixes my problem, but I am not sure if the
> > > intended behaviour is still working as expected.
> >
> > Your patch breaks assumption that task_session(current->group_leader) is
> > not equal to new session id,
>
> Afaics, no.
>
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -1090,6 +1090,7 @@ SYSCALL_DEFINE0(setsid)
> >  	struct pid *sid = task_pid(group_leader);
> >  	pid_t session = pid_vnr(sid);
> >  	int err = -EPERM;
> > +	int send_cn = 0;
> >
> >  	write_lock_irq(&tasklist_lock);
> >  	/* Fail if I am already a session leader */
> > @@ -1104,12 +1105,18 @@ SYSCALL_DEFINE0(setsid)
> >
> >  	group_leader->signal->leader = 1;
> >  	__set_special_pids(sid);
> > +	if (task_session(group_leader) != sid)
> > +		send_cn = 1;
>
> This is not right, task_session(group_leader) must be == sid after
> __set_special_pids().
>
> And I don't think "int send_cn" is needed. sys_setsid() must not
> succeed if the caller lived in session == task_pid(group_leader).

IOW, if sys_setsid() succeeds, we know it creates the new unique session,
we should report this change.

Note this check

	if (pid_task(sid, PIDTYPE_PGID))
		goto out;

before we actually change pids.

I think Christian's patch only needs the small fixup.

Oleg.

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Christian Borntraeger]

Am Dienstag 29 September 2009 16:45:54 schrieb Oleg Nesterov:
> I think Christian's patch only needs the small fixup.

Oleg, Evgeniy,

just in case the discussion concludes that my patch is fine,
here is a fixed version.

[PATCH] connector: Fix sid connector

The sid connector gives the following warning:
Badness at kernel/softirq.c:143
[...]
Call Trace:
([<000000013fe04100>] 0x13fe04100)
 [<000000000048a946>] sk_filter+0x9a/0xd0
 [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
 [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
 [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
 [<0000000000142604>] __set_special_pids+0x58/0x90
 [<0000000000159938>] sys_setsid+0xb4/0xd8
 [<00000000001187fe>] sysc_noemu+0x10/0x16
 [<00000041616cb266>] 0x41616cb266

The warning is
--->    WARN_ON_ONCE(in_irq() || irqs_disabled());

The network code must not be called with disabled interrupts but
sys_setsid holds the tasklist_lock with spinlock_irq while calling
the connector. We can safely move proc_sid_connector from
__set_special_pids to sys_setsid.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
 kernel/exit.c |    4 +---
 kernel/sys.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/kernel/exit.c
===================================================================
--- linux-2.6.orig/kernel/exit.c
+++ linux-2.6/kernel/exit.c
@@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
 {
 	struct task_struct *curr = current->group_leader;
 
-	if (task_session(curr) != pid) {
+	if (task_session(curr) != pid)
 		change_pid(curr, PIDTYPE_SID, pid);
-		proc_sid_connector(curr);
-	}
 
 	if (task_pgrp(curr) != pid)
 		change_pid(curr, PIDTYPE_PGID, pid);
Index: linux-2.6/kernel/sys.c
===================================================================
--- linux-2.6.orig/kernel/sys.c
+++ linux-2.6/kernel/sys.c
@@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
 	err = session;
 out:
 	write_unlock_irq(&tasklist_lock);
+	if (err > 0)
+		proc_sid_connector(sid);
 	return err;
 }
 

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Oleg Nesterov]

On 09/29, Christian Borntraeger wrote:
>
>
> The network code must not be called with disabled interrupts but
> sys_setsid holds the tasklist_lock with spinlock_irq while calling
> the connector. We can safely move proc_sid_connector from
> __set_special_pids to sys_setsid.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> ---
>  kernel/exit.c |    4 +---
>  kernel/sys.c  |    2 ++
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/kernel/exit.c
> ===================================================================
> --- linux-2.6.orig/kernel/exit.c
> +++ linux-2.6/kernel/exit.c
> @@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
>  {
>  	struct task_struct *curr = current->group_leader;
>
> -	if (task_session(curr) != pid) {
> +	if (task_session(curr) != pid)
>  		change_pid(curr, PIDTYPE_SID, pid);
> -		proc_sid_connector(curr);
> -	}
>
>  	if (task_pgrp(curr) != pid)
>  		change_pid(curr, PIDTYPE_PGID, pid);
> Index: linux-2.6/kernel/sys.c
> ===================================================================
> --- linux-2.6.orig/kernel/sys.c
> +++ linux-2.6/kernel/sys.c
> @@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
>  	err = session;
>  out:
>  	write_unlock_irq(&tasklist_lock);
> +	if (err > 0)
> +		proc_sid_connector(sid);
>  	return err;
>  }

Acked-by: Oleg Nesterov <oleg@redhat.com>

I'd suggest you to resend this patch to Andrew. Unless you know
another way to push it into Linus's tree ;)

Perhaps it makes sense to update the changelog, it should mention
the issues with daemonize().

Oleg.

[PATCH] connector: Fix regression introduced by sid connector

[Christian Borntraeger]

Hi Andrew,

since commit 02b51df1b07b4e9ca823c89284e704cadb323cd1 (proc connector: add event 
for process becoming session leader) we have the following warning:
Badness at kernel/softirq.c:143
[...]
Krnl PSW : 0404c00180000000 00000000001481d4 (local_bh_enable+0xb0/0xe0)
[...]
Call Trace:
([<000000013fe04100>] 0x13fe04100)
 [<000000000048a946>] sk_filter+0x9a/0xd0
 [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
 [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
 [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
 [<0000000000142604>] __set_special_pids+0x58/0x90
 [<0000000000159938>] sys_setsid+0xb4/0xd8
 [<00000000001187fe>] sysc_noemu+0x10/0x16
 [<00000041616cb266>] 0x41616cb266

The warning is
--->    WARN_ON_ONCE(in_irq() || irqs_disabled());

The network code must not be called with disabled interrupts but
sys_setsid holds the tasklist_lock with spinlock_irq while calling
the connector. 
After a discussion we agreed that we can move proc_sid_connector
from __set_special_pids to sys_setsid.
We also agreed that it is sufficient to change the check from
task_session(curr) != pid into err > 0, since if we don't change the
session, this means we were already the leader and return -EPERM.

One last thing:
There is also daemonize(), and some people might want to get a
notification in that case. Since daemonize() is only needed if a user
space does kernel_thread this does not look important (and there seems
to be no consensus if this connector should be called in daemonize). If
we really want this, we can add proc_sid_connector to daemonize() in an
additional patch (Scott?)

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CCed: Scott James Remnant <scott@ubuntu.com>
CCed: Matt Helsley <matthltc@us.ibm.com>
CCed: David S. Miller <davem@davemloft.net>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 kernel/exit.c |    4 +---
 kernel/sys.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/kernel/exit.c
===================================================================
--- linux-2.6.orig/kernel/exit.c
+++ linux-2.6/kernel/exit.c
@@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
 {
 	struct task_struct *curr = current->group_leader;
 
-	if (task_session(curr) != pid) {
+	if (task_session(curr) != pid)
 		change_pid(curr, PIDTYPE_SID, pid);
-		proc_sid_connector(curr);
-	}
 
 	if (task_pgrp(curr) != pid)
 		change_pid(curr, PIDTYPE_PGID, pid);
Index: linux-2.6/kernel/sys.c
===================================================================
--- linux-2.6.orig/kernel/sys.c
+++ linux-2.6/kernel/sys.c
@@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
 	err = session;
 out:
 	write_unlock_irq(&tasklist_lock);
+	if (err > 0)
+		proc_sid_connector(sid);
 	return err;
 }
 

Re: [PATCH] connector: Fix regression introduced by sid connector

[Andrew Morton]

On Wed, 30 Sep 2009 08:43:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Hi Andrew,
> 
> since commit 02b51df1b07b4e9ca823c89284e704cadb323cd1 (proc connector: add event 
> for process becoming session leader) we have the following warning:
> Badness at kernel/softirq.c:143
> [...]
> Krnl PSW : 0404c00180000000 00000000001481d4 (local_bh_enable+0xb0/0xe0)
> [...]
> Call Trace:
> ([<000000013fe04100>] 0x13fe04100)
>  [<000000000048a946>] sk_filter+0x9a/0xd0
>  [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
>  [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
>  [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
>  [<0000000000142604>] __set_special_pids+0x58/0x90
>  [<0000000000159938>] sys_setsid+0xb4/0xd8
>  [<00000000001187fe>] sysc_noemu+0x10/0x16
>  [<00000041616cb266>] 0x41616cb266
> 
> The warning is
> --->    WARN_ON_ONCE(in_irq() || irqs_disabled());
> 
> The network code must not be called with disabled interrupts but
> sys_setsid holds the tasklist_lock with spinlock_irq while calling
> the connector. 
> After a discussion we agreed that we can move proc_sid_connector
> from __set_special_pids to sys_setsid.
> We also agreed that it is sufficient to change the check from
> task_session(curr) != pid into err > 0, since if we don't change the
> session, this means we were already the leader and return -EPERM.
> 
> One last thing:
> There is also daemonize(), and some people might want to get a
> notification in that case. Since daemonize() is only needed if a user
> space does kernel_thread this does not look important (and there seems
> to be no consensus if this connector should be called in daemonize). If
> we really want this, we can add proc_sid_connector to daemonize() in an
> additional patch (Scott?)
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CCed: Scott James Remnant <scott@ubuntu.com>
> CCed: Matt Helsley <matthltc@us.ibm.com>
> CCed: David S. Miller <davem@davemloft.net>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
> ---
>  kernel/exit.c |    4 +---
>  kernel/sys.c  |    2 ++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/kernel/exit.c
> ===================================================================
> --- linux-2.6.orig/kernel/exit.c
> +++ linux-2.6/kernel/exit.c
> @@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
>  {
>  	struct task_struct *curr = current->group_leader;
>  
> -	if (task_session(curr) != pid) {
> +	if (task_session(curr) != pid)
>  		change_pid(curr, PIDTYPE_SID, pid);
> -		proc_sid_connector(curr);
> -	}
>  
>  	if (task_pgrp(curr) != pid)
>  		change_pid(curr, PIDTYPE_PGID, pid);
> Index: linux-2.6/kernel/sys.c
> ===================================================================
> --- linux-2.6.orig/kernel/sys.c
> +++ linux-2.6/kernel/sys.c
> @@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
>  	err = session;
>  out:
>  	write_unlock_irq(&tasklist_lock);
> +	if (err > 0)
> +		proc_sid_connector(sid);
>  	return err;
>  }

kernel/sys.c: In function 'sys_setsid':
kernel/sys.c:1114: warning: passing argument 1 of 'proc_sid_connector' from incompatible pointer type

Pass a `struct pid*' into a function expecting a `struct task_struct*'.
Surely it will crash??

Please redo the patch and test it more carefully.

Re: [PATCH] connector: Fix regression introduced by sid connector

[Oleg Nesterov]

On 10/01, Andrew Morton wrote:
>
> On Wed, 30 Sep 2009 08:43:11 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
> > --- linux-2.6.orig/kernel/sys.c
> > +++ linux-2.6/kernel/sys.c
> > @@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
> >  	err = session;
> >  out:
> >  	write_unlock_irq(&tasklist_lock);
> > +	if (err > 0)
> > +		proc_sid_connector(sid);
> >  	return err;
> >  }
>
> kernel/sys.c: In function 'sys_setsid':
> kernel/sys.c:1114: warning: passing argument 1 of 'proc_sid_connector' from incompatible pointer type
>
> Pass a `struct pid*' into a function expecting a `struct task_struct*'.
> Surely it will crash??

Oh, indeed, it should be

	proc_sid_connector(group_leader);

Oleg.

Re: [PATCH] connector: Fix regression introduced by sid connector

[Christian Borntraeger]

Sorry about that. Dont know how this escaped.  It was probably hiding between
all the sparse warnings I get in kernel/* and a lack of knowledge in this
area.
Here is a new version:



since commit 02b51df1b07b4e9ca823c89284e704cadb323cd1 (proc connector: add event 
for process becoming session leader) we have the following warning:
Badness at kernel/softirq.c:143
[...]
Krnl PSW : 0404c00180000000 00000000001481d4 (local_bh_enable+0xb0/0xe0)
[...]
Call Trace:
([<000000013fe04100>] 0x13fe04100)
 [<000000000048a946>] sk_filter+0x9a/0xd0
 [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
 [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
 [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
 [<0000000000142604>] __set_special_pids+0x58/0x90
 [<0000000000159938>] sys_setsid+0xb4/0xd8
 [<00000000001187fe>] sysc_noemu+0x10/0x16
 [<00000041616cb266>] 0x41616cb266

The warning is
--->    WARN_ON_ONCE(in_irq() || irqs_disabled());

The network code must not be called with disabled interrupts but
sys_setsid holds the tasklist_lock with spinlock_irq while calling
the connector. 
After a discussion we agreed that we can move proc_sid_connector
from __set_special_pids to sys_setsid.
We also agreed that it is sufficient to change the check from
task_session(curr) != pid into err > 0, since if we don't change the
session, this means we were already the leader and return -EPERM.

One last thing:
There is also daemonize(), and some people might want to get a
notification in that case. Since daemonize() is only needed if a user
space does kernel_thread this does not look important (and there seems
to be no consensus if this connector should be called in daemonize). If
we really want this, we can add proc_sid_connector to daemonize() in an
additional patch (Scott?)

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CCed: Scott James Remnant <scott@ubuntu.com>
CCed: Matt Helsley <matthltc@us.ibm.com>
CCed: David S. Miller <davem@davemloft.net>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 kernel/exit.c |    4 +---
 kernel/sys.c  |    2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/kernel/exit.c
===================================================================
--- linux-2.6.orig/kernel/exit.c
+++ linux-2.6/kernel/exit.c
@@ -359,10 +359,8 @@ void __set_special_pids(struct pid *pid)
 {
 	struct task_struct *curr = current->group_leader;
 
-	if (task_session(curr) != pid) {
+	if (task_session(curr) != pid)
 		change_pid(curr, PIDTYPE_SID, pid);
-		proc_sid_connector(curr);
-	}
 
 	if (task_pgrp(curr) != pid)
 		change_pid(curr, PIDTYPE_PGID, pid);
Index: linux-2.6/kernel/sys.c
===================================================================
--- linux-2.6.orig/kernel/sys.c
+++ linux-2.6/kernel/sys.c
@@ -1110,6 +1110,8 @@ SYSCALL_DEFINE0(setsid)
 	err = session;
 out:
 	write_unlock_irq(&tasklist_lock);
+	if (err > 0)
+		proc_sid_connector(group_leader);
 	return err;
 }
 

Re: [PATCH] connector: Fix regression introduced by sid connector

[David Rientjes]

On Fri, 2 Oct 2009, Christian Borntraeger wrote:

> since commit 02b51df1b07b4e9ca823c89284e704cadb323cd1 (proc connector: add event 
> for process becoming session leader) we have the following warning:
> Badness at kernel/softirq.c:143
> [...]
> Krnl PSW : 0404c00180000000 00000000001481d4 (local_bh_enable+0xb0/0xe0)
> [...]
> Call Trace:
> ([<000000013fe04100>] 0x13fe04100)
>  [<000000000048a946>] sk_filter+0x9a/0xd0
>  [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
>  [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
>  [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
>  [<0000000000142604>] __set_special_pids+0x58/0x90
>  [<0000000000159938>] sys_setsid+0xb4/0xd8
>  [<00000000001187fe>] sysc_noemu+0x10/0x16
>  [<00000041616cb266>] 0x41616cb266
> 
> The warning is
> --->    WARN_ON_ONCE(in_irq() || irqs_disabled());
> 
> The network code must not be called with disabled interrupts but
> sys_setsid holds the tasklist_lock with spinlock_irq while calling
> the connector. 
> After a discussion we agreed that we can move proc_sid_connector
> from __set_special_pids to sys_setsid.
> We also agreed that it is sufficient to change the check from
> task_session(curr) != pid into err > 0, since if we don't change the
> session, this means we were already the leader and return -EPERM.
> 
> One last thing:
> There is also daemonize(), and some people might want to get a
> notification in that case. Since daemonize() is only needed if a user
> space does kernel_thread this does not look important (and there seems
> to be no consensus if this connector should be called in daemonize). If
> we really want this, we can add proc_sid_connector to daemonize() in an
> additional patch (Scott?)
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CCed: Scott James Remnant <scott@ubuntu.com>
> CCed: Matt Helsley <matthltc@us.ibm.com>
> CCed: David S. Miller <davem@davemloft.net>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

Acked-by: David Rientjes <rientjes@google.com>

I was getting the same softirq warnings and later in slub in slab_alloc() 
at might_sleep_if(gfpflags & __GFP_WAIT) from __alloc_skb(..., GFP_KERNEL) 
in this context that would infinitely spam my log.

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Evgeniy Polyakov]

On Tue, Sep 29, 2009 at 05:12:33PM +0200, Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> just in case the discussion concludes that my patch is fine,
> here is a fixed version.
> 
> [PATCH] connector: Fix sid connector
> 
> The sid connector gives the following warning:
> Badness at kernel/softirq.c:143
> [...]
> Call Trace:
> ([<000000013fe04100>] 0x13fe04100)
>  [<000000000048a946>] sk_filter+0x9a/0xd0
>  [<000000000049d938>] netlink_broadcast+0x2c0/0x53c
>  [<00000000003ba9ae>] cn_netlink_send+0x272/0x2b0
>  [<00000000003baef0>] proc_sid_connector+0xc4/0xd4
>  [<0000000000142604>] __set_special_pids+0x58/0x90
>  [<0000000000159938>] sys_setsid+0xb4/0xd8
>  [<00000000001187fe>] sysc_noemu+0x10/0x16
>  [<00000041616cb266>] 0x41616cb266
> 
> The warning is
> --->    WARN_ON_ONCE(in_irq() || irqs_disabled());
> 
> The network code must not be called with disabled interrupts but
> sys_setsid holds the tasklist_lock with spinlock_irq while calling
> the connector. We can safely move proc_sid_connector from
> __set_special_pids to sys_setsid.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Looks good, thank you.
Ack.

-- 
	Evgeniy Polyakov

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Evgeniy Polyakov]

On Tue, Sep 29, 2009 at 04:25:38PM +0200, Oleg Nesterov (oleg@redhat.com) wrote:
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -1090,6 +1090,7 @@ SYSCALL_DEFINE0(setsid)
> >  	struct pid *sid = task_pid(group_leader);
> >  	pid_t session = pid_vnr(sid);
> >  	int err = -EPERM;
> > +	int send_cn = 0;
> >
> >  	write_lock_irq(&tasklist_lock);
> >  	/* Fail if I am already a session leader */
> > @@ -1104,12 +1105,18 @@ SYSCALL_DEFINE0(setsid)
> >
> >  	group_leader->signal->leader = 1;
> >  	__set_special_pids(sid);
> > +	if (task_session(group_leader) != sid)
> > +		send_cn = 1;
> 
> This is not right, task_session(group_leader) must be == sid after
> __set_special_pids().

Yeah, that check should be done before __set_special_pids().

> And I don't think "int send_cn" is needed. sys_setsid() must not
> succeed if the caller lived in session == task_pid(group_leader).

Doesn't it only check pgid while patch intention was to send
notification about sid? I.e. setsid() succeeds, although nothing
happens.

-- 
	Evgeniy Polyakov

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Oleg Nesterov]

On 09/29, Evgeniy Polyakov wrote:
>
> On Tue, Sep 29, 2009 at 04:25:38PM +0200, Oleg Nesterov (oleg@redhat.com) wrote:
> > > --- a/kernel/sys.c
> > > +++ b/kernel/sys.c
> > > @@ -1090,6 +1090,7 @@ SYSCALL_DEFINE0(setsid)
> > >  	struct pid *sid = task_pid(group_leader);
> > >  	pid_t session = pid_vnr(sid);
> > >  	int err = -EPERM;
> > > +	int send_cn = 0;
> > >
> > >  	write_lock_irq(&tasklist_lock);
> > >  	/* Fail if I am already a session leader */
> > > @@ -1104,12 +1105,18 @@ SYSCALL_DEFINE0(setsid)
> > >
> > >  	group_leader->signal->leader = 1;
> > >  	__set_special_pids(sid);
> > > +	if (task_session(group_leader) != sid)
> > > +		send_cn = 1;
> >
> > This is not right, task_session(group_leader) must be == sid after
> > __set_special_pids().
>
> Yeah, that check should be done before __set_special_pids().
>
> > And I don't think "int send_cn" is needed. sys_setsid() must not
> > succeed if the caller lived in session == task_pid(group_leader).
>
> Doesn't it only check pgid while patch intention was to send
> notification about sid?

If the proposed sid already was the session id, then prgp shouldn't
be empty.

but this doesn't really matter, we also check ->signal->leader
(not sure, but afaics this check is not strictly necessary because
 of PIDTYPE_PGID check)

> I.e. setsid() succeeds, although nothing
> happens.

This shouldn't happen, or sys_setsid() is buggy. Look, the new session
id is task_pid(current). If sys_setsid() succeeds but we don't change
the session, this means we were already the leader. In that case we
should return -EPERM.

Oleg.

Re: [PATCH] connector: Fix sid connector (was: Badness at kernel/softirq.c:143...)

[Evgeniy Polyakov]

On Tue, Sep 29, 2009 at 05:36:31PM +0200, Oleg Nesterov (oleg@redhat.com) wrote:
> > Doesn't it only check pgid while patch intention was to send
> > notification about sid?
> 
> If the proposed sid already was the session id, then prgp shouldn't
> be empty.
> 
> but this doesn't really matter, we also check ->signal->leader
> (not sure, but afaics this check is not strictly necessary because
>  of PIDTYPE_PGID check)

Ok, I see, thanks.

-- 
	Evgeniy Polyakov