From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752878AbcBVTUO (ORCPT ); Mon, 22 Feb 2016 14:20:14 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:31074 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751514AbcBVTUM (ORCPT ); Mon, 22 Feb 2016 14:20:12 -0500 Date: Mon, 22 Feb 2016 11:19:46 -0800 From: Calvin Owens To: Corey Minyard CC: , , Subject: Re: [PATCH][QUESTION] Intentional memory leak in ipmi_msghandler? Message-ID: <20160222191946.GA980385@devbig337.prn1.facebook.com> References: <20160219064132.GA2859126@devbig337.prn1.facebook.com> <56C7154F.3040604@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <56C7154F.3040604@acm.org> User-Agent: Mutt/1.5.20 (2009-12-10) X-Originating-IP: [192.168.52.123] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-02-22_07:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday 02/19 at 07:14 -0600, Corey Minyard wrote: > On 02/19/2016 12:41 AM, Calvin Owens wrote: > >Hello, > > > >I've got a few boxes that are leaking memory in handle_new_recv_msgs() > >in ipmi_msghandler. AFAICS this is intentional, there's even an explicit > >counter that tracks the number of times smi_msg is leaked. > > Are you 100% sure about this? I'm absolutely certain this is where I'm leaking: I threw in a printk() and saw exact correlation between the number of times I saw that and the number of kmalloc-1024 objects leaked. But... > There's no intentional leak, a negative return from this function > means the message was used for another purpose and thus shouldn't be > freed. There's only one situation where this happens and you should > never hit it in normal operation. This is actually extremely helpful: we have some horrible non-upstream code behind this that I thought I had ruled out being at fault here, but this sounds like it might be. In any case, I won't waste your time any more until I can reproduce it on upstream (which is unfortunately impossible on this particular HW I see the leak on, otherwise I would have done that in the first place...). Thanks very much for the prompt response, I really appreciate it. Calvin > >I'm guessing there was a reason for doing this, but there wasn't any > >discussion about it on LKML when the patch was accepted. Can you clarify > >why something like the below patch won't work? I tried it on one of my > >leaky boxes and nothing obviously horrible happened. > > Well, that's because nothing probably happened, and it probably had no > effect on the leak. A better comment on this code is probably in > order. But that patch is incorrect. > > I doubt the leak is here. If you are having a leak, is it possible to > characterize it better? Are you handling commands from IPMB? Are you > handling LAN commands here? > > -corey > > > > >Thanks, > >Calvin > > > >----8<---- > >From: Calvin Owens > >Subject: [PATCH] ipmi_msghandler: Don't leak memory on errors > > > >Signed-off-by: Calvin Owens > >--- > > drivers/char/ipmi/ipmi_msghandler.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > >diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c > >index 94fb407..ed82ffa 100644 > >--- a/drivers/char/ipmi/ipmi_msghandler.c > >+++ b/drivers/char/ipmi/ipmi_msghandler.c > >@@ -3834,10 +3834,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf) > > break; > > } else { > > list_del(&smi_msg->link); > >- if (rv == 0) > >- /* Message handled */ > >- ipmi_free_smi_msg(smi_msg); > >- /* If rv < 0, fatal error, del but don't free. */ > >+ ipmi_free_smi_msg(smi_msg); > > } > > } > > if (!run_to_completion) >