netdev - Re: [PATCH] WireGuard: restrict packet handling to non-isolated CPUs.

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] [day] [month] [year] [list]

Message-ID: <YmNRdLy1U2N9JN2n@zx2c4.com>
Date:   Sat, 23 Apr 2022 03:08:04 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Charles-François Natali <cf.natali@...il.com>
Cc:     wireguard@...ts.zx2c4.com, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [PATCH] WireGuard: restrict packet handling to non-isolated CPUs.

Hi Charles,

On Fri, Apr 22, 2022 at 11:23:01PM +0100, Charles-François Natali wrote:
> > Regarding your patch, is there a way to make that a bit more succinct,
> > without introducing all of those helper functions? It seems awfully
> > verbose for something that seems like a matter of replacing the online
> > mask with the housekeeping mask.
> 
> Indeed, I wasn't really happy about that.
> The reason I've written those helper functions is that the housekeeping mask
> includes possible CPUs (cpu_possible_mask), so unfortunately it's not just a
> matter of e.g. replacing cpu_online_mask with
> housekeeping_cpumask(HK_FLAG_DOMAIN), we have to perform an AND
> whenever we compute the weight, find the next CPU in the mask etc.
> 
> And I'd rather have the operations and mask in a single location instead of
> scattered throughout the code, to make it easier to understand and maintain.
> 
> Happy to change to something more inline though, or open to suggestions.

Probably more inlined, yea. A simpler version of your patch would
probably be something like this, right?

diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 583adb37ee1e..b3117cdd647d 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -112,6 +112,8 @@ static inline int wg_cpumask_choose_online(int *stored_cpu, unsigned int id)
 		cpu = cpumask_first(cpu_online_mask);
 		for (i = 0; i < cpu_index; ++i)
 			cpu = cpumask_next(cpu, cpu_online_mask);
+		while (!housekeeping_test_cpu(cpu, HK_???))
+			cpu = cpumask_next(cpu, cpu_online_mask);
 		*stored_cpu = cpu;
 	}
 	return cpu;
@@ -128,7 +130,7 @@ static inline int wg_cpumask_next_online(int *next)
 {
 	int cpu = *next;

-	while (unlikely(!cpumask_test_cpu(cpu, cpu_online_mask)))
+	while (unlikely(!cpumask_test_cpu(cpu, cpu_online_mask) && !housekeeping_test_cpu(cpu, HK_???)))
 		cpu = cpumask_next(cpu, cpu_online_mask) % nr_cpumask_bits;
 	*next = cpumask_next(cpu, cpu_online_mask) % nr_cpumask_bits;
 	return cpu;

However, from looking at kernel/sched/isolation.c a bit, I noticed that
indeed you're right that most of these functions (save one) are based on
cpu_possible_mask rather than cpu_online_mask. This is frustrating
because the code makes smart use of static branches to remain quick, but
ANDing housekeeping_cpumask() with cpu_online_mask would, in the fast
path, wind up ANDing cpu_online_mask with cpu_possible_mask, which is
silly and pointless. That makes me suspect that maybe the best approach
would be adding a relevant helper to kernel/sched/isolation.c, so that
the helper can then do the `if (static_branch_unlikely(&housekeeping_overridden))`
stuff internally.

Or maybe you'll do some measurements and decide that just [ab]using
housekeeping_test_cpu() like above is actually optimal? Not really sure
myself.

Anyway, I'll keep an eye out for your joint wireguard/padata series. Be
sure to CC the people who wrote the isolation & housekeeping code, as
they likely have opinions about this stuff (and certainly know more than
me about it).

Jason