linux-kernel - Re: [PATCH 1/5] user_hooks: New user hooks subsystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120730155059.GB17078@somewhere.redhat.com>
Date:	Mon, 30 Jul 2012 17:51:02 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Alessio Igor Bogani <abogani@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Avi Kivity <avi@...hat.com>,
	Chris Metcalf <cmetcalf@...era.com>,
	Christoph Lameter <cl@...ux.com>,
	Geoff Levand <geoff@...radead.org>,
	Gilad Ben Yossef <gilad@...yossef.com>,
	Hakan Akkan <hakanakkan@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
	Kevin Hilman <khilman@...com>,
	Max Krasnyansky <maxk@...lcomm.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Stephen Hemminger <shemminger@...tta.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Sven-Thorsten Dietrich <thebigcorporation@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 1/5] user_hooks: New user hooks subsystem

On Mon, Jul 30, 2012 at 05:08:12PM +0200, Peter Zijlstra wrote:
> On Fri, 2012-07-27 at 17:40 +0200, Frederic Weisbecker wrote:
> > +++ b/kernel/user_hooks.c
> > @@ -0,0 +1,56 @@
> > +#include <linux/user_hooks.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/sched.h>
> > +#include <linux/percpu.h>
> > +
> > +struct user_hooks {
> > +       bool hooking;
> > +       bool in_user;
> > +};
> 
> I really detest using bool in structures.. but that's just me. Also this
> really wants a comment as to wtf 'hooking' means. in_user I can just
> about guess.

I really don't mind changing that to int. I just like them as bool because
they better describe the purpose of the field.

hooking means that the hooks are set (the TIF flag is set on the current task
and we also handle the exception hooks).

I can call that is_hooking instead? And/or add a comment to explain the
purpose of this.

> 
> > +DEFINE_PER_CPU(struct user_hooks, user_hooks) = {
> > +#ifdef CONFIG_USER_HOOKS_FORCE
> > +       .hooking = true,
> > +#endif
> > +};
> > +
> > +void user_enter(void)
> > +{
> > +       unsigned long flags;
> > +       struct user_hooks *uh;
> > +
> > +       WARN_ON_ONCE(!current->mm);
> > +       local_irq_save(flags);
> > +       uh = &__get_cpu_var(user_hooks);
> > +       if (uh->hooking && !uh->in_user) {
> > +               uh->in_user = true;
> > +               rcu_user_enter();
> > +       }
> 
> By not using __get_cpu_var() but __this_cpu_*() you generate much better
> code (esp. on x86).
> 
> IOW. something like:
> 
>   if (__this_cpu_read(uh.hooking) && !__this_cpu_read(uh.in_user)) {
> 	__this_cpu_write(uh.in_user, true);
> 	rcu_user_enter();
>   }

Ok, I'll replace.

> 
> > +       local_irq_restore(flags);
> > +}
> > +
> > +void user_exit(void)
> > +{
> > +       unsigned long flags;
> > +       struct user_hooks *uh;
> > +
> > +       local_irq_save(flags);
> > +       uh = &__get_cpu_var(user_hooks);
> > +       if (uh->in_user) {
> > +               uh->in_user = false;
> > +               rcu_user_exit();
> > +       }
> > +       local_irq_restore(flags);
> > +}
> > +
> > +void user_hooks_switch(struct task_struct *prev,
> > +                      struct task_struct *next)
> > +{
> > +       struct user_hooks *uh;
> > +
> > +       uh = &__get_cpu_var(user_hooks);
> > +       if (uh->hooking) {
> > +               clear_tsk_thread_flag(prev, TIF_NOHZ);
> > +               set_tsk_thread_flag(next, TIF_NOHZ);
> > +       }
> 
> This seems pointless to me.. why are we flipping that flag on context
> switch instead of keeping it enabled at all times? This are two atomic
> ops in the context switch path, why?

Because the hooks are per cpu. If we activate the hooks on CPU 1 but not
on CPU 2 and prev was running on CPU 1 and migrates on CPU 2, it's going
to keep the hook on that CPU 2 if we don't clear the flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/