| Message ID | 20090929140718.GA23858@ioremap.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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; }