Re: [PATCH][QUESTION] Intentional memory leak in ipmi_msghandler?

[Calvin Owens <calvinowens@fb.com>]

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 <calvinowens@fb.com>
> >Subject: [PATCH] ipmi_msghandler: Don't leak memory on errors
> >
> >Signed-off-by: Calvin Owens <calvinowens@fb.com>
> >---
> >  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)
>