Bug 32722

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.

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.

Created attachment 53832 [details]
0001-ath5k-improve-pcal-error-handling-for-ENOMEM-case.patch

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?

I agree, it is a separate issue. I have filed a bug report for it:
https://bugzilla.kernel.org/show_bug.cgi?id=32942

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>