Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732713 > unrolled thread

[PATCH V2] megaraid: kmemleak: Track page allocation for fusion

Back to article view | Back to linux.kernel

Contents

  [PATCH V2] megaraid: kmemleak: Track page allocation for fusion shuwang@redhat.com - 2017-09-15 07:30 +0200
    Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Catalin Marinas <catalin.marinas@arm.com> - 2017-09-15 15:20 +0200
    Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Bart Van Assche <Bart.VanAssche@wdc.com> - 2017-09-15 18:20 +0200
    Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Christoph Hellwig <hch@infradead.org> - 2017-09-15 20:10 +0200
      RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Shivasharan Srikanteshwara          <shivasharan.srikanteshwara@broadcom.com> - 2017-09-18 17:20 +0200
        Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Christoph Hellwig <hch@infradead.org> - 2017-09-18 18:30 +0200
          RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion Shivasharan Srikanteshwara          <shivasharan.srikanteshwara@broadcom.com> - 2017-09-25 10:50 +0200

#1732713 — [PATCH V2] megaraid: kmemleak: Track page allocation for fusion

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

[toc] | [next] | [standalone]

#1732877

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

[toc] | [prev] | [next] | [standalone]

#1732941

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>

[toc] | [prev] | [next] | [standalone]

#1732995

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.

[toc] | [prev] | [next] | [standalone]

#1734217

> -----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

[toc] | [prev] | [next] | [standalone]

#1734277

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.

[toc] | [prev] | [next] | [standalone]

#1738833

> -----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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web