From: Mike Galbraith <efault@gmx.de>
To: Oleg Nesterov <oleg@redhat.com>
Cc: linux-tip-commits@vger.kernel.org, linux-kernel@vger.kernel.org,
hpa@zytor.com, mingo@redhat.com, mathieu.desnoyers@efficios.com,
a.p.zijlstra@chello.nl, torvalds@linux-foundation.org,
pjt@google.com, markus@trippelsdorf.de, tglx@linutronix.de,
mingo@elte.hu
Subject: Re: [tip:sched/core] sched: Add 'autogroup' scheduling feature: automated per session task groups
Date: Thu, 16 Dec 2010 08:53:56 +0100 [thread overview]
Message-ID: <1292486036.10931.314.camel@maggy.simson.net> (raw)
In-Reply-To: <20101215175010.GA14267@redhat.com>
On Wed, 2010-12-15 at 18:50 +0100, Oleg Nesterov wrote:
> I assume this is the latest version. In this case I think it needs
> minor fixes.
>
> > +#ifdef CONFIG_PROC_FS
> > +
> > +/* Called with siglock held. */
>
> This is not true, and that is why we can't blindly use kref_get().
I was going to lock it all up, but convinced myself it wasn't necessary.
The comment should have also gone away.
> > +int proc_sched_autogroup_set_nice(struct task_struct *p, int *nice)
> > +{
> > + static unsigned long next = INITIAL_JIFFIES;
> > + struct autogroup *ag;
> > + int err;
> > +
> > + if (*nice < -20 || *nice > 19)
> > + return -EINVAL;
> > +
> > + err = security_task_setnice(current, *nice);
> > + if (err)
> > + return err;
> > +
> > + if (*nice < 0 && !can_nice(current, *nice))
> > + return -EPERM;
> > +
> > + /* this is a heavy operation taking global locks.. */
> > + if (!capable(CAP_SYS_ADMIN) && time_before(jiffies, next))
> > + return -EAGAIN;
> > +
> > + next = HZ / 10 + jiffies;
> > + ag = autogroup_kref_get(p->signal->autogroup);
>
> We can race with autogroup_move_group() and use the already freed
> ->autogroup. We need ->siglock or task_rq_lock() to read it.
I don't see how/why. I took a reference to the new group before
assignment of p->signal->autogroup, and put the previous group after
it's assigned.
Ponders that.. uhoh.
Mover does atomic write, but signal->autogroup write comes after that,
so can still be in flight when reader dereferences. Game over unless
the reader beats ->autogroup writer to the punch.
Thanks again for your excellent eyeballs. The below should plug that
hole, no? (hope so, seems pointless to lock movement)
> IOW, I think we need something like the patch below, but - sorry -
> if was completely untested.
>
> And the question,
>
> > + down_write(&ag->lock);
> > + err = sched_group_set_shares(ag->tg, prio_to_weight[*nice + 20]);
>
> Do we really want this if ag == autogroup_default ? Say, autogroup_create()
> fails, now the owner of this process can affect init_task_group. Or admin
> can change init_task_group "by accident" (although currently this is hardly
> possible, sched_autogroup_detach() has no callers). Just curious.
sched_group_set_shares() does the right thing, says no to changing the
root task group's shares.
sched: fix potential access to freed memory
Oleg pointed out that the /proc interface kref_get() useage may race with
the final put during autogroup_move_group(). A signal->autogroup assignment
may be in flight when the /proc interface dereference, leaving them taking
a reference to an already dead group.
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Mike Galbraith <efault@gmx.de>
diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 57a7ac2..713b6c0 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -41,6 +41,12 @@ static inline struct autogroup *autogroup_kref_get(struct autogroup *ag)
return ag;
}
+static inline struct autogroup *autogroup_task_get(struct task_struct *p)
+{
+ smp_rmb();
+ return autogroup_kref_get(p->signal->autogroup);
+}
+
static inline struct autogroup *autogroup_create(void)
{
struct autogroup *ag = kzalloc(sizeof(*ag), GFP_KERNEL);
@@ -119,6 +125,7 @@ autogroup_move_group(struct task_struct *p, struct autogroup *ag)
}
p->signal->autogroup = autogroup_kref_get(ag);
+ smp_mb();
t = p;
do {
@@ -172,7 +179,6 @@ __setup("noautogroup", setup_autogroup);
#ifdef CONFIG_PROC_FS
-/* Called with siglock held. */
int proc_sched_autogroup_set_nice(struct task_struct *p, int *nice)
{
static unsigned long next = INITIAL_JIFFIES;
@@ -194,7 +200,7 @@ int proc_sched_autogroup_set_nice(struct task_struct *p, int *nice)
return -EAGAIN;
next = HZ / 10 + jiffies;
- ag = autogroup_kref_get(p->signal->autogroup);
+ ag = autogroup_task_get(p);
down_write(&ag->lock);
err = sched_group_set_shares(ag->tg, prio_to_weight[*nice + 20]);
@@ -209,7 +215,7 @@ int proc_sched_autogroup_set_nice(struct task_struct *p, int *nice)
void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
{
- struct autogroup *ag = autogroup_kref_get(p->signal->autogroup);
+ struct autogroup *ag = autogroup_task_get(p);
down_read(&ag->lock);
seq_printf(m, "/autogroup-%ld nice %d\n", ag->id, ag->nice);
next prev parent reply other threads:[~2010-12-16 7:54 UTC|newest]
Thread overview: 264+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-19 9:16 [RFC/RFT PATCH] sched: automated per tty task groups Mike Galbraith
2010-10-19 9:26 ` Peter Zijlstra
2010-10-19 9:39 ` Mike Galbraith
2010-10-19 9:43 ` Peter Zijlstra
2010-10-19 9:46 ` Mike Galbraith
2010-10-21 7:55 ` Mike Galbraith
2010-10-21 10:28 ` Peter Zijlstra
2010-10-19 9:29 ` Peter Zijlstra
2010-10-19 9:42 ` Mike Galbraith
2010-10-19 11:29 ` Mike Galbraith
2010-10-19 11:56 ` Ingo Molnar
2010-10-19 13:12 ` Mike Galbraith
2010-10-19 15:28 ` Linus Torvalds
2010-10-19 18:13 ` Mike Galbraith
2010-10-19 18:53 ` Mike Galbraith
2010-10-20 2:56 ` Ingo Molnar
2010-10-21 8:11 ` Mike Galbraith
2010-10-21 8:31 ` Ingo Molnar
2010-10-21 8:39 ` Mike Galbraith
2010-10-21 8:48 ` Markus Trippelsdorf
2010-10-21 8:52 ` Mike Galbraith
[not found] ` <20101021115723.GA1587@arch.trippelsdorf.de>
2010-10-21 16:22 ` Mathieu Desnoyers
2010-10-21 10:51 ` Mathieu Desnoyers
2010-10-21 11:25 ` Peter Zijlstra
2010-10-21 16:29 ` Oleg Nesterov
2010-10-21 19:11 ` Mike Galbraith
2010-10-26 7:07 ` [RFC/RFT PATCH v3] " Mike Galbraith
2010-10-26 7:29 ` Mike Galbraith
2010-10-26 15:47 ` Linus Torvalds
2010-10-27 1:58 ` Mike Galbraith
2010-11-11 15:26 ` Mike Galbraith
2010-11-11 18:04 ` Ingo Molnar
2010-11-11 18:34 ` Linus Torvalds
2010-11-11 19:08 ` Mike Galbraith
2010-11-11 19:37 ` Linus Torvalds
2010-11-11 20:29 ` Oleg Nesterov
2010-11-11 19:15 ` Markus Trippelsdorf
2010-11-11 19:35 ` Mike Galbraith
2010-11-11 19:38 ` Markus Trippelsdorf
2010-11-11 19:58 ` Mike Galbraith
2010-11-11 20:27 ` Oleg Nesterov
2010-11-11 22:20 ` Mike Galbraith
2010-11-12 18:12 ` Oleg Nesterov
2010-11-13 11:42 ` Mike Galbraith
2010-11-14 17:19 ` Mike Galbraith
2010-11-14 17:49 ` Markus Trippelsdorf
2010-11-14 18:10 ` Mike Galbraith
2010-11-14 19:28 ` Linus Torvalds
2010-11-14 20:20 ` Linus Torvalds
2010-11-14 20:27 ` Markus Trippelsdorf
2010-11-14 20:48 ` Linus Torvalds
2010-11-14 23:43 ` Mike Galbraith
2010-11-15 0:15 ` Linus Torvalds
2010-11-15 0:26 ` Linus Torvalds
2010-11-15 1:13 ` Mike Galbraith
2010-11-15 3:12 ` Linus Torvalds
2010-11-15 14:00 ` Mike Galbraith
2010-11-15 8:57 ` Peter Zijlstra
2010-11-15 11:32 ` Mike Galbraith
2010-11-15 11:46 ` Mike Galbraith
2010-11-15 12:57 ` Oleg Nesterov
2010-11-15 21:25 ` Mike Galbraith
2010-11-15 22:48 ` Peter Zijlstra
2010-11-16 1:56 ` Vivek Goyal
2010-11-16 2:18 ` Linus Torvalds
2010-11-17 8:06 ` Balbir Singh
2010-11-16 14:02 ` Mike Galbraith
2010-11-16 14:11 ` Peter Zijlstra
2010-11-16 14:47 ` Dhaval Giani
2010-11-16 17:03 ` Lennart Poettering
2010-11-16 17:11 ` Linus Torvalds
2010-11-16 18:16 ` Lennart Poettering
2010-11-16 18:21 ` Peter Zijlstra
2010-11-16 18:33 ` Paul Menage
2010-11-16 18:55 ` david
2010-11-16 18:59 ` Peter Zijlstra
2010-11-16 19:09 ` Vivek Goyal
2010-11-16 19:13 ` Peter Zijlstra
2010-11-16 19:22 ` Vivek Goyal
2010-11-16 19:25 ` Peter Zijlstra
2010-11-16 19:40 ` Vivek Goyal
2010-11-16 19:43 ` Peter Zijlstra
2010-11-16 19:49 ` Linus Torvalds
2010-11-16 19:35 ` Linus Torvalds
2010-11-16 20:03 ` Lennart Poettering
2010-11-16 20:12 ` Peter Zijlstra
2010-11-16 18:49 ` Linus Torvalds
2010-11-16 19:03 ` Pekka Enberg
2010-11-16 20:21 ` Kay Sievers
2010-11-16 20:35 ` Linus Torvalds
2010-11-16 20:31 ` Lennart Poettering
2010-11-17 13:21 ` Stephen Clark
2010-11-16 19:08 ` david
2010-11-16 20:33 ` Lennart Poettering
2010-11-16 20:38 ` Linus Torvalds
2010-11-16 21:14 ` Lennart Poettering
2010-11-17 13:23 ` Stephen Clark
2010-11-18 22:33 ` Hans-Peter Jansen
2010-11-18 23:12 ` Samuel Thibault
2010-11-18 23:35 ` Mike Galbraith
2010-11-18 23:43 ` Samuel Thibault
2010-11-18 23:51 ` Linus Torvalds
2010-11-19 0:02 ` Samuel Thibault
2010-11-19 0:07 ` Samuel Thibault
2010-11-19 11:57 ` Peter Zijlstra
2010-11-19 14:24 ` Samuel Thibault
2010-11-19 14:43 ` Peter Zijlstra
2010-11-19 14:55 ` Samuel Thibault
2010-11-19 0:42 ` Linus Torvalds
2010-11-19 0:59 ` Samuel Thibault
2010-11-19 1:11 ` Linus Torvalds
2010-11-19 1:12 ` Mike Galbraith
2010-11-19 1:23 ` Samuel Thibault
2010-11-19 2:28 ` Mike Galbraith
2010-11-19 9:02 ` Samuel Thibault
2010-11-19 11:49 ` Peter Zijlstra
2010-11-19 12:19 ` Peter Zijlstra
2010-11-19 12:55 ` Mathieu Desnoyers
2010-11-19 13:00 ` Peter Zijlstra
2010-11-19 13:20 ` Mathieu Desnoyers
2010-11-19 12:31 ` Paul Menage
2010-11-19 12:51 ` Peter Zijlstra
2010-11-19 13:03 ` Mike Galbraith
2010-11-19 12:38 ` Mike Galbraith
2010-11-22 6:22 ` Balbir Singh
2010-11-18 23:29 ` Mike Galbraith
2010-11-16 20:44 ` Pekka Enberg
2010-11-16 19:27 ` Dhaval Giani
2010-11-16 19:42 ` Diego Calleja
2010-11-16 19:45 ` Linus Torvalds
2010-11-16 19:56 ` Paul Menage
2010-11-16 20:17 ` Vivek Goyal
2010-11-16 20:50 ` Lennart Poettering
2010-11-20 22:16 ` Mika Laitio
2010-11-21 0:19 ` Mike Galbraith
2010-11-16 20:28 ` Lennart Poettering
2010-11-16 20:46 ` David Miller
2010-11-16 21:08 ` Lennart Poettering
2010-11-16 21:14 ` David Miller
2010-11-16 20:52 ` Alan Cox
2010-11-16 21:08 ` Linus Torvalds
2010-11-16 21:19 ` Lennart Poettering
2010-11-16 23:39 ` Ted Ts'o
2010-11-17 0:21 ` Lennart Poettering
2010-11-17 2:06 ` Ted Ts'o
2010-11-17 14:57 ` Vivek Goyal
2010-11-17 15:01 ` Lennart Poettering
2010-11-17 17:16 ` John Stoffel
2010-11-19 5:20 ` Andev
2010-11-19 11:59 ` Peter Zijlstra
2010-11-19 13:03 ` Ben Gamari
2010-11-19 13:07 ` Theodore Tso
2010-11-19 16:29 ` David Miller
2010-11-19 16:34 ` Lennart Poettering
2010-11-19 16:43 ` David Miller
2010-11-19 17:51 ` Linus Torvalds
2010-11-19 19:12 ` Ben Gamari
2010-11-19 19:48 ` Linus Torvalds
2010-11-20 1:33 ` Lennart Poettering
2010-11-19 20:38 ` Paul Menage
2010-11-20 1:13 ` Lennart Poettering
2010-11-20 4:25 ` Balbir Singh
2010-11-20 15:41 ` Lennart Poettering
2010-11-22 6:24 ` Balbir Singh
2010-11-22 19:21 ` Lennart Poettering
2010-11-19 19:31 ` Mike Galbraith
2010-11-19 13:21 ` Peter Zijlstra
2010-11-17 22:34 ` Lennart Poettering
2010-11-17 22:37 ` Peter Zijlstra
2010-11-17 22:45 ` Lennart Poettering
2010-11-17 22:52 ` Peter Zijlstra
2010-11-18 15:00 ` Stephen Clark
2010-11-17 23:49 ` Lennart Poettering
2010-11-16 21:17 ` Lennart Poettering
2010-11-17 20:59 ` James Cloos
2010-11-22 6:16 ` Balbir Singh
2010-11-16 18:57 ` Stephen Clark
2010-11-16 19:12 ` Vivek Goyal
2010-11-16 19:57 ` Mike Galbraith
2010-11-16 20:36 ` Lennart Poettering
2010-11-16 19:42 ` Markus Trippelsdorf
2010-11-16 18:08 ` Peter Zijlstra
2010-11-16 18:56 ` Stephen Clark
2010-11-16 20:05 ` Lennart Poettering
2010-11-16 20:15 ` Peter Zijlstra
2010-11-19 0:35 ` H. Peter Anvin
2010-11-19 0:42 ` Samuel Thibault
2010-11-19 3:15 ` Mathieu Desnoyers
[not found] ` <20101120090955.GB12043@balbir.in.ibm.com>
2010-11-20 19:47 ` Mike Galbraith
2010-11-16 13:04 ` Oleg Nesterov
2010-11-16 14:18 ` Mike Galbraith
2010-11-16 15:03 ` Oleg Nesterov
2010-11-16 15:41 ` Mike Galbraith
2010-11-16 17:28 ` Ingo Molnar
2010-11-16 17:42 ` Mike Galbraith
2010-11-20 19:35 ` [PATCH v4] sched: automated per session " Mike Galbraith
2010-11-30 15:39 ` [tip:sched/core] sched: Add 'autogroup' scheduling feature: " tip-bot for Mike Galbraith
2010-12-15 17:50 ` Oleg Nesterov
2010-12-16 7:53 ` Mike Galbraith [this message]
2010-12-16 14:09 ` Mike Galbraith
2010-12-16 15:07 ` Oleg Nesterov
2011-01-04 14:18 ` [tip:sched/core] sched, autogroup: Fix potential access to freed memory tip-bot for Mike Galbraith
2010-12-20 13:08 ` [tip:sched/core] sched: Add 'autogroup' scheduling feature: automated per session task groups Bharata B Rao
2010-12-20 13:19 ` Peter Zijlstra
2010-12-20 15:46 ` Bharata B Rao
2010-12-20 15:53 ` Bharata B Rao
2010-12-21 8:33 ` Peter Zijlstra
2010-12-20 16:39 ` Mike Galbraith
2010-12-21 5:04 ` Bharata B Rao
2010-12-21 5:50 ` Mike Galbraith
2010-12-04 17:39 ` [PATCH v4] sched: " Colin Walters
2010-12-04 18:33 ` Linus Torvalds
2010-12-04 20:01 ` Colin Walters
2010-12-04 22:39 ` Linus Torvalds
2010-12-04 23:43 ` Colin Walters
2010-12-05 0:31 ` Linus Torvalds
2010-12-05 7:47 ` Ray Lee
2010-12-05 19:22 ` Colin Walters
2010-12-05 20:47 ` Linus Torvalds
2010-12-05 22:47 ` Colin Walters
2010-12-05 22:58 ` Jesper Juhl
2010-12-05 23:05 ` Jesper Juhl
2010-12-07 18:51 ` Peter Zijlstra
2010-12-05 10:18 ` Con Kolivas
2010-12-05 11:36 ` Mike Galbraith
2010-12-05 20:58 ` Ingo Molnar
2010-12-04 23:31 ` david
2010-12-05 11:11 ` Nikos Chantziaras
2010-12-05 15:12 ` [PATCH v4] Regression: " Alan Cox
2010-12-05 16:16 ` Florian Mickler
2010-12-05 19:48 ` Alan Cox
2010-12-06 16:03 ` Florian Mickler
2010-12-05 16:59 ` Mike Galbraith
2010-12-05 17:09 ` Mike Galbraith
2010-12-05 17:15 ` Mike Galbraith
2010-12-06 0:28 ` [PATCH v4] " Valdis.Kletnieks
2010-11-16 14:01 ` [RFC/RFT PATCH v3] sched: automated per tty " Peter Zijlstra
2010-11-16 14:19 ` Mike Galbraith
2010-11-17 1:31 ` Kyle McMartin
2010-11-17 1:50 ` Linus Torvalds
2010-11-17 1:56 ` Kyle McMartin
2010-11-17 2:14 ` Mike Galbraith
2010-11-15 0:02 ` Mike Galbraith
2010-11-15 22:41 ` Valdis.Kletnieks
2010-11-15 23:25 ` Linus Torvalds
2010-11-20 19:33 ` Jesper Juhl
2010-11-20 19:51 ` Mike Galbraith
2010-11-20 20:37 ` Jesper Juhl
2010-11-20 22:02 ` Konstantin Svist
2010-11-20 22:15 ` Samuel Thibault
2010-11-20 22:18 ` Thomas Fjellstrom
2010-11-20 20:25 ` Samuel Thibault
2010-11-15 23:46 ` Mike Galbraith
2010-11-15 23:50 ` Linus Torvalds
2010-11-16 0:04 ` Mike Galbraith
2010-11-16 1:18 ` Linus Torvalds
2010-11-16 1:55 ` Paul Menage
2010-11-16 12:58 ` Mike Galbraith
2010-11-16 18:25 ` Paul Menage
2010-11-16 13:59 ` Peter Zijlstra
2010-11-16 14:26 ` Mike Galbraith
2010-10-21 11:27 ` [RFC/RFT PATCH] " Mike Galbraith
2010-10-20 13:55 ` Markus Trippelsdorf
2010-10-20 14:41 ` Mike Galbraith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1292486036.10931.314.camel@maggy.simson.net \
--to=efault@gmx.de \
--cc=a.p.zijlstra@chello.nl \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tip-commits@vger.kernel.org \
--cc=markus@trippelsdorf.de \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@elte.hu \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=pjt@google.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).