* [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
@ 2017-09-15 5:21 shuwang
2017-09-15 13:12 ` Catalin Marinas
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: shuwang @ 2017-09-15 5:21 UTC (permalink / raw)
To: kashyap.desai, sumit.saxena, shivasharan.srikanteshwara, jejb,
martin.petersen
Cc: megaraidlinux.pdl, linux-scsi, linux-kernel, chuhu, yizhan,
catalin.marinas, Shu Wang
From: Shu Wang <shuwang@redhat.com>
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 <shuwang@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-15 5:21 [PATCH V2] megaraid: kmemleak: Track page allocation for fusion shuwang
@ 2017-09-15 13:12 ` Catalin Marinas
2017-09-15 16:16 ` Bart Van Assche
2017-09-15 18:00 ` Christoph Hellwig
2 siblings, 0 replies; 7+ messages in thread
From: Catalin Marinas @ 2017-09-15 13:12 UTC (permalink / raw)
To: shuwang
Cc: kashyap.desai, sumit.saxena, shivasharan.srikanteshwara, jejb,
martin.petersen, megaraidlinux.pdl, linux-scsi, linux-kernel,
chuhu, yizhan
On Fri, Sep 15, 2017 at 01:21:52PM +0800, shuwang@redhat.com wrote:
> From: Shu Wang <shuwang@redhat.com>
>
> 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 <shuwang@redhat.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Thanks.
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-15 5:21 [PATCH V2] megaraid: kmemleak: Track page allocation for fusion shuwang
2017-09-15 13:12 ` Catalin Marinas
@ 2017-09-15 16:16 ` Bart Van Assche
2017-09-15 18:00 ` Christoph Hellwig
2 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2017-09-15 16:16 UTC (permalink / raw)
To: jejb, martin.petersen, shuwang, kashyap.desai,
shivasharan.srikanteshwara, sumit.saxena
Cc: linux-scsi, linux-kernel, chuhu, catalin.marinas, yizhan,
megaraidlinux.pdl
On Fri, 2017-09-15 at 13:21 +0800, shuwang@redhat.com 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 <bart.vanassche@wdc.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-15 5:21 [PATCH V2] megaraid: kmemleak: Track page allocation for fusion shuwang
2017-09-15 13:12 ` Catalin Marinas
2017-09-15 16:16 ` Bart Van Assche
@ 2017-09-15 18:00 ` Christoph Hellwig
2017-09-18 15:10 ` Shivasharan Srikanteshwara
2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2017-09-15 18:00 UTC (permalink / raw)
To: shuwang
Cc: kashyap.desai, sumit.saxena, shivasharan.srikanteshwara, jejb,
martin.petersen, megaraidlinux.pdl, linux-scsi, linux-kernel,
chuhu, yizhan, catalin.marinas
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-15 18:00 ` Christoph Hellwig
@ 2017-09-18 15:10 ` Shivasharan Srikanteshwara
2017-09-18 16:21 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Shivasharan Srikanteshwara @ 2017-09-18 15:10 UTC (permalink / raw)
To: Christoph Hellwig, shuwang
Cc: Kashyap Desai, Sumit Saxena, jejb, martin.petersen,
PDL,MEGARAIDLINUX, linux-scsi, linux-kernel, chuhu, yizhan,
catalin.marinas
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, September 15, 2017 11:30 PM
> To: shuwang@redhat.com
> Cc: kashyap.desai@broadcom.com; sumit.saxena@broadcom.com;
> shivasharan.srikanteshwara@broadcom.com; jejb@linux.vnet.ibm.com;
> martin.petersen@oracle.com; megaraidlinux.pdl@broadcom.com; linux-
> scsi@vger.kernel.org; linux-kernel@vger.kernel.org; chuhu@redhat.com;
> yizhan@redhat.com; catalin.marinas@arm.com
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-18 15:10 ` Shivasharan Srikanteshwara
@ 2017-09-18 16:21 ` Christoph Hellwig
2017-09-25 8:08 ` Shivasharan Srikanteshwara
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2017-09-18 16:21 UTC (permalink / raw)
To: Shivasharan Srikanteshwara
Cc: Christoph Hellwig, shuwang, Kashyap Desai, Sumit Saxena, jejb,
martin.petersen, PDL,MEGARAIDLINUX, linux-scsi, linux-kernel,
chuhu, yizhan, catalin.marinas
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
2017-09-18 16:21 ` Christoph Hellwig
@ 2017-09-25 8:08 ` Shivasharan Srikanteshwara
0 siblings, 0 replies; 7+ messages in thread
From: Shivasharan Srikanteshwara @ 2017-09-25 8:08 UTC (permalink / raw)
To: Christoph Hellwig
Cc: shuwang, Kashyap Desai, Sumit Saxena, jejb, martin.petersen,
PDL,MEGARAIDLINUX, linux-scsi, linux-kernel, chuhu, yizhan,
catalin.marinas
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Monday, September 18, 2017 9:52 PM
> To: Shivasharan Srikanteshwara
> Cc: Christoph Hellwig; shuwang@redhat.com; Kashyap Desai; Sumit Saxena;
> jejb@linux.vnet.ibm.com; martin.petersen@oracle.com;
> PDL,MEGARAIDLINUX; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org; chuhu@redhat.com; yizhan@redhat.com;
> catalin.marinas@arm.com
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-25 8:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-15 5:21 [PATCH V2] megaraid: kmemleak: Track page allocation for fusion shuwang
2017-09-15 13:12 ` Catalin Marinas
2017-09-15 16:16 ` Bart Van Assche
2017-09-15 18:00 ` Christoph Hellwig
2017-09-18 15:10 ` Shivasharan Srikanteshwara
2017-09-18 16:21 ` Christoph Hellwig
2017-09-25 8:08 ` Shivasharan Srikanteshwara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).