linux-kernel - Re: [RFC][PATCH v2 09/11] context_tracking,livepatch: Dont disturb NOHZ

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:   Wed, 6 Oct 2021 13:48:01 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Petr Mladek <pmladek@...e.com>
cc:     Peter Zijlstra <peterz@...radead.org>, gor@...ux.ibm.com,
        jpoimboe@...hat.com, jikos@...nel.org, mingo@...nel.org,
        linux-kernel@...r.kernel.org, joe.lawrence@...hat.com,
        fweisbec@...il.com, tglx@...utronix.de, hca@...ux.ibm.com,
        svens@...ux.ibm.com, sumanthk@...ux.ibm.com,
        live-patching@...r.kernel.org, paulmck@...nel.org,
        rostedt@...dmis.org, x86@...nel.org
Subject: Re: [RFC][PATCH v2 09/11] context_tracking,livepatch: Dont disturb
 NOHZ_FULL

> > That's not sufficient, you need to tag the remote task with a ct_work
> > item to also runs klp_update_patch_state(), otherwise the remote CPU can
> > enter kernel space between checking is_ct_user() and doing
> > klp_update_patch_state():
> > 
> > 	CPU0				CPU1
> > 
> > 					<user>
> > 
> > 	if (is_ct_user()) // true
> > 					<kernel-entry>
> > 					  // run some kernel code
> > 	  klp_update_patch_state()
> > 	  *WHOOPSIE*
> > 
> > 
> > So it needs to be something like:
> > 
> > 
> > 	CPU0				CPU1
> > 
> > 					<user>
> > 
> > 	if (context_tracking_set_cpu_work(task_cpu(), CT_WORK_KLP))
> > 
> > 					<kernel-entry>
> > 	  klp_update_patch_state	  klp_update_patch_state()
> > 
> > 
> > So that CPU0 and CPU1 race to complete klp_update_patch_state() *before*
> > any regular (!noinstr) code gets run.
> 
> Grr, you are right. I thought that we migrated the task when entering
> kernel even before. But it seems that we do it only when leaving
> the kernel in exit_to_user_mode_loop().

That is correct. You are probably confused by the old kGraft 
implementation which added the TIF also to _TIF_WORK_SYSCALL_ENTRY so 
that syscall_trace_enter() processed it too. But upstream solution has 
always switched tasks only on their exit to user mode, because it was 
deemed sufficient.

Btw, just for fun, the old kGraft in one of its first incarnations also 
had the following horrible hack to exactly "solve" the problem.

+/*
+ * Tasks which are running in userspace after the patching has been started
+ * can immediately be marked as migrated to the new universe.
+ *
+ * If this function returns non-zero (i.e. also when error happens), the task
+ * needs to be migrated using kgraft lazy mechanism.
+ */
+static inline bool kgr_needs_lazy_migration(struct task_struct *p)
+{
+       unsigned long s[3];
+       struct stack_trace t = {
+               .nr_entries = 0,
+               .skip = 0,
+               .max_entries = 3,
+               .entries = s,
+       };
+
+       save_stack_trace_tsk(p, &t);
+
+       return t.nr_entries > 2;
+}

Miroslav