Bug 32722 - Memory kcalloc'ed in ath5k_eeprom_convert_pcal_info_*() is not always kfree'd
Summary: Memory kcalloc'ed in ath5k_eeprom_convert_pcal_info_*() is not always kfree'd
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: network-wireless (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: John W. Linville
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-05 14:34 UTC by Eugene A. Shatokhin
Modified: 2011-04-27 19:14 UTC (History)
3 users (show)

See Also:
Kernel Version: 2.6.39-rc1
Tree: Mainline
Regression: No


Attachments
0001-ath5k-improve-pcal-error-handling-for-ENOMEM-case.patch (6.34 KB, patch)
2011-04-08 19:34 UTC, John W. Linville
Details | Diff

Description Eugene A. Shatokhin 2011-04-05 14:34:29 UTC
This problem concerns 'ath5k' driver. 

When ath5k_hw_init() performs necessary initialization tasks, ath5k_eeprom_init() is called. One of ath5k_eeprom_convert_pcal_info_[5111|5112|2413]() functions is called then among other things. On my system, it is ath5k_eeprom_convert_pcal_info_2413() but everything also applies to the other two functions.

(drivers/net/wireless/ath/ath5k/eeprom.c) 
ath5k_eeprom_convert_pcal_info_2413() allocates several memory blocks in 'for' loops with kcalloc (see the lines 1153, 1176, 1182). It seems unlikely that any of these allocations will fail, the sizes of the memory blocks are rather small. Nevertheless, if one of these allocations fails, the memory blocks allocated in ath5k_eeprom_convert_pcal_info_2413() before will probably never be freed.

They could be freed by ath5k_eeprom_free_pcal_info() but this function is not called in this case. 

Not a critical problem, it seems, but still.
Comment 1 Eugene A. Shatokhin 2011-04-06 14:38:16 UTC
There is probably one more problem with these memory allocations. Even if no memory allocations fail, some of the memory blocks kcalloc'ed in ath5k_eeprom_convert_pcal_info_*() are not freed even when module ath5k is unloaded.

Here is the trace for one iteration of the outer 'for' loop, ath5k_eeprom_convert_pcal_info_2413():

called___kmalloc: (.text+0x212f) arguments: (48, 80d0), result: ab3ad780
called___kmalloc: (.text+0x212f) arguments: (4, 80d0), result: ab1c3f68
called___kmalloc: (.text+0x212f) arguments: (8, 80d0), result: ab1c3f70
called___kmalloc: (.text+0x212f) arguments: (5, 80d0), result: ab1c3f78
called___kmalloc: (.text+0x212f) arguments: (10, 80d0), result: ab17ce80
<...> 

Here is the trace for one iteration of the outer 'for' loop, ath5k_eeprom_free_pcal_info():

called_kfree: (.text+0x20be) arguments: ((null))
called_kfree: (.text+0x20c6) arguments: ((null))
called_kfree: (.text+0x20be) arguments: (ab1c3f78)
called_kfree: (.text+0x20c6) arguments: (ab17ce80)
called_kfree: (.text+0x20de) arguments: (ab3ad780)
<...> 

The blocks of size 4 and 8 bytes 
(pointed to by chinfo[pier].pd_curves[0].pd_step and 
chinfo[pier].pd_curves[0].pd_pwr, respectively) were not freed. 

Same for other iterations of the outer 'for' loops in ath5k_eeprom_convert_pcal_info_*() and ath5k_eeprom_free_pcal_info().

It seems, chinfo[pier].pd_curves[0] was filled with zeros somehow. I have not yet figured out how this could happen.

The memory leaks were detected and the trace was saved by the tools from KEDR framework (http://kedr.berlios.de/).

For now, I have checked this on the kernel 2.6.37, haven't tried 2.6.39-rcN for this part of the problem yet.
Comment 2 Eugene A. Shatokhin 2011-04-08 12:41:29 UTC
I spent some time digging into this and probably found what had caused these memory leaks (only in case when no allocation failures occur). 

Consider the loops in ath5k_eeprom_convert_pcal_info_2413(). 
Array chinfo[pier].pd_curves[] has space for 4 (AR5K_EEPROM_N_PD_CURVES) elements, each of which is initially filled with zeros. Only 2 of these elements seem to be needed on my system (ee->ee_pd_gains[mode] is 2), the elements #1 and #2. The elements #0 and #3 are left untouched.

But in ath5k_eeprom_free_pcal_info(), ee->ee_pd_gains[mode] first elements are freed not matter for which ones memory was actually allocated before. That is, kfree() is called for the components of the elements #0 and #1 rather than #1 and #2, hence the behaviour I observed.

I suppose the following simple patch for ath5k_eeprom_free_pcal_info() would fix this part of the problem (I would also remove the inner conditional as 'pd' is never NULL anyway):

drivers/net/wireless/ath/ath5k/eeprom.c:
-	for (pdg = 0; pdg < ee->ee_pd_gains[mode]; pdg++) {
+	for (pdg = 0; pdg < AR5K_EEPROM_N_PD_CURVES; pdg++) {
		struct ath5k_pdgain_info *pd =
				&chinfo[pier].pd_curves[pdg];

-		if (pd != NULL) {
			kfree(pd->pd_step);
			kfree(pd->pd_pwr);
-		}
	}

This does not affect the problem with memory leaks when kcalloc fails, of course.
Comment 3 John W. Linville 2011-04-08 19:34:28 UTC
Created attachment 53832 [details]
0001-ath5k-improve-pcal-error-handling-for-ENOMEM-case.patch
Comment 4 John W. Linville 2011-04-08 19:36:43 UTC
I'm not so sure about what you report in comment 2 and comment 3.  In particular, those pd_step allocation appear to be in loops that check ee->ee_pd_gains[mode] anyway.  So while checking AR5K_EEPROM_N_PD_CURVES seems fine as an alternative, I wonder if we will just be hiding a problem?

Anyway, I think it is a separate issue.  So, maybe you should open a separate bug for what you report in comment 2 and comment 3?
Comment 5 Eugene A. Shatokhin 2011-04-09 12:13:14 UTC
I agree, it is a separate issue. I have filed a bug report for it:
https://bugzilla.kernel.org/show_bug.cgi?id=32942
Comment 6 John W. Linville 2011-04-27 19:13:16 UTC
Queued for 2.6.40...

commit a065784620a2b78a2bbd00e066c004644d227ea8
Author: John W. Linville <linville@tuxdriver.com>
Date:   Fri Apr 8 15:33:12 2011 -0400

    ath5k: improve pcal error handling for ENOMEM case
    
    The ath5k driver does kmalloc allocations for pcal info in a loop.
    But, if one fails it was simply returning -ENOMEM without freeing
    already allocated memory.  This patch corrects that oversight.
    
    Reported-by: Eugene A. Shatokhin <dame_eugene@mail.ru>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>
    Reviewed-by: Bob Copeland <me@bobcopeland.com>

Note You need to log in before you can comment on or make changes to this bug.