netdev - Re: pull-request: mac80211-next 2019-07-31

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: <CAK8P3a2BiDYXR-x_RyAOLZL_dL6m49JMy13U5VQ_gp9MbLfGGA@mail.gmail.com>
Date:   Mon, 28 Oct 2019 11:53:55 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        John Crispin <john@...ozen.org>
Subject: Re: pull-request: mac80211-next 2019-07-31

On Mon, Oct 28, 2019 at 10:52 AM Johannes Berg
<johannes@...solutions.net> wrote:
>
> > It looks like one of the last additions pushed the stack usage over
> > the 1024 byte limit
> > for 32-bit architectures:
> >
> > net/mac80211/mlme.c:4063:6: error: stack frame size of 1032 bytes in
> > function 'ieee80211_sta_rx_queued_mgmt' [-Werror,-Wframe-larger-than=]
> >
> > struct ieee802_11_elems is fairly large, and just grew another two pointers.
> > When ieee80211_rx_mgmt_assoc_resp() and ieee80211_assoc_success()
> > are inlined into ieee80211_sta_rx_queued_mgmt(), there are three copies
> > of this structure, which is slightly too much.
>
> Hmm. I guess that means the compiler isn't smart enough to make the
> copies from the inlined sub-functions alias each other? I mean, if I
> have
>
> fn1(...) { struct ... elems1; ... }
> fn2(...) { struct ... elems2; ... }
>
> fn(...)
> {
>   fn1();
>   fn2();
> }
>
> then it could reasonably use the same stack memory for elems1 and
> elems2, at least theoretically, but you're saying it doesn't do that I
> guess?

No, that's not the problem here (it can happen if the compiler is
unable to prove
the object lifetimes are non-overlapping).

What we have here are multiple functions that are in the same call chain:

fn1()
{
     struct ieee802_11_elems e ;
}

fn2()
{
   struct ieee802_11_elems e;
  ...
   fn1();
}

fn3()
{
   struct ieee802_11_elems e;
  ...
   fn2();
}

Here, the object lifetimes actually do overlap, so the compiler cannot easily
optimize that away.

> I don't think dynamic allocation would be nice - but we could manually
> do this by passing the elems pointer into the
> ieee80211_rx_mgmt_assoc_resp() and ieee80211_assoc_success() functions.

Ah, so you mean we can reuse the objects manually? I think that would
be great. I could not tell if that's possible when reading the source, but
if you can show that this works, that would be an easy solution.

> Why do you say 32-bit btw, it should be *bigger* on 64-bit, but I didn't
> see this ... hmm.

That is correct. For historic reasons, both the total amount of stack space
per thread and the warning limit on 64 bit are twice the amount that we
have on 32-bit kernels, so even though the problem is more serious on
64-bit architectures, we do not see a warning about it because we remain
well under the warning limit.

      Arnd