[Bugme-new] [Bug 32942] New: (ath5k) Not all elements of chinfo[pier].pd_curves[] are freed

bugzilla-daemon at bugzilla.kernel.org bugzilla-daemon at bugzilla.kernel.org
Sat Apr 9 05:10:44 PDT 2011


https://bugzilla.kernel.org/show_bug.cgi?id=32942

           Summary: (ath5k) Not all elements of chinfo[pier].pd_curves[]
                    are freed
           Product: Drivers
           Version: 2.5
    Kernel Version: 2.6.39-rc2
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: network-wireless
        AssignedTo: drivers_network-wireless at kernel-bugs.osdl.org
        ReportedBy: dame_eugene at mail.ru
        Regression: No


Some of the memory blocks kcalloc'ed in
ath5k_eeprom_convert_pcal_info_*() are not freed even when module ath5k is
unloaded.

To reproduce the problem, it is enough to load ath5k.ko module and then unload
it again.

Here is the trace for one iteration of the outer 'for' loop with my comments.
ath5k_eeprom_convert_pcal_info_2413(), 
drivers/net/wireless/ath/ath5k/eeprom.c:1148:

// Allocation of chinfo[pier].pd_curves, line 1153. Room for 4 elements of 
// type struct ath5k_pdgain_info is created.
  called___kmalloc: (.text+0x212f) arguments: (48, 80d0), result: ab3ad780

// The inner loop, iter #0, allocation of pd->pd_step and pd->pd_pwr  
// for chinfo[pier].pd_curves[pdgain_idx[0]] (lines 1176 and 1182)
  called___kmalloc: (.text+0x212f) arguments: (4, 80d0), result: ab1c3f68
  called___kmalloc: (.text+0x212f) arguments: (8, 80d0), result: ab1c3f70

// The inner loop, iter #1, allocation of pd->pd_step and pd->pd_pwr  
// for chinfo[pier].pd_curves[pdgain_idx[1]] (lines 1176 and 1182)
  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(),
drivers/net/wireless/ath/ath5k/eeprom.c:1564

// The inner loop, iter #0, line 1573
  called_kfree: (.text+0x20be) arguments: ((null))
  called_kfree: (.text+0x20c6) arguments: ((null))

// The inner loop, iter #1, line 1573
  called_kfree: (.text+0x20be) arguments: (ab1c3f78)
  called_kfree: (.text+0x20c6) arguments: (ab17ce80)

// Deallocation of chinfo[pier].pd_curves array
  called_kfree: (.text+0x20de) arguments: (ab3ad780)
<...> 

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

It looks like ath5k_eeprom_free_pcal_info() tries to free 'pd_step' and
'pd_pwr' fields of wrong elements. 

On my system, chinfo[pier].pd_curves[] is an array of 4
(AR5K_EEPROM_N_PD_CURVES) elements, only 2 of which are actually used, #1 and
#2, which is probably OK. The elements #0 and #3 remain filled with zeros. 

Consider line 1162:
    for (pdg = 0; pdg < ee->ee_pd_gains[mode]; pdg++) {
        u8 idx = pdgain_idx[pdg];
        struct ath5k_pdgain_info *pd =
                &chinfo[pier].pd_curves[idx];

ee->ee_pd_gains[mode] is 2 on my system and pdgain_idx[] array is {1; 2}. From
the code in eeprom.c, it looks like only these elements of
chinfo[pier].pd_curves are actually used for calibration, etc.

Only ath5k_eeprom_free_pcal_info() works in a different way. It tries to free
'pd_step' and 'pd_pwr' for the first two elements of chinfo[pier].pd_curves
instead of the elements that were actually allocated before: 

    for (pdg = 0; pdg < ee->ee_pd_gains[mode]; pdg++) {
        struct ath5k_pdgain_info *pd =
                &chinfo[pier].pd_curves[pdg];

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

That is, it processes pd_curves[0] and pd_curves[1] rather than 
pd_curves[pdgain_idx[0]] and pd_curves[pdgain_idx[1]].

There are probably two ways to fix this issue: we can just process all 4
elements of pd_curves[] or we can process only the elements actually used (like
it is done in other places in eeprom.c).

Here is the patch for the first variant (I would also remove the inner
conditional as 'pd' is never NULL anyway):

eeprom.c:1568:
-    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);
-        }
    }

Here is the patch for the second variant:
eeprom.c:1541:
    struct ath5k_chan_pcal_info *chinfo;
+    u8 *pdgain_idx = ee->ee_pdc_to_idx[mode];
    u8 pier, pdg;

eeprom.c:1568:

    for (pdg = 0; pdg < ee->ee_pd_gains[mode]; pdg++) {
-        struct ath5k_pdgain_info *pd =
-                &chinfo[pier].pd_curves[pdg];
+        u8 idx = pdgain_idx[pdg];
+        struct ath5k_pdgain_info *pd =
+                &chinfo[pier].pd_curves[idx];

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

I guess the result is the same in both these variants.

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.


More information about the Bugme-new mailing list