On Tue, 2021-09-07 at 16:14 -0700, Linus Torvalds wrote:
The mac802.11 one seems to be due to 'struct ieee802_11_elems' being big, and allocated on the stack. I think it's probably made worse there with inlining, ie
- ieee80211_sta_rx_queued_mgmt() has one copy
- ieee80211_rx_mgmt_beacon() is possibly inlined, and has its own copy
but even if it isn't due to that kind of duplication due to inlining, that code is dangerous. Exactly because it has two nested stack frames with that big structure, and they are active at the same time in the callchain whether inlined or not.
And it's *pointlessly* dangerous, because the 'struct ieee802_11_elems elems' in ieee80211_sta_rx_queued_mgmt() is only used for the IEEE80211_STYPE_ACTION case, so it is entirely disjoint from the IEEE80211_STYPE_BEACON case, and those stack allocations simply should not nest like that in the first place.
Making the IEEE80211_STYPE_ACTION case be its own function - like the other cases - and moving the struct there should fix it. Possibly a "noinline" or two necessary to make sure that the compiler doesn't then undo the "these two cases are disjoint" thing.
Yeah, I'm aware, and I agree. We've been looking at it every now and then. This got made worse by us actually adding a fair amount of pointers to the struct recently (in this merge window).
Ultimately, every new spec addition ends up needing to add something there, so I think ultimately we'll probably want to either dynamically allocate it somewhere (perhaps in a data structure used here already), or possibly not have this at all and just find a way to return only the bits that are interesting. Even parsing a ~1k frame (typical, max ~2k) a handful of times is probably not even worse than having this large a structure that gets filled data that's probably useless in many cases (I think the different cases all just need a subset). But not sure, I'll take a look.
johannes