Re: Random arrays on kernel stack..

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hey Linus,

On Thu, Jul 28, 2022 at 04:51:58PM -0700, Linus Torvalds wrote:
> So I finally have an arm64 laptop that I'm playing with, and as a

Some intrepid diving into Asahi world, eh? I admit that after playing
with the M1 and benching/optimizing some code on it, I wasn't man enough
to daily drive the thing, and I wound up selling the (already
second hand) Mac Mini shortly after acquiring it.

> result building the kernel the way I usually do - with warnings as
> errors.
> 
> And I get this:
> 
>   drivers/net/wireguard/allowedips.c: In function ‘root_remove_peer_lists’:
>   drivers/net/wireguard/allowedips.c:77:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>      77 | }
>         | ^
>   drivers/net/wireguard/allowedips.c: In function ‘root_free_rcu’:
>   drivers/net/wireguard/allowedips.c:64:1: error: the frame size of
> 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>      64 | }
>         | ^
> 
> and clearly it only happens for me because it turns out Asahi has for
> some odd reason picked that low CONFIG_FRAME_WARN of just 1kB.

Arnd (now CC'd) and I had a discussion about this a few years ago, and
IIRC the conclusion was that the kernel targets 1280 bytes on 64-bit
platforms and 1024 bytes on 32-bit platforms. There's been gradual work
done over the years to get gcc to generate code that matched this
expectation.

So root_remove_peer_lists and root_free_rcu are really nestling up
against the boundary. We'd looked into ways of avoid that, but decided
in the end it was fine, and probably the most okay way to implement
those algorithms, all things considered.

> But when I look at the code that generates that warning, it just
> worries me. It has that magical constant 128.
> 
> Sure, that same constant is also in push_rcu().

I could (should?) put it in a constant. But as context, note that it
doesn't look _so_ magical for anyone hacking on the code. It's a trie
for IPv4 and IPv6 addresses. If you're writing networking code, the
128-bitness of IPv6 addresses is nearly always at the top of your brain,
its general inconvenience an elephant not possible to ignore.

I guess `sizeof(struct in6_addr) * 8` would be a more accurate
reference.

>  (b) it should be documented some way as a #define

Alright, will do.

> And there it is randomly as a warning, and then it will happily
> overflow the stack frame.
> 
> That's not ok.
>
>  (c) push_rcu() should damn well not "warn and corrupt the stack". It
> should warn-and-not-corrupt the stack, even if that then means that
> the thing isn't pushed at all.

That's a good point. Though note that the current WARN_ON is also
dependent on DEBUG being set. The purpose is to catch errors when the
selftest in drivers/net/wireguard/selftest/allowedips.c runs. That file
has this snippet:

  /* These will hit the WARN_ON(len >= 128) in free_node if something
   * goes wrong.
   */
  for (i = 0; i < 128; ++i) {
    part = cpu_to_be64(~(1LLU << (i % 64)));
    memset(&ip, 0xff, 16);
    memcpy((u8 *)&ip + (i < 64) * 8, &part, 8);
    wg_allowedips_insert_v6(&t, &ip, 128, a, &mutex);
  }
  wg_allowedips_free(&t, &mutex);

Anyway, I'll conditionalize it so the stack isn't corrupted.

>  (a) that constant should be a bit lower, so that we *can* use a 1kB
> stack frame warning on 64-bit architectures

That's not so much possible. This needs to do a DFS traversal of the
trie, which means it can be 128 nodes deep since IPv6 addresses have
128 bits. I could just malloc, instead, but malloc'ing in order to free
something is kind of annoying. And it *does* fit after all. I could
suppress that option for just that file, but that might hide other bugs.
And there's the 1280 target I mentioned earlier, so maybe this is all
fine? Unless you have a different suggestion?

I think, anyhow, this is roughly where Arnd and I left off a few years
ago.

Jason