* [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits [not found] <4B040A03.2020508@gmail.com> @ 2009-11-18 14:51 ` Jiri Slaby 2009-11-18 18:56 ` Luck, Tony 0 siblings, 1 reply; 4+ messages in thread From: Jiri Slaby @ 2009-11-18 14:51 UTC (permalink / raw) To: jirislaby Cc: mingo, nhorman, sfr, linux-kernel, akpm, marcin.slusarz, tglx, mingo, hpa, torvalds, Jiri Slaby, James Morris, Heiko Carstens, linux-ia64 Make sure compiler won't do weird things with limits. E.g. fetching them twice may return 2 different values after writable limits are implemented. Signed-off-by: Jiri Slaby <jslaby@novell.com> Cc: James Morris <jmorris@namei.org> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: linux-ia64@vger.kernel.org --- arch/ia64/kernel/perfmon.c | 2 +- arch/ia64/kernel/sys_ia64.c | 2 +- arch/ia64/mm/init.c | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c index f178270..91b8607 100644 --- a/arch/ia64/kernel/perfmon.c +++ b/arch/ia64/kernel/perfmon.c @@ -2298,7 +2298,7 @@ pfm_smpl_buffer_alloc(struct task_struct *task, struct file *filp, pfm_context_t * if ((mm->total_vm << PAGE_SHIFT) + len> task->rlim[RLIMIT_AS].rlim_cur) * return -ENOMEM; */ - if (size > task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur) + if (size > ACCESS_ONCE(task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur)) return -ENOMEM; /* diff --git a/arch/ia64/kernel/sys_ia64.c b/arch/ia64/kernel/sys_ia64.c index 92ed83f..6a2b5d9 100644 --- a/arch/ia64/kernel/sys_ia64.c +++ b/arch/ia64/kernel/sys_ia64.c @@ -129,7 +129,7 @@ ia64_brk (unsigned long brk) goto out; /* Check against rlimit.. */ - rlim = current->signal->rlim[RLIMIT_DATA].rlim_cur; + rlim = ACCESS_ONCE(current->signal->rlim[RLIMIT_DATA].rlim_cur); if (rlim < RLIM_INFINITY && brk - mm->start_data > rlim) goto out; diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c index 1857766..fe6d63f 100644 --- a/arch/ia64/mm/init.c +++ b/arch/ia64/mm/init.c @@ -91,7 +91,8 @@ dma_mark_clean(void *addr, size_t size) inline void ia64_set_rbs_bot (void) { - unsigned long stack_size = current->signal->rlim[RLIMIT_STACK].rlim_max & -16; + unsigned long stack_size = ACCESS_ONCE(current->signal-> + rlim[RLIMIT_STACK].rlim_max) & -16; if (stack_size > MAX_USER_STACK_SIZE) stack_size = MAX_USER_STACK_SIZE; -- 1.6.4.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits 2009-11-18 14:51 ` [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits Jiri Slaby @ 2009-11-18 18:56 ` Luck, Tony 2009-11-18 19:48 ` Linus Torvalds 0 siblings, 1 reply; 4+ messages in thread From: Luck, Tony @ 2009-11-18 18:56 UTC (permalink / raw) To: Jiri Slaby, jirislaby@gmail.com Cc: mingo@elte.hu, nhorman@tuxdriver.com, sfr@canb.auug.org.au, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, marcin.slusarz@gmail.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, torvalds@linux-foundation.org, James Morris, Heiko Carstens, linux-ia64@vger.kernel.org > Make sure compiler won't do weird things with limits. E.g. fetching > them twice may return 2 different values after writable limits are > implemented. - if (size > task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur) + if (size > ACCESS_ONCE(task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur)) I don't see how this helps. If someone else is changing limits while we are looking at them, then there is a race. We either get the old or the new value. Using ACCESS_ONCE (which on ia64 forces a "volatile" access, which will make the compiler generate "ld.acq" rather than a plain "ld") won't make any difference to this race. Please explain what issue you see with the current code. -Tony ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits 2009-11-18 18:56 ` Luck, Tony @ 2009-11-18 19:48 ` Linus Torvalds 2009-11-19 2:28 ` Ingo Molnar 0 siblings, 1 reply; 4+ messages in thread From: Linus Torvalds @ 2009-11-18 19:48 UTC (permalink / raw) To: Luck, Tony Cc: Jiri Slaby, jirislaby@gmail.com, mingo@elte.hu, nhorman@tuxdriver.com, sfr@canb.auug.org.au, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, marcin.slusarz@gmail.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, James Morris, Heiko Carstens, linux-ia64@vger.kernel.org On Wed, 18 Nov 2009, Luck, Tony wrote: > > > Make sure compiler won't do weird things with limits. E.g. fetching > > them twice may return 2 different values after writable limits are > > implemented. > > - if (size > task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur) > + if (size > ACCESS_ONCE(task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur)) > > I don't see how this helps. If someone else is changing limits while > we are looking at them, then there is a race. We either get the old > or the new value. Using ACCESS_ONCE (which on ia64 forces a "volatile" > access, which will make the compiler generate "ld.acq" rather than a > plain "ld") won't make any difference to this race. > > Please explain what issue you see with the current code. The problem may not be in _that_ particular code, but imagine code like this: if (a > MEMORY) { do1; do2; do3; } else { do2; } where the compiler could actually turn this into (having noticed that neither "do1" nor "do2" can alias with MEMORY): if (a > MEMORY) do1; do2; if (a > MEMORY) do3; and now what you end up having is a situation where it's possible that "do1" gets executed but "do3" does not (or vice versa). Notice how when you look at the code, it looks impossible, and then you get subtle security bugs. Now, you may say that "but _my_ code doesn't have that "else" statement", and maybe you're right. In fact, maybe the source code was really just if (a > MEMORY) return something(); return do_something_else(); and you are _sure_ that the ACCESS_ONCE() cannot possibly be needed. But what if those 'something()' and 'do_something_else()' were inlines, and the compiler internally turns it into if (a > MEMORY) { ret = something(); } else { ret = do_something_else(); } return ret; and you now hit the case above where part of it was shared after all, and the compiler for some strange reason (register reload, whatever) ends up doing it as two conditionals after all? The thing is, you can't _prove_ that the compiler won't do it, especially if you end up changing the code later (without thinking about the fact that you're loading things without locking). So the rule is: if you access unlocked values, you use ACCESS_ONCE(). You don't say "but it can't matter". Because you simply don't know. Linus ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits 2009-11-18 19:48 ` Linus Torvalds @ 2009-11-19 2:28 ` Ingo Molnar 0 siblings, 0 replies; 4+ messages in thread From: Ingo Molnar @ 2009-11-19 2:28 UTC (permalink / raw) To: Linus Torvalds Cc: Luck, Tony, Jiri Slaby, jirislaby@gmail.com, nhorman@tuxdriver.com, sfr@canb.auug.org.au, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, marcin.slusarz@gmail.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, James Morris, Heiko Carstens, linux-ia64@vger.kernel.org * Linus Torvalds <torvalds@linux-foundation.org> wrote: > So the rule is: if you access unlocked values, you use ACCESS_ONCE(). > You don't say "but it can't matter". Because you simply don't know. Most of the time we are being lax about it, especially when it's some global value we are accessing, which can only be changed as a sysadmin via a sysctl or so. [ For example we access pid_max in kernel/pid.c, outside of any lock and without ACCESS_ONCE() - but that particular case is not a big deal because changes to pid_max via a sysctl are so rare and are privileged, and because the effects of any race there are benign. ] But this patch series is about setrlimit, which makes the per task rlimit value pretty SMP-volatile (a parallel, unprivileged setrlimit can race with usage of the value elsewhere) - and the rlimits have security relevance as well so some extra care in accessing them outside of locks is prudent IMO. Ingo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-11-19 2:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4B040A03.2020508@gmail.com>
2009-11-18 14:51 ` [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits Jiri Slaby
2009-11-18 18:56 ` Luck, Tony
2009-11-18 19:48 ` Linus Torvalds
2009-11-19 2:28 ` Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox