Re: [PATCH net-next] airo: work around stack usage warning
From: Tom Rix <trix@redhat.com>
Date: 2021-03-23 15:04:31
Also in:
lkml, netdev
On 3/23/21 6:16 AM, Arnd Bergmann wrote:
quoted hunk ↗ jump to hunk
From: Arnd Bergmann <arnd@arndb.de> gcc-11 with KASAN on 32-bit arm produces a warning about a function that needs a lot of stack space: drivers/net/wireless/cisco/airo.c: In function 'setup_card.constprop': drivers/net/wireless/cisco/airo.c:3960:1: error: the frame size of 1512 bytes is larger than 1400 bytes [-Werror=frame-larger-than=] Most of this is from a single large structure that could be dynamically allocated or moved into the per-device structure. However, as the callers all seem to have a fairly well bounded call chain, the easiest change is to pull out the part of the function that needs the large variables into a separate function and mark that as noinline_for_stack. This does not reduce the total stack usage, but it gets rid of the warning and requires minimal changes otherwise. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/wireless/cisco/airo.c | 117 +++++++++++++++++------------- 1 file changed, 65 insertions(+), 52 deletions(-)diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c index e35e1380ae43..540ba694899c 100644 --- a/drivers/net/wireless/cisco/airo.c +++ b/drivers/net/wireless/cisco/airo.c@@ -3818,6 +3818,68 @@ static inline void set_auth_type(struct airo_info *local, int auth_type) local->last_auth = auth_type; } +static int noinline_for_stack airo_readconfig(struct airo_info *ai, u8 *mac, int lock) +{ + int i, status; + /* large variables, so don't inline this function, + * maybe change to kmalloc + */ + tdsRssiRid rssi_rid; + CapabilityRid cap_rid; + + kfree(ai->SSID); + ai->SSID = NULL; + // general configuration (read/modify/write) + status = readConfigRid(ai, lock); + if (status != SUCCESS) return ERROR; + + status = readCapabilityRid(ai, &cap_rid, lock); + if (status != SUCCESS) return ERROR; + + status = PC4500_readrid(ai, RID_RSSI, &rssi_rid, sizeof(rssi_rid), lock); + if (status == SUCCESS) { + if (ai->rssi || (ai->rssi = kmalloc(512, GFP_KERNEL)) != NULL) + memcpy(ai->rssi, (u8*)&rssi_rid + 2, 512); /* Skip RID length member */ + } + else { + kfree(ai->rssi); + ai->rssi = NULL; + if (cap_rid.softCap & cpu_to_le16(8)) + ai->config.rmode |= RXMODE_NORMALIZED_RSSI; + else + airo_print_warn(ai->dev->name, "unknown received signal " + "level scale"); + } + ai->config.opmode = adhoc ? MODE_STA_IBSS : MODE_STA_ESS; + set_auth_type(ai, AUTH_OPEN); + ai->config.modulation = MOD_CCK; + + if (le16_to_cpu(cap_rid.len) >= sizeof(cap_rid) && + (cap_rid.extSoftCap & cpu_to_le16(1)) && + micsetup(ai) == SUCCESS) { + ai->config.opmode |= MODE_MIC; + set_bit(FLAG_MIC_CAPABLE, &ai->flags); + } + + /* Save off the MAC */ + for (i = 0; i < ETH_ALEN; i++) { + mac[i] = ai->config.macAddr[i]; + } + + /* Check to see if there are any insmod configured + rates to add */ + if (rates[0]) { + memset(ai->config.rates, 0, sizeof(ai->config.rates)); + for (i = 0; i < 8 && rates[i]; i++) { + ai->config.rates[i] = rates[i]; + } + } + set_bit (FLAG_COMMIT, &ai->flags); + + return SUCCESS; +} + + static u16 setup_card(struct airo_info *ai, u8 *mac, int lock) { Cmd cmd;@@ -3864,58 +3926,9 @@ static u16 setup_card(struct airo_info *ai, u8 *mac, int lock) if (lock) up(&ai->sem); if (ai->config.len == 0) { - int i; - tdsRssiRid rssi_rid;
This is mostly an array, and what I guess is the problem.
- CapabilityRid cap_rid; - - kfree(ai->SSID); - ai->SSID = NULL; - // general configuration (read/modify/write) - status = readConfigRid(ai, lock); - if (status != SUCCESS) return ERROR; - - status = readCapabilityRid(ai, &cap_rid, lock); - if (status != SUCCESS) return ERROR; - - status = PC4500_readrid(ai, RID_RSSI,&rssi_rid, sizeof(rssi_rid), lock);
This is reading into a stack temp
- if (status == SUCCESS) {
- if (ai->rssi || (ai->rssi = kmalloc(512, GFP_KERNEL)) != NULL)
- memcpy(ai->rssi, (u8*)&rssi_rid + 2, 512); /* Skip RID length member */Here is the temp being copied to heap memory. This is the only use of rssi_rid Why not do the alloc in PC4500_read ? The call would be something like status = PC4500_readrid(ai, RID_RSSI, &ai->rssi, sizeof(), lock) or since ai, is common reduce the signature of the function and make the call PC4500_readid(ai, RID_RSSI, lock) Tom
- }
- else {
- kfree(ai->rssi);
- ai->rssi = NULL;
- if (cap_rid.softCap & cpu_to_le16(8))
- ai->config.rmode |= RXMODE_NORMALIZED_RSSI;
- else
- airo_print_warn(ai->dev->name, "unknown received signal "
- "level scale");
- }
- ai->config.opmode = adhoc ? MODE_STA_IBSS : MODE_STA_ESS;
- set_auth_type(ai, AUTH_OPEN);
- ai->config.modulation = MOD_CCK;
-
- if (le16_to_cpu(cap_rid.len) >= sizeof(cap_rid) &&
- (cap_rid.extSoftCap & cpu_to_le16(1)) &&
- micsetup(ai) == SUCCESS) {
- ai->config.opmode |= MODE_MIC;
- set_bit(FLAG_MIC_CAPABLE, &ai->flags);
- }
-
- /* Save off the MAC */
- for (i = 0; i < ETH_ALEN; i++) {
- mac[i] = ai->config.macAddr[i];
- }
-
- /* Check to see if there are any insmod configured
- rates to add */
- if (rates[0]) {
- memset(ai->config.rates, 0, sizeof(ai->config.rates));
- for (i = 0; i < 8 && rates[i]; i++) {
- ai->config.rates[i] = rates[i];
- }
- }
- set_bit (FLAG_COMMIT, &ai->flags);
+ status = airo_readconfig(ai, mac, lock);
+ if (status != SUCCESS)
+ return ERROR;
}
/* Setup the SSIDs if present */