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]

Date:	Mon, 30 Jul 2012 17:08:12 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Frederic Weisbecker <fweisbec@...il.com>
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 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.

> +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();
  }

> +       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?

> +} 
--
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/