New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 781315 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ar10k driver puts a bit too much on the stack

Project Member Reported by diand...@chromium.org, Nov 3 2017

Issue description

When poking around w/ kasan, I found that kasan was tripping the "stack too big" warning in the kernel in a function in the ark10k driver.  Specifically the build yelled about:

ieee80211_sta_rx_queued_mgmt

...in "drivers/net/wireless/ar10k/mac80211/mlme.c"

===

Something about kasan (specifically with the clang compiler) seems to be making things worse and I'll file a bug about that...

...but the net-net is that it's probably not super wise that functions in that file stick structures like this on the stack:

  struct ieee802_11_elems elems;

That structure is 312 bytes big (on arm64) according to gdb.  312 is not _tons_, but generally that seems like it's getting to be the size that maybe should be allocated elsewhere...


...this is a bit of a judgement call, but it does seem like maybe we should fix it?
 
Details about the kasan / clang issue can be found in  bug #781317 
For reference, the code seems to be copy-pasted from net/mac80211/mlme.c:ieee80211_sta_rx_queued_mgmt, which (along with many other functions in net/mac80211) also put that structure on the stack.
Cc: mka@chromium.org
@2: Ah, interesting.  We could certainly just WontFix this bug and call it a clang issue that it's inlining drops so much crud on the stack at once...
Components: OS>Kernel
@2 (drinkcat): Yes, the approach with several wifi drivers (including ar10k and iwl7000) is to maintain a copy of their respective upstream drivers (ath10k and iwlwifi) along with the mac80211 stack. The idea is to keep a single "QCA" and a single "Intel" driver package across multiple kernels, since (allegedly) mac80211 can be a big source of regressions, and this makes testing a little more straightforward.

So, any issue like that is definitely an upstream one.

@3 (dianders): were you referring to comment #1 or #2?
@4: My suggestion was to remove some of these large structures from the stack since they seemed a bit on the big side.  ...but how much is "too much" to put on the stack is a gray area.  If the main mac80211 stack upstream puts these things on the stack then I have a feeling we won't change their mind.

---

...the reason that we care, though, is because clang seemed to do something different that gcc and seems to be using up more stack space.  I haven't dug into the assembly enough, but my first guess was this was happening:

Imagine that we have:

---

static void subfunc1(void)
{
   char buf1[256];
   sprintf(buf1, "%s");
}
static void subfunc2(void)
{
   char buf2[256];
   sprintf(buf2, "%s");
}
void mainfunc(void)
{
   char buf0[256];
   sprintf(buf0, "%s");

   subfunc1();
   subfunc2();
}

---

If we do a bad job of inlining then the above code might be transformed into something equivalent to this:

void mainfunc(void)
{
   char buf0[256];
   char buf1[256];
   char buf2[256];
   sprintf(buf0, "%s");
   sprintf(buf1, "%s");
   sprintf(buf2, "%s");
}

...that would eat up 768 bytes of stack space as opposed to 512 bytes in the original case.

---

In any case, if we decide that we're not going to attempt to take the structures off the stack, presumably we should just WontFix this and dig into the clang issues in  bug #781317 

Comment 6 by mka@chromium.org, Nov 6 2017

Cc: manojgupta@chromium.org
This discussion is related, though not clang specific: 
https://patchwork.kernel.org/patch/9573049/

Apparently increase in stack size in combination with kasan are also an issue with gcc 7.0. Doesn't look like any of the proposed changes landed upstream.
@6: for whatever reason, though, gcc doesn't blow up the stack nearly as much.  In  bug #781317  I measured that "kasan + gcc" = ~500 bytes and "no kasan + clang" > 1000 bytes.  So clang _without_ kasan is actually worse than gcc _with_ kasan.
Status: WontFix (was: Untriaged)
OK, closing this one.  Feel free to subscribe to  bug #781317  if you're interested.  If someone would like me to post a "clang seems to grow the stack more than gcc" bug I can.  Let me know if that would be useful.

Sign in to add a comment