From: Shu Wang <[email protected]>
Kmemleak reports about a thousand false positives for fusion->
cmd_list[]. Root casue is the cmd_list objects are allocated from
slab allocator, and stored its pointer in object allocated by page
allocator. The fix will tell kmemleak to track and scan fusion
object.
V2:
Add comment, balance braces, move kmemleak_free before free_pages.
checkpatch passed.
Before patch:
kmemleak: 1004 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
unreferenced object 0xffff88042584e000 (size 8192):
backtrace:
kmemleak_alloc+0x4a/0xa0
__kmalloc+0xec/0x220
megasas_alloc_cmdlist_fusion+0x3e/0x140 [megaraid_sas]
megasas_alloc_cmds_fusion+0x44/0x450 [megaraid_sas]
megasas_init_adapter_fusion+0x21d/0x6e0 [megaraid_sas]
megasas_init_fw+0x357/0xd30 [megaraid_sas]
megasas_probe_one.part.34+0x5be/0x1040 [megaraid_sas]
megasas_probe_one+0x46/0xc0 [megaraid_sas]
local_pci_probe+0x45/0xa0
work_for_cpu_fn+0x14/0x20
process_one_work+0x149/0x360
worker_thread+0x1d8/0x3c0
kthread+0x109/0x140
ret_from_fork+0x25/0x30
Signed-off-by: Shu Wang <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas_fusion.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 11bd2e698b84..161b8e5518c0 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -48,6 +48,7 @@
#include <linux/mutex.h>
#include <linux/poll.h>
#include <linux/vmalloc.h>
+#include <linux/kmemleak.h>
#include <scsi/scsi.h>
#include <scsi/scsi_cmnd.h>
@@ -4512,6 +4513,13 @@ megasas_alloc_fusion_context(struct megasas_instance *instance)
dev_err(&instance->pdev->dev, "Failed from %s %d\n", __func__, __LINE__);
return -ENOMEM;
}
+ } else {
+ /*
+ * Allow kmemleak to scan these pages as they contain pointers
+ * to additional allocations. fusion->cmd_list[].
+ */
+ kmemleak_alloc(instance->ctrl_context,
+ sizeof(struct fusion_context), 1, GFP_KERNEL);
}
fusion = instance->ctrl_context;
@@ -4548,9 +4556,11 @@ megasas_free_fusion_context(struct megasas_instance *instance)
if (is_vmalloc_addr(fusion))
vfree(fusion);
- else
+ else {
+ kmemleak_free(fusion);
free_pages((ulong)fusion,
instance->ctrl_context_pages);
+ }
}
}
--
2.13.5
On Fri, Sep 15, 2017 at 01:21:52PM +0800, [email protected] wrote:
> From: Shu Wang <[email protected]>
>
> Kmemleak reports about a thousand false positives for fusion->
> cmd_list[]. Root casue is the cmd_list objects are allocated from
> slab allocator, and stored its pointer in object allocated by page
> allocator. The fix will tell kmemleak to track and scan fusion
> object.
>
> V2:
> Add comment, balance braces, move kmemleak_free before free_pages.
> checkpatch passed.
>
> Before patch:
> kmemleak: 1004 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> unreferenced object 0xffff88042584e000 (size 8192):
> backtrace:
> kmemleak_alloc+0x4a/0xa0
> __kmalloc+0xec/0x220
> megasas_alloc_cmdlist_fusion+0x3e/0x140 [megaraid_sas]
> megasas_alloc_cmds_fusion+0x44/0x450 [megaraid_sas]
> megasas_init_adapter_fusion+0x21d/0x6e0 [megaraid_sas]
> megasas_init_fw+0x357/0xd30 [megaraid_sas]
> megasas_probe_one.part.34+0x5be/0x1040 [megaraid_sas]
> megasas_probe_one+0x46/0xc0 [megaraid_sas]
> local_pci_probe+0x45/0xa0
> work_for_cpu_fn+0x14/0x20
> process_one_work+0x149/0x360
> worker_thread+0x1d8/0x3c0
> kthread+0x109/0x140
> ret_from_fork+0x25/0x30
>
> Signed-off-by: Shu Wang <[email protected]>
Reviewed-by: Catalin Marinas <[email protected]>
Thanks.
--
Catalin
On Fri, 2017-09-15 at 13:21 +0800, [email protected] wrote:
> @@ -4548,9 +4556,11 @@ megasas_free_fusion_context(struct megasas_instance *instance)
>
> if (is_vmalloc_addr(fusion))
> vfree(fusion);
> - else
> + else {
> + kmemleak_free(fusion);
> free_pages((ulong)fusion,
> instance->ctrl_context_pages);
> + }
> }
Braces are still not balanced in the above code. This is something
checkpatch should have told you. Anyway:
Reviewed-by: Bart Van Assche <[email protected]>
I think the megaraid fusion code has a deeper problem here.
Instead of playing weird games with get_free_pages and vmalloc
the structure just needs to shrink by moving all the arrays
of MAX_MSIX_QUEUES_FUSION size into a separate allocation for each,
and then we have normall, small kmalloc allocations.
> -----Original Message-----
> From: Christoph Hellwig [mailto:[email protected]]
> Sent: Friday, September 15, 2017 11:30 PM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH V2] megaraid: kmemleak: Track page allocation for
fusion
>
> I think the megaraid fusion code has a deeper problem here.
>
> Instead of playing weird games with get_free_pages and vmalloc the
structure
> just needs to shrink by moving all the arrays of MAX_MSIX_QUEUES_FUSION
> size into a separate allocation for each, and then we have normall,
small
> kmalloc allocations.
Hi Christoph,
We understand your suggestion on shrinking the size of fusion_context so
that we can use kmalloc to allocate the structure.
Size of fusion_context structure now is about 179kB and it is contributed
almost entirely by log_to_span array (~176kB).
The rest of the arrays do not make as much difference to the size.
We will send a new patch that separates allocation for log_to_span array
from fusion_context.
For now it is a Nack for this patch then.
crash> struct -o fusion_context
struct fusion_context {
[0] struct megasas_cmd_fusion **cmd_list;
[8] dma_addr_t req_frames_desc_phys;
[16] u8 *req_frames_desc;
[24] struct dma_pool *io_request_frames_pool;
[32] dma_addr_t io_request_frames_phys;
[40] u8 *io_request_frames;
[48] struct dma_pool *sg_dma_pool;
[56] struct dma_pool *sense_dma_pool;
[64] dma_addr_t reply_frames_desc_phys[128];
[1088] union MPI2_REPLY_DESCRIPTORS_UNION *reply_frames_desc[128];
[2112] struct dma_pool *reply_frames_desc_pool;
[2120] u16 last_reply_idx[128];
[2376] u32 reply_q_depth;
[2380] u32 request_alloc_sz;
[2384] u32 reply_alloc_sz;
[2388] u32 io_frames_alloc_sz;
[2392] struct MPI2_IOC_INIT_RDPQ_ARRAY_ENTRY *rdpq_virt;
[2400] dma_addr_t rdpq_phys;
[2408] u16 max_sge_in_main_msg;
[2410] u16 max_sge_in_chain;
[2412] u8 chain_offset_io_request;
[2413] u8 chain_offset_mfi_pthru;
[2416] struct MR_FW_RAID_MAP_DYNAMIC *ld_map[2];
[2432] dma_addr_t ld_map_phys[2];
[2448] struct MR_DRV_RAID_MAP_ALL *ld_drv_map[2];
[2464] u32 max_map_sz;
[2468] u32 current_map_sz;
[2472] u32 old_map_sz;
[2476] u32 new_map_sz;
[2480] u32 drv_map_sz;
[2484] u32 drv_map_pages;
[2488] struct MR_PD_CFG_SEQ_NUM_SYNC *pd_seq_sync[2];
[2504] dma_addr_t pd_seq_phys[2];
[2520] u8 fast_path_io;
[2528] struct LD_LOAD_BALANCE_INFO *load_balance_info;
[2536] u32 load_balance_info_pages;
[2544] LD_SPAN_INFO log_to_span[256];
[182768] u8 adapter_type;
[182776] struct LD_STREAM_DETECT **stream_detect_by_ld; }
SIZE: 182784
Thanks,
Shivasharan
Oh, I missed log_to_span. Well, in that case log_to_span is
_the_ candidate for moving into a separate allocation.
And in fact you're probably better off by using a sensible data
structure for it, e.g. a radix tree.
> -----Original Message-----
> From: Christoph Hellwig [mailto:[email protected]]
> Sent: Monday, September 18, 2017 9:52 PM
> To: Shivasharan Srikanteshwara
> Cc: Christoph Hellwig; [email protected]; Kashyap Desai; Sumit Saxena;
> [email protected]; [email protected];
> PDL,MEGARAIDLINUX; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH V2] megaraid: kmemleak: Track page allocation for
fusion
>
> Oh, I missed log_to_span. Well, in that case log_to_span is _the_
candidate
> for moving into a separate allocation.
>
> And in fact you're probably better off by using a sensible data
structure for it,
> e.g. a radix tree.
Thanks Christoph.
We will make the changes suggested in phased approach.
First we will fix kmemleak false positives by moving log_to_span
allocation separate from fusion_context.
The data structure change would involve major changes which affects IO
path as well.
Also driver expects log_to_span and other data structures to be available
at load time itself. Considering this, we need to understand if radix tree
would be a good choice for the change.
Based on internal discussions, we see other similar arrays in driver code
that we can change similarly eg. load_balance_info.
This is definitely something to add to our to-do lists.
These changes need to go through our internal regression test cycle and
then submit it to upstream.
Best regards,
Shivasharan